[PATCH 0/3] ACPI: PCC: optimize pcc codes and fix one bug

[PATCH 0/3] ACPI: PCC: optimize pcc codes and fix one bug

[Huisong Li]

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

[PATCH 1/3] mailbox: pcc: rename platform interrupt bit macro name

[Huisong Li]

Currently, the name of platform interrupt bit macro, ACPI_PCCT_DOORBELL,
is not very appropriate. The doorbell is generally considered as an action
when send mailbox data. Actually, the macro value comes from Platform
Interrupt in Platform Communications Channel Global Flags. If the bit is
'1', it means that the platform is capable of generating an interrupt to
indicate completion of a command.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/mailbox/pcc.c | 2 +-
 include/acpi/actbl2.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3c2bc0ca454c..7cee37dd3b73 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -665,7 +665,7 @@ static int pcc_mbox_probe(struct platform_device *pdev)
 		(unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
 
 	acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
-	if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
+	if (acpi_pcct_tbl->flags & BIT(ACPI_PCCT_FLAGS_PLAT_INTERRUPT_B))
 		pcc_mbox_ctrl->txdone_irq = true;
 
 	for (i = 0; i < count; i++) {
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 655102bc6d14..3840507fdc79 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -1810,7 +1810,7 @@ struct acpi_table_pcct {
 
 /* Values for Flags field above */
 
-#define ACPI_PCCT_DOORBELL              1
+#define ACPI_PCCT_FLAGS_PLAT_INTERRUPT_B              1
 
 /* Values for subtable type in struct acpi_subtable_header */
 
-- 
2.22.0

Re: [PATCH 1/3] mailbox: pcc: rename platform interrupt bit macro name

[Sudeep Holla]

On Thu, Nov 10, 2022 at 09:50:32AM +0800, Huisong Li wrote:
> Currently, the name of platform interrupt bit macro, ACPI_PCCT_DOORBELL,
> is not very appropriate. The doorbell is generally considered as an action
> when send mailbox data. Actually, the macro value comes from Platform
> Interrupt in Platform Communications Channel Global Flags. If the bit is
> '1', it means that the platform is capable of generating an interrupt to
> indicate completion of a command.
>

This is touching ACPICA header file, so it must be submitted to ACPICA
separately following the guidelines in the github and imported into the
kernel.

However, I don't see any point in this change. Yes the language "doorbell"
is not used in this particular context in the spec, but it is implicit from
other parts. I am not opposing the change though if Rafael is OK and ACPICA
project accepts it.

-- 
Regards,
Sudeep

Re: [PATCH 1/3] mailbox: pcc: rename platform interrupt bit macro name

[lihuisong (C)]

在 2022/11/10 18:25, Sudeep Holla 写道:
> On Thu, Nov 10, 2022 at 09:50:32AM +0800, Huisong Li wrote:
>> Currently, the name of platform interrupt bit macro, ACPI_PCCT_DOORBELL,
>> is not very appropriate. The doorbell is generally considered as an action
>> when send mailbox data. Actually, the macro value comes from Platform
>> Interrupt in Platform Communications Channel Global Flags. If the bit is
>> '1', it means that the platform is capable of generating an interrupt to
>> indicate completion of a command.
>>
> This is touching ACPICA header file, so it must be submitted to ACPICA
> separately following the guidelines in the github and imported into the
> kernel.
Got it, thanks.
>
> However, I don't see any point in this change. Yes the language "doorbell"
> is not used in this particular context in the spec, but it is implicit from
> other parts. I am not opposing the change though if Rafael is OK and ACPICA
> project accepts it.
@Rafael, what do you think?
>

Re: [PATCH 1/3] mailbox: pcc: rename platform interrupt bit macro name

[Rafael J. Wysocki]

On Thu, Nov 10, 2022 at 1:17 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2022/11/10 18:25, Sudeep Holla 写道:
> > On Thu, Nov 10, 2022 at 09:50:32AM +0800, Huisong Li wrote:
> >> Currently, the name of platform interrupt bit macro, ACPI_PCCT_DOORBELL,
> >> is not very appropriate. The doorbell is generally considered as an action
> >> when send mailbox data. Actually, the macro value comes from Platform
> >> Interrupt in Platform Communications Channel Global Flags. If the bit is
> >> '1', it means that the platform is capable of generating an interrupt to
> >> indicate completion of a command.
> >>
> > This is touching ACPICA header file, so it must be submitted to ACPICA
> > separately following the guidelines in the github and imported into the
> > kernel.
> Got it, thanks.
> >
> > However, I don't see any point in this change. Yes the language "doorbell"
> > is not used in this particular context in the spec, but it is implicit from
> > other parts. I am not opposing the change though if Rafael is OK and ACPICA
> > project accepts it.
> @Rafael, what do you think?

Well, I wouldn't send a patch to make this change myself, but if you
really care about it, please submit an upstream ACPICA pull request in
the first place and we'll see.

Re: [PATCH 1/3] mailbox: pcc: rename platform interrupt bit macro name

[lihuisong (C)]

在 2022/11/11 3:29, Rafael J. Wysocki 写道:
> On Thu, Nov 10, 2022 at 1:17 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2022/11/10 18:25, Sudeep Holla 写道:
>>> On Thu, Nov 10, 2022 at 09:50:32AM +0800, Huisong Li wrote:
>>>> Currently, the name of platform interrupt bit macro, ACPI_PCCT_DOORBELL,
>>>> is not very appropriate. The doorbell is generally considered as an action
>>>> when send mailbox data. Actually, the macro value comes from Platform
>>>> Interrupt in Platform Communications Channel Global Flags. If the bit is
>>>> '1', it means that the platform is capable of generating an interrupt to
>>>> indicate completion of a command.
>>>>
>>> This is touching ACPICA header file, so it must be submitted to ACPICA
>>> separately following the guidelines in the github and imported into the
>>> kernel.
>> Got it, thanks.
>>> However, I don't see any point in this change. Yes the language "doorbell"
>>> is not used in this particular context in the spec, but it is implicit from
>>> other parts. I am not opposing the change though if Rafael is OK and ACPICA
>>> project accepts it.
>> @Rafael, what do you think?
> Well, I wouldn't send a patch to make this change myself, but if you
> really care about it, please submit an upstream ACPICA pull request in
> the first place and we'll see.
All right. Indeed, it doesn't matter. Ignore it.
> .

[PATCH 2/3] ACPI: PCC: add check for platform interrupt

[Huisong Li]

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.0

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

[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.

>  	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)

-- 
Regards,
Sudeep

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

[lihuisong (C)]

在 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
>

[PATCH 3/3] mailbox: pcc: fix 'pcc_chan_count' when fail to initialize PCC

[Huisong Li]

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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 7cee37dd3b73..47d70c5884e3 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -294,6 +294,7 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 		pr_err("Channel not found for idx: %d\n", subspace_id);
 		return ERR_PTR(-EBUSY);
 	}
+
 	dev = chan->mbox->dev;
 
 	spin_lock_irqsave(&chan->lock, flags);
@@ -735,7 +736,8 @@ static int __init pcc_init(void)
 
 	if (ret) {
 		pr_debug("ACPI PCC probe failed.\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out;
 	}
 
 	pcc_pdev = platform_create_bundle(&pcc_mbox_driver,
@@ -743,10 +745,13 @@ static int __init pcc_init(void)
 
 	if (IS_ERR(pcc_pdev)) {
 		pr_debug("Err creating PCC platform bundle\n");
-		return PTR_ERR(pcc_pdev);
+		ret = PTR_ERR(pcc_pdev);
+		goto out;
 	}
 
-	return 0;
+out:
+	pcc_chan_count = 0;
+	return ret;
 }
 
 /*
-- 
2.22.0

Re: [PATCH 3/3] mailbox: pcc: fix 'pcc_chan_count' when fail to initialize PCC

[Sudeep Holla]

On Thu, Nov 10, 2022 at 09:50:34AM +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.
> 
> 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 7cee37dd3b73..47d70c5884e3 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -294,6 +294,7 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>  		pr_err("Channel not found for idx: %d\n", subspace_id);
>  		return ERR_PTR(-EBUSY);
>  	}
> +

Spurious/not needed change ?

>  	dev = chan->mbox->dev;
>  
>  	spin_lock_irqsave(&chan->lock, flags);
> @@ -735,7 +736,8 @@ static int __init pcc_init(void)
>  
>  	if (ret) {
>  		pr_debug("ACPI PCC probe failed.\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto out;

Not needed, we don't set pcc_chan_count if the probe failed.

>  	}
>  
>  	pcc_pdev = platform_create_bundle(&pcc_mbox_driver,
> @@ -743,10 +745,13 @@ static int __init pcc_init(void)
>  
>  	if (IS_ERR(pcc_pdev)) {
>  		pr_debug("Err creating PCC platform bundle\n");
> -		return PTR_ERR(pcc_pdev);
> +		ret = PTR_ERR(pcc_pdev);

You just need to set pcc_chan_count to 0 here, so no need for goto.

-- 
Regards,
Sudeep

Re: [PATCH 3/3] mailbox: pcc: fix 'pcc_chan_count' when fail to initialize PCC

[lihuisong (C)]

在 2022/11/10 18:44, Sudeep Holla 写道:
> On Thu, Nov 10, 2022 at 09:50:34AM +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.
>>
>> 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 | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 7cee37dd3b73..47d70c5884e3 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -294,6 +294,7 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>>   		pr_err("Channel not found for idx: %d\n", subspace_id);
>>   		return ERR_PTR(-EBUSY);
>>   	}
>> +
> Spurious/not needed change ?
Ack
>
>>   	dev = chan->mbox->dev;
>>   
>>   	spin_lock_irqsave(&chan->lock, flags);
>> @@ -735,7 +736,8 @@ static int __init pcc_init(void)
>>   
>>   	if (ret) {
>>   		pr_debug("ACPI PCC probe failed.\n");
>> -		return -ENODEV;
>> +		ret = -ENODEV;
>> +		goto out;
> Not needed, we don't set pcc_chan_count if the probe failed.
You are right. will fix it in v2, thanks.
>
>>   	}
>>   
>>   	pcc_pdev = platform_create_bundle(&pcc_mbox_driver,
>> @@ -743,10 +745,13 @@ static int __init pcc_init(void)
>>   
>>   	if (IS_ERR(pcc_pdev)) {
>>   		pr_debug("Err creating PCC platform bundle\n");
>> -		return PTR_ERR(pcc_pdev);
>> +		ret = PTR_ERR(pcc_pdev);
> You just need to set pcc_chan_count to 0 here, so no need for goto.
Ack
>

[PATCH V2 0/2] optimize pcc code and fix one bug

[Huisong Li]

This 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

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

[Huisong Li]

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.0

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

[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.

>  	}
>  
>  	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

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

[lihuisong (C)]

在 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>
>

[PATCH V2 2/2] mailbox: pcc: fix 'pcc_chan_count' when fail to initialize PCC

[Huisong Li]

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.0

Re: [PATCH V2 2/2] mailbox: pcc: fix 'pcc_chan_count' when fail to initialize PCC

[Sudeep Holla]

Change $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

[PATCH V3 0/2] Optimize PCC OpRegion code and reset pcc_chan_count

[Huisong Li]

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

[PATCH V3 1/2] ACPI: PCC: Setup PCC Opregion handler only if platform interrupt is available

[Huisong Li]

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.0

[PATCH V3 2/2] mailbox: pcc: Reset pcc_chan_count to zero in case of PCC probe failure

[Huisong Li]

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")
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.0

Re: [PATCH V3 0/2] Optimize PCC OpRegion code and reset pcc_chan_count

[Rafael J. Wysocki]

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!