Re: [PATCH V2 1/2] ACPI: PCC: add check for platform interrupt

[Sudeep Holla <sudeep.holla@arm.com>]

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>

-- 
Regards,
Sudeep