All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] ACPI: PCC: add waiting timeout and fix Tx done interface
@ 2022-09-20  9:44 Huisong Li
  2022-09-20  9:44 ` [PATCH 1/2] ACPI: PCC: replace wait_for_completion() Huisong Li
  2022-09-20  9:45 ` [PATCH 2/2] ACPI: PCC: fix Tx done interface in handler Huisong Li
  0 siblings, 2 replies; 8+ messages in thread
From: Huisong Li @ 2022-09-20  9:44 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: rafael, sudeep.holla, rafael.j.wysocki, wanghuiqiang, huangdaode,
	lihuisong

This patchset is used to fix following problems:
1) method execution may be blocked in abnormal case.
2) a error log when command is executed successfully.

Huisong Li (2):
  ACPI: PCC: replace wait_for_completion()
  ACPI: PCC: fix Tx done interface in handler

 drivers/acpi/acpi_pcc.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

-- 
2.33.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] ACPI: PCC: replace wait_for_completion()
  2022-09-20  9:44 [PATCH 0/2] ACPI: PCC: add waiting timeout and fix Tx done interface Huisong Li
@ 2022-09-20  9:44 ` Huisong Li
  2022-09-21 15:47   ` Sudeep Holla
  2022-09-20  9:45 ` [PATCH 2/2] ACPI: PCC: fix Tx done interface in handler Huisong Li
  1 sibling, 1 reply; 8+ messages in thread
From: Huisong Li @ 2022-09-20  9:44 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: rafael, sudeep.holla, rafael.j.wysocki, wanghuiqiang, huangdaode,
	lihuisong

Currently, the function waiting for completion of mailbox operation is
'wait_for_completion()'.  The PCC method will be permanently blocked if
this mailbox message fails to execute. So this patch replaces it with
'wait_for_completion_timeout()'. And set the timeout interval to an
arbitrary retries on top of nominal to prevent the remote processor is
slow to respond to PCC commands.

Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/acpi_pcc.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
index a12b55d81209..a1052fe998bf 100644
--- a/drivers/acpi/acpi_pcc.c
+++ b/drivers/acpi/acpi_pcc.c
@@ -23,6 +23,12 @@
 
 #include <acpi/pcc.h>
 
+/*
+ * Arbitrary retries in case the remote processor is slow to respond
+ * to PCC commands
+ */
+#define PCC_CMD_WAIT_RETRIES_NUM	500
+
 struct pcc_data {
 	struct pcc_mbox_chan *pcc_chan;
 	void __iomem *pcc_comm_addr;
@@ -86,6 +92,7 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
 {
 	int ret;
 	struct pcc_data *data = region_context;
+	u64 usecs_lat;
 
 	reinit_completion(&data->done);
 
@@ -96,8 +103,20 @@ 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)
-		wait_for_completion(&data->done);
+	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;
+		}
+	}
 
 	mbox_client_txdone(data->pcc_chan->mchan, ret);
 
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] ACPI: PCC: fix Tx done interface in handler
  2022-09-20  9:44 [PATCH 0/2] ACPI: PCC: add waiting timeout and fix Tx done interface Huisong Li
  2022-09-20  9:44 ` [PATCH 1/2] ACPI: PCC: replace wait_for_completion() Huisong Li
@ 2022-09-20  9:45 ` Huisong Li
  2022-09-21 15:43   ` Sudeep Holla
  1 sibling, 1 reply; 8+ messages in thread
From: Huisong Li @ 2022-09-20  9:45 UTC (permalink / raw)
  To: linux-acpi, linux-kernel
  Cc: rafael, sudeep.holla, rafael.j.wysocki, wanghuiqiang, huangdaode,
	lihuisong

A error, "Client can't run the TX ticker", is printed even if PCC command
executed successfully. This root cause is that PCC handler calls
'mbox_client_txdone()' which depands on the client can received 'ACK'
packet. But PCC handler detects whether the command is complete through
the Tx ACK interrupt. So this patch fix it.

Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/acpi/acpi_pcc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
index a1052fe998bf..95d2dc274bd9 100644
--- a/drivers/acpi/acpi_pcc.c
+++ b/drivers/acpi/acpi_pcc.c
@@ -118,7 +118,7 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
 		}
 	}
 
