drivers/acpi/acpi_pcc.c | 47 +++++++++++++++++++++++++---------------- drivers/mailbox/pcc.c | 13 ++++++++---- include/acpi/actbl2.h | 2 +- 3 files changed, 39 insertions(+), 23 deletions(-)
This series optimizes PCC codes as follows: - Rename platform interrupt bit macro name to make it more reasonable and readable. - add check for platform interrupt in acpi_pcc.c In addition, this series fixes a problem that mailbox channel can still be requested successfully when fail to initialize PCC. Huisong Li (3): mailbox: pcc: rename platform interrupt bit macro name ACPI: PCC: add check for platform interrupt mailbox: pcc: fix 'pcc_chan_count' when fail to initialize PCC drivers/acpi/acpi_pcc.c | 47 +++++++++++++++++++++++++---------------- drivers/mailbox/pcc.c | 13 ++++++++---- include/acpi/actbl2.h | 2 +- 3 files changed, 39 insertions(+), 23 deletions(-) -- 2.22.0
This series clarifies the dependency of PCC OpRegion on interrupt and reset
pcc_chan_count to zero in case of PCC probe failure.
---
- v3: fix commit log and goto tags.
- v2: drop the patch that rename platform interrupt bit macro
---
Huisong Li (2):
ACPI: PCC: Setup PCC Opregion handler only if platform interrupt is
available
mailbox: pcc: Reset pcc_chan_count to zero in case of PCC probe
failure
drivers/acpi/acpi_pcc.c | 47 +++++++++++++++++++++++++----------------
drivers/mailbox/pcc.c | 1 +
2 files changed, 30 insertions(+), 18 deletions(-)
--
2.22.0
On Sat, Nov 12, 2022 at 3:07 AM Huisong Li <lihuisong@huawei.com> wrote: > > This series clarifies the dependency of PCC OpRegion on interrupt and reset > pcc_chan_count to zero in case of PCC probe failure. > > > --- > - v3: fix commit log and goto tags. > - v2: drop the patch that rename platform interrupt bit macro > --- > > Huisong Li (2): > ACPI: PCC: Setup PCC Opregion handler only if platform interrupt is > available > mailbox: pcc: Reset pcc_chan_count to zero in case of PCC probe > failure > > drivers/acpi/acpi_pcc.c | 47 +++++++++++++++++++++++++---------------- > drivers/mailbox/pcc.c | 1 + > 2 files changed, 30 insertions(+), 18 deletions(-) > > -- Both applied as 6.2 material, thanks!
Currently, PCC OpRegion handler depends on the availability of platform
interrupt to be functional currently. If it is not available, the OpRegion
can't be executed successfully or the desired outcome won't be possible.
So let's reject setting up the PCC OpRegion handler on the platform if
it doesn't support or have platform interrupt available.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/acpi/acpi_pcc.c | 47 +++++++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
index 3e252be047b8..07a034a53aca 100644
--- a/drivers/acpi/acpi_pcc.c
+++ b/drivers/acpi/acpi_pcc.c
@@ -53,6 +53,7 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
struct pcc_data *data;
struct acpi_pcc_info *ctx = handler_context;
struct pcc_mbox_chan *pcc_chan;
+ static acpi_status ret;
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
@@ -69,23 +70,35 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
if (IS_ERR(data->pcc_chan)) {
pr_err("Failed to find PCC channel for subspace %d\n",
ctx->subspace_id);
- kfree(data);
- return AE_NOT_FOUND;
+ ret = AE_NOT_FOUND;
+ goto err_free_data;
}
pcc_chan = data->pcc_chan;
+ if (!pcc_chan->mchan->mbox->txdone_irq) {
+ pr_err("This channel-%d does not support interrupt.\n",
+ ctx->subspace_id);
+ ret = AE_SUPPORT;
+ goto err_free_channel;
+ }
data->pcc_comm_addr = acpi_os_ioremap(pcc_chan->shmem_base_addr,
pcc_chan->shmem_size);
if (!data->pcc_comm_addr) {
pr_err("Failed to ioremap PCC comm region mem for %d\n",
ctx->subspace_id);
- pcc_mbox_free_channel(data->pcc_chan);
- kfree(data);
- return AE_NO_MEMORY;
+ ret = AE_NO_MEMORY;
+ goto err_free_channel;
}
*region_context = data;
return AE_OK;
+
+err_free_channel:
+ pcc_mbox_free_channel(data->pcc_chan);
+err_free_data:
+ kfree(data);
+
+ return ret;
}
static acpi_status
@@ -106,19 +119,17 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
if (ret < 0)
return AE_ERROR;
- if (data->pcc_chan->mchan->mbox->txdone_irq) {
- /*
- * pcc_chan->latency is just a Nominal value. In reality the remote
- * processor could be much slower to reply. So add an arbitrary
- * amount of wait on top of Nominal.
- */
- usecs_lat = PCC_CMD_WAIT_RETRIES_NUM * data->pcc_chan->latency;
- ret = wait_for_completion_timeout(&data->done,
- usecs_to_jiffies(usecs_lat));
- if (ret == 0) {
- pr_err("PCC command executed timeout!\n");
- return AE_TIME;
- }
+ /*
+ * pcc_chan->latency is just a Nominal value. In reality the remote
+ * processor could be much slower to reply. So add an arbitrary
+ * amount of wait on top of Nominal.
+ */
+ usecs_lat = PCC_CMD_WAIT_RETRIES_NUM * data->pcc_chan->latency;
+ ret = wait_for_completion_timeout(&data->done,
+ usecs_to_jiffies(usecs_lat));
+ if (ret == 0) {
+ pr_err("PCC command executed timeout!\n");
+ return AE_TIME;
}
mbox_chan_txdone(data->pcc_chan->mchan, ret);
--
2.22.0Currently, 'pcc_chan_count' is remains set to a non-zero value if PCC
subspaces are parsed successfully but something else fail later during
the initial PCC probing phase. This will result in pcc_mbox_request_channel
trying to access the resources that are not initialised or allocated and
may end up in a system crash.
Reset pcc_chan_count to 0 when the PCC probe fails in order to prevent
the possible issue as described above.
Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/mailbox/pcc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3c2bc0ca454c..105d46c9801b 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -743,6 +743,7 @@ static int __init pcc_init(void)
if (IS_ERR(pcc_pdev)) {
pr_debug("Err creating PCC platform bundle\n");
+ pcc_chan_count = 0;
return PTR_ERR(pcc_pdev);
}
--
2.22.0This series addes check for platform interrupt in acpi_pcc.c and fixes a problem that mailbox channel can still be requested successfully when fail to initialize PCC. --- -v2: drop the patch that rename platform interrupt bit macro --- Huisong Li (2): ACPI: PCC: add check for platform interrupt mailbox: pcc: fix 'pcc_chan_count' when fail to initialize PCC drivers/acpi/acpi_pcc.c | 47 +++++++++++++++++++++++++---------------- drivers/mailbox/pcc.c | 1 + 2 files changed, 30 insertions(+), 18 deletions(-) -- 2.22.0
Currently, PCC Operation Region driver senses the completion of command by
interrupt way. If platform can not generate an interrupt when a command
complete, the caller never gets the desired result. So let's reject the
setup of the PCC address space on platform that do not support interrupt
mode.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/acpi/acpi_pcc.c | 47 +++++++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
index 3e252be047b8..8efd08e469aa 100644
--- a/drivers/acpi/acpi_pcc.c
+++ b/drivers/acpi/acpi_pcc.c
@@ -53,6 +53,7 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
struct pcc_data *data;
struct acpi_pcc_info *ctx = handler_context;
struct pcc_mbox_chan *pcc_chan;
+ static acpi_status ret;
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
@@ -69,23 +70,35 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
if (IS_ERR(data->pcc_chan)) {
pr_err("Failed to find PCC channel for subspace %d\n",
ctx->subspace_id);
- kfree(data);
- return AE_NOT_FOUND;
+ ret = AE_NOT_FOUND;
+ goto request_channel_fail;
}
pcc_chan = data->pcc_chan;
+ if (!pcc_chan->mchan->mbox->txdone_irq) {
+ pr_err("This channel-%d does not support interrupt.\n",
+ ctx->subspace_id);
+ ret = AE_SUPPORT;
+ goto request_channel_fail;
+ }
data->pcc_comm_addr = acpi_os_ioremap(pcc_chan->shmem_base_addr,
pcc_chan->shmem_size);
if (!data->pcc_comm_addr) {
pr_err("Failed to ioremap PCC comm region mem for %d\n",
ctx->subspace_id);
- pcc_mbox_free_channel(data->pcc_chan);
- kfree(data);
- return AE_NO_MEMORY;
+ ret = AE_NO_MEMORY;
+ goto ioremap_fail;
}
*region_context = data;
return AE_OK;
+
+ioremap_fail:
+ pcc_mbox_free_channel(data->pcc_chan);
+request_channel_fail:
+ kfree(data);
+
+ return ret;
}
static acpi_status
@@ -106,19 +119,17 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
if (ret < 0)
return AE_ERROR;
- if (data->pcc_chan->mchan->mbox->txdone_irq) {
- /*
- * pcc_chan->latency is just a Nominal value. In reality the remote
- * processor could be much slower to reply. So add an arbitrary
- * amount of wait on top of Nominal.
- */
- usecs_lat = PCC_CMD_WAIT_RETRIES_NUM * data->pcc_chan->latency;
- ret = wait_for_completion_timeout(&data->done,
- usecs_to_jiffies(usecs_lat));
- if (ret == 0) {
- pr_err("PCC command executed timeout!\n");
- return AE_TIME;
- }
+ /*
+ * pcc_chan->latency is just a Nominal value. In reality the remote
+ * processor could be much slower to reply. So add an arbitrary
+ * amount of wait on top of Nominal.
+ */
+ usecs_lat = PCC_CMD_WAIT_RETRIES_NUM * data->pcc_chan->latency;
+ ret = wait_for_completion_timeout(&data->done,
+ usecs_to_jiffies(usecs_lat));
+ if (ret == 0) {
+ pr_err("PCC command executed timeout!\n");
+ return AE_TIME;
}
mbox_chan_txdone(data->pcc_chan->mchan, ret);
--
2.22.0Change $subject to:
"ACPI: PCC: Setup PCC Opregion handler only if platform interrupt is available"
On Fri, Nov 11, 2022 at 10:44:47AM +0800, Huisong Li wrote:
> Currently, PCC Operation Region driver senses the completion of command by
> interrupt way. If platform can not generate an interrupt when a command
> complete, the caller never gets the desired result. So let's reject the
> setup of the PCC address space on platform that do not support interrupt
> mode.
>
Please reword something like below:
"Currently, PCC OpRegion handler depends on the availability of platform
interrupt to be functional currently. If it is not available, the OpRegion
can't be executed successfully or the desired outcome won't be possible.
So let's reject setting up the PCC OpRegion handler on the platform if
it doesn't support or have platform interrupt available"
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> drivers/acpi/acpi_pcc.c | 47 +++++++++++++++++++++++++----------------
> 1 file changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
> index 3e252be047b8..8efd08e469aa 100644
> --- a/drivers/acpi/acpi_pcc.c
> +++ b/drivers/acpi/acpi_pcc.c
> @@ -53,6 +53,7 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
> struct pcc_data *data;
> struct acpi_pcc_info *ctx = handler_context;
> struct pcc_mbox_chan *pcc_chan;
> + static acpi_status ret;
>
> data = kzalloc(sizeof(*data), GFP_KERNEL);
> if (!data)
> @@ -69,23 +70,35 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
> if (IS_ERR(data->pcc_chan)) {
> pr_err("Failed to find PCC channel for subspace %d\n",
> ctx->subspace_id);
> - kfree(data);
> - return AE_NOT_FOUND;
> + ret = AE_NOT_FOUND;
> + goto request_channel_fail;
The labels are confusing IMO. I would do 's/request_channel_fail/err_free_data/'
to indicate what is exactly done there rather than just describing what
failed.
> }
>
> pcc_chan = data->pcc_chan;
> + if (!pcc_chan->mchan->mbox->txdone_irq) {
> + pr_err("This channel-%d does not support interrupt.\n",
> + ctx->subspace_id);
> + ret = AE_SUPPORT;
> + goto request_channel_fail;
This is wrong, you must goto "ioremap_fail" as you have been successful in
opening the channel and now need to free the same.
> + }
> data->pcc_comm_addr = acpi_os_ioremap(pcc_chan->shmem_base_addr,
> pcc_chan->shmem_size);
> if (!data->pcc_comm_addr) {
> pr_err("Failed to ioremap PCC comm region mem for %d\n",
> ctx->subspace_id);
> - pcc_mbox_free_channel(data->pcc_chan);
> - kfree(data);
> - return AE_NO_MEMORY;
> + ret = AE_NO_MEMORY;
> + goto ioremap_fail;
Again I prefer 's/ioremap_fail/err_free_channel/' or something similar.
With all the above comments incorporated, you can add:
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
--
Regards,
Sudeep
在 2022/11/11 22:26, Sudeep Holla 写道:
> Change $subject to:
> "ACPI: PCC: Setup PCC Opregion handler only if platform interrupt is available"
>
> On Fri, Nov 11, 2022 at 10:44:47AM +0800, Huisong Li wrote:
>> Currently, PCC Operation Region driver senses the completion of command by
>> interrupt way. If platform can not generate an interrupt when a command
>> complete, the caller never gets the desired result. So let's reject the
>> setup of the PCC address space on platform that do not support interrupt
>> mode.
>>
> Please reword something like below:
>
> "Currently, PCC OpRegion handler depends on the availability of platform
> interrupt to be functional currently. If it is not available, the OpRegion
> can't be executed successfully or the desired outcome won't be possible.
> So let's reject setting up the PCC OpRegion handler on the platform if
> it doesn't support or have platform interrupt available"
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> drivers/acpi/acpi_pcc.c | 47 +++++++++++++++++++++++++----------------
>> 1 file changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
>> index 3e252be047b8..8efd08e469aa 100644
>> --- a/drivers/acpi/acpi_pcc.c
>> +++ b/drivers/acpi/acpi_pcc.c
>> @@ -53,6 +53,7 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
>> struct pcc_data *data;
>> struct acpi_pcc_info *ctx = handler_context;
>> struct pcc_mbox_chan *pcc_chan;
>> + static acpi_status ret;
>>
>> data = kzalloc(sizeof(*data), GFP_KERNEL);
>> if (!data)
>> @@ -69,23 +70,35 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
>> if (IS_ERR(data->pcc_chan)) {
>> pr_err("Failed to find PCC channel for subspace %d\n",
>> ctx->subspace_id);
>> - kfree(data);
>> - return AE_NOT_FOUND;
>> + ret = AE_NOT_FOUND;
>> + goto request_channel_fail;
> The labels are confusing IMO. I would do 's/request_channel_fail/err_free_data/'
> to indicate what is exactly done there rather than just describing what
> failed.
Good idea. Ack.
>
>> }
>>
>> pcc_chan = data->pcc_chan;
>> + if (!pcc_chan->mchan->mbox->txdone_irq) {
>> + pr_err("This channel-%d does not support interrupt.\n",
>> + ctx->subspace_id);
>> + ret = AE_SUPPORT;
>> + goto request_channel_fail;
> This is wrong, you must goto "ioremap_fail" as you have been successful in
> opening the channel and now need to free the same.
Sorry, I will fix it in v3.
>
>> + }
>> data->pcc_comm_addr = acpi_os_ioremap(pcc_chan->shmem_base_addr,
>> pcc_chan->shmem_size);
>> if (!data->pcc_comm_addr) {
>> pr_err("Failed to ioremap PCC comm region mem for %d\n",
>> ctx->subspace_id);
>> - pcc_mbox_free_channel(data->pcc_chan);
>> - kfree(data);
>> - return AE_NO_MEMORY;
>> + ret = AE_NO_MEMORY;
>> + goto ioremap_fail;
> Again I prefer 's/ioremap_fail/err_free_channel/' or something similar.
>
> With all the above comments incorporated, you can add:
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>
Currently, 'pcc_chan_count' is a non-zero value if PCC subspaces are parsed
successfully and subsequent processes is failure during initializing PCC
process. This may cause that pcc_mbox_request_channel() can still be
executed successfully , which will misleads the caller that this channel is
available.
Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/mailbox/pcc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3c2bc0ca454c..105d46c9801b 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -743,6 +743,7 @@ static int __init pcc_init(void)
if (IS_ERR(pcc_pdev)) {
pr_debug("Err creating PCC platform bundle\n");
+ pcc_chan_count = 0;
return PTR_ERR(pcc_pdev);
}
--
2.22.0Change $subject as
"mailbox: pcc: Reset pcc_chan_count to zero in case of PCC probe failure"
On Fri, Nov 11, 2022 at 10:44:48AM +0800, Huisong Li wrote:
> Currently, 'pcc_chan_count' is a non-zero value if PCC subspaces are parsed
> successfully and subsequent processes is failure during initializing PCC
> process. This may cause that pcc_mbox_request_channel() can still be
> executed successfully , which will misleads the caller that this channel is
> available.
>
I would reword this something like:
"Currently, 'pcc_chan_count' is remains set to a non-zero value if PCC
subspaces are parsed successfully but something else fail later during
the initial PCC probing phase. This will result in pcc_mbox_request_channel()
trying to access the resources that are not initialised or allocated and
may end up in a system crash.
Reset pcc_chan_count to 0 when the PCC probe fails in order to prevent
the possible issue as described above.
"
> Fixes: ce028702ddbc ("mailbox: pcc: Move bulk of PCCT parsing into pcc_mbox_probe")
Other than that,
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
--
Regards,
Sudeep
© 2016 - 2025 Red Hat, Inc.