| Message ID | 20221110015034.7943-3-lihuisong@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ACPI: PCC: optimize pcc codes and fix one bug | expand |
在 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 >
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);
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(-)