-	mbox_client_txdone(data->pcc_chan->mchan, ret);
+	mbox_chan_txdone(data->pcc_chan->mchan, ret);
 
 	memcpy_fromio(value, data->pcc_comm_addr, data->ctx.length);
 
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] ACPI: PCC: fix Tx done interface in handler
  2022-09-20  9:45 ` [PATCH 2/2] ACPI: PCC: fix Tx done interface in handler Huisong Li
@ 2022-09-21 15:43   ` Sudeep Holla
  2022-09-22  2:30     ` lihuisong (C)
  2022-09-22 19:10     ` Rafael J. Wysocki
  0 siblings, 2 replies; 8+ messages in thread
From: Sudeep Holla @ 2022-09-21 15:43 UTC (permalink / raw)
  To: Huisong Li
  Cc: linux-acpi, linux-kernel, rafael, rafael.j.wysocki, wanghuiqiang,
	huangdaode

On Tue, Sep 20, 2022 at 05:45:00PM +0800, Huisong Li wrote:
> A error, "Client can't run the TX ticker", is printed even if PCC command
> executed successfully. This root cause is that PCC handler calls
> 'mbox_client_txdone()' which depands on the client can received 'ACK'
> packet. But PCC handler detects whether the command is complete through
> the Tx ACK interrupt. So this patch fix it.
>

Thanks for fixing this. Someone mentioned about the error and it was in
my TODO list.

I would prefer to reword the subject and commit message as below:
"
ACPI: PCC: Fix Tx acknowledge in the PCC address space handler

Currently, mbox_client_txdone() is called from the PCC address space
handler and that expects the user the Tx state machine to be controlled
by the client which is not the case and the below warning is thrown:

  | PCCT: Client can't run the TX ticker

Let the controller run the state machine and the end of Tx can be
acknowledge by calling mbox_chan_txdone() instead.
"

With that:

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] ACPI: PCC: replace wait_for_completion()
  2022-09-20  9:44 ` [PATCH 1/2] ACPI: PCC: replace wait_for_completion() Huisong Li
@ 2022-09-21 15:47   ` Sudeep Holla
  2022-09-22  2:29     ` lihuisong (C)
  0 siblings, 1 reply; 8+ messages in thread
From: Sudeep Holla @ 2022-09-21 15:47 UTC (permalink / raw)
  To: Huisong Li
  Cc: linux-acpi, linux-kernel, rafael, rafael.j.wysocki, wanghuiqiang,
	huangdaode

On Tue, Sep 20, 2022 at 05:44:59PM +0800, Huisong Li wrote:
> Currently, the function waiting for completion of mailbox operation is
> 'wait_for_completion()'.  The PCC method will be permanently blocked if
> this mailbox message fails to execute. So this patch replaces it with
> 'wait_for_completion_timeout()'. And set the timeout interval to an
> arbitrary retries on top of nominal to prevent the remote processor is
> slow to respond to PCC commands.
>

Sounds good to me. The only concern(may be not serious) is what happens
if we receive response from the platform after the timeout. I have tested
for that in non ACPI non PCC context. I don't have a setup to trigger that
with ACPI PCC + this patch to test. Other than that, I am fine with this:

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] ACPI: PCC: replace wait_for_completion()
  2022-09-21 15:47   ` Sudeep Holla
@ 2022-09-22  2:29     ` lihuisong (C)
  0 siblings, 0 replies; 8+ messages in thread
From: lihuisong (C) @ 2022-09-22  2:29 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-acpi, linux-kernel, rafael, rafael.j.wysocki, wanghuiqiang,
	huangdaode


在 2022/9/21 23:47, Sudeep Holla 写道:
> On Tue, Sep 20, 2022 at 05:44:59PM +0800, Huisong Li wrote:
>> Currently, the function waiting for completion of mailbox operation is
>> 'wait_for_completion()'.  The PCC method will be permanently blocked if
>> this mailbox message fails to execute. So this patch replaces it with
>> 'wait_for_completion_timeout()'. And set the timeout interval to an
>> arbitrary retries on top of nominal to prevent the remote processor is
>> slow to respond to PCC commands.
>>
> Sounds good to me. The only concern(may be not serious) is what happens
> if we receive response from the platform after the timeout. I have tested
If OS still cann't receive response in noramal latency(must be filled 
accurately
as protocol said) + retries, there is a high probability that an 
exception occurs.
Even if we receive response after the timeout, I think there may be no 
impact,
but the response data in PCC share memory is ignored.
> for that in non ACPI non PCC context. I don't have a setup to trigger that
> with ACPI PCC + this patch to test. Other than that, I am fine with this:
>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] ACPI: PCC: fix Tx done interface in handler
  2022-09-21 15:43   ` Sudeep Holla
