[2/3] ACPI: PCC: add check for platform interrupt
Commit Message
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
On Thu, Nov 10, 2022 at 09:50:33AM +0800, Huisong Li wrote:
> 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;
> }
>
Your patch seems to be not based on the upstream.
Commit f890157e61b8 ("ACPI: PCC: Release resources on address space setup
failure path") has addressed it already.
> 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;
> + }
Indeed, I supported only interrupt case and this approach is better than
checking it in handler atleast until we add support for polling based
transfers in future(hope that never happens, but you never know)
在 2022/11/10 18:36, Sudeep Holla 写道:
> On Thu, Nov 10, 2022 at 09:50:33AM +0800, Huisong Li wrote:
>> 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;
>> }
>>
> Your patch seems to be not based on the upstream.
> Commit f890157e61b8 ("ACPI: PCC: Release resources on address space setup
> failure path") has addressed it already.
I make this patch based on the commit f890157e61b8.
Here is to unify the relese resources path.
>
>> 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;
>> + }
> Indeed, I supported only interrupt case and this approach is better than
> checking it in handler atleast until we add support for polling based
> transfers in future(hope that never happens, but you never know)
Yes
>
@@ -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);