[V2,1/2] ACPI: PCC: add check for platform interrupt
Commit Message
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(-)
Comments
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.
> }
>
> 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>
在 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>
>
@@ -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);