@ 2022-09-22  2:30     ` lihuisong (C)
  2022-09-22 19:10     ` Rafael J. Wysocki
  1 sibling, 0 replies; 8+ messages in thread
From: lihuisong (C) @ 2022-09-22  2:30 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-acpi, linux-kernel, rafael, rafael.j.wysocki, wanghuiqiang,
	huangdaode


在 2022/9/21 23:43, Sudeep Holla 写道:
> On Tue, Sep 20, 2022 at 05:45:00PM +0800, Huisong Li wrote:
>> A error, "Client can't run the TX ticker", is printed even if PCC command
>> executed successfully. This root cause is that PCC handler calls
>> 'mbox_client_txdone()' which depands on the client can received 'ACK'
>> packet. But PCC handler detects whether the command is complete through
>> the Tx ACK interrupt. So this patch fix it.
>>
> Thanks for fixing this. Someone mentioned about the error and it was in
> my TODO list.
Great minds think alike😁
>
> I would prefer to reword the subject and commit message as below:
> "
> ACPI: PCC: Fix Tx acknowledge in the PCC address space handler
>
> Currently, mbox_client_txdone() is called from the PCC address space
> handler and that expects the user the Tx state machine to be controlled
> by the client which is not the case and the below warning is thrown:
>
>    | PCCT: Client can't run the TX ticker
>
> Let the controller run the state machine and the end of Tx can be
> acknowledge by calling mbox_chan_txdone() instead.
> "
Thank you for your suggestion. I will fix it in V2.
> With that:
>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] ACPI: PCC: fix Tx done interface in handler
  2022-09-21 15:43   ` Sudeep Holla
  2022-09-22  2:30     ` lihuisong (C)
@ 2022-09-22 19:10     ` Rafael J. Wysocki
  1 sibling, 0 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2022-09-22 19:10 UTC (permalink / raw)
  To: Sudeep Holla, Huisong Li
  Cc: ACPI Devel Maling List, Linux Kernel Mailing List,
	Rafael J. Wysocki, Rafael Wysocki, wanghuiqiang, huangdaode

On Wed, Sep 21, 2022 at 5:43 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Sep 20, 2022 at 05:45:00PM +0800, Huisong Li wrote:
> > A error, "Client can't run the TX ticker", is printed even if PCC command
> > executed successfully. This root cause is that PCC handler calls
> > 'mbox_client_txdone()' which depands on the client can received 'ACK'
> > packet. But PCC handler detects whether the command is complete through
> > the Tx ACK interrupt. So this patch fix it.
> >
>
> Thanks for fixing this. Someone mentioned about the error and it was in
> my TODO list.
>
> I would prefer to reword the subject and commit message as below:
> "
> ACPI: PCC: Fix Tx acknowledge in the PCC address space handler
>
> Currently, mbox_client_txdone() is called from the PCC address space
> handler and that expects the user the Tx state machine to be controlled
> by the client which is not the case and the below warning is thrown:
>
>   | PCCT: Client can't run the TX ticker
>
> Let the controller run the state machine and the end of Tx can be
> acknowledge by calling mbox_chan_txdone() instead.
> "
>
> With that:
>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Applied as 6.1 material along with the [1/2].

I used the above text in quotes as the subject and changelog instead
of the original pieces.

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-09-22 19:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-20  9:44 [PATCH 0/2] ACPI: PCC: add waiting timeout and fix Tx done interface Huisong Li
2022-09-20  9:44 ` [PATCH 1/2] ACPI: PCC: replace wait_for_completion() Huisong Li
2022-09-21 15:47   ` Sudeep Holla
2022-09-22  2:29     ` lihuisong (C)
2022-09-20  9:45 ` [PATCH 2/2] ACPI: PCC: fix Tx done interface in handler Huisong Li
2022-09-21 15:43   ` Sudeep Holla
2022-09-22  2:30     ` lihuisong (C)
2022-09-22 19:10     ` Rafael J. Wysocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.