[v6,1/2] mctp pcc: Check before sending MCTP PCC response ACK

Commit Message

[e-mail] Oct. 29, 2024, 4:54 p.m. UTC

From: Adam Young <[e-mail]>

Type 4 PCC channels have an option to send back a response
to the platform when they are done processing the request.
The flag to indicate whether or not to respond is inside
the message body, and thus is not available to the pcc
mailbox.

In order to read the flag, this patch maps the shared
buffer to virtual memory.

If the flag is not set, still set command completion
bit after processing message.

Signed-off-by: Adam Young <[e-mail]>
---
 drivers/mailbox/pcc.c | 35 +++++++++++++++++++++++++++--------
 include/acpi/pcc.h    |  8 ++++++++
 2 files changed, 35 insertions(+), 8 deletions(-)

Comments

lihuisong (C) Oct. 30, 2024, 9:45 a.m. UTC | #1

Hi Adam,

在 2024/10/30 0:54, [e-mail] 写道:
> From: Adam Young <[e-mail]>
>
> Type 4 PCC channels have an option to send back a response
> to the platform when they are done processing the request.
> The flag to indicate whether or not to respond is inside
> the message body, and thus is not available to the pcc
> mailbox.
>
> In order to read the flag, this patch maps the shared
> buffer to virtual memory.
>
> If the flag is not set, still set command completion
> bit after processing message.
>
> Signed-off-by: Adam Young <[e-mail]>
> ---
>   drivers/mailbox/pcc.c | 35 +++++++++++++++++++++++++++--------
>   include/acpi/pcc.h    |  8 ++++++++
>   2 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 94885e411085..b2a66e8a6cd6 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -90,6 +90,7 @@ struct pcc_chan_reg {
>    * @cmd_complete: PCC register bundle for the command complete check register
>    * @cmd_update: PCC register bundle for the command complete update register
>    * @error: PCC register bundle for the error status register
> + * @shmem_base_addr: the virtual memory address of the shared buffer
>    * @plat_irq: platform interrupt
>    * @type: PCC subspace type
>    * @plat_irq_flags: platform interrupt flags
> @@ -107,6 +108,7 @@ struct pcc_chan_info {
>   	struct pcc_chan_reg cmd_complete;
>   	struct pcc_chan_reg cmd_update;
>   	struct pcc_chan_reg error;
> +	void __iomem *shmem_base_addr;
>   	int plat_irq;
>   	u8 type;
>   	unsigned int plat_irq_flags;
> @@ -269,6 +271,27 @@ static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
>   	return !!val;
>   }
>   
> +static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
> +{
> +	struct pcc_extended_type_hdr pcc_hdr;
> +
> +	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> +		return;
> +	memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr,
> +		      sizeof(struct pcc_extended_type_hdr));
> +	/*
> +	 * The PCC slave subspace channel needs to set the command complete bit
> +	 * after processing message. If the PCC_ACK_FLAG is set, it should also
> +	 * ring the doorbell.
> +	 *
> +	 * The PCC master subspace channel clears chan_in_use to free channel.
> +	 */
> +	if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
> +		pcc_send_data(chan, NULL);
> +	else
> +		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> +}
> +
>   /**
>    * pcc_mbox_irq - PCC mailbox interrupt handler
>    * @irq:	interrupt number
> @@ -306,14 +329,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   
>   	mbox_chan_received_data(chan, NULL);
>   
> -	/*
> -	 * The PCC slave subspace channel needs to set the command complete bit
> -	 * and ring doorbell after processing message.
> -	 *
> -	 * The PCC master subspace channel clears chan_in_use to free channel.
> -	 */
> -	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> -		pcc_send_data(chan, NULL);
> +	check_and_ack(pchan, chan);
>   	pchan->chan_in_use = false;
>   
>   	return IRQ_HANDLED;
> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	if (rc)
>   		return ERR_PTR(rc);
>   
> +	pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
> +					      pchan->chan.shmem_base_addr,
> +					      pchan->chan.shmem_size);
Currently, the PCC mbox client does ioremap after requesting PCC channel.
Thus all current clients will ioremap twice. This is not good to me.
How about add a new interface and give the type4 client the right to 
decide whether to reply in rx_callback?
>   	return &pchan->chan;
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 9b373d172a77..0bcb86dc4de7 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -18,6 +18,13 @@ struct pcc_mbox_chan {
>   	u16 min_turnaround_time;
>   };
>   
> +struct pcc_extended_type_hdr {
> +	__le32 signature;
> +	__le32 flags;
> +	__le32 length;
> +	char command[4];
> +};

No need to define this new structure and directly use "struct 
acpi_pcct_ext_pcc_shared_memory".

> +
>   /* Generic Communications Channel Shared Memory Region */
>   #define PCC_SIGNATURE			0x50434300
>   /* Generic Communications Channel Command Field */
> @@ -31,6 +38,7 @@ struct pcc_mbox_chan {
>   #define PCC_CMD_COMPLETION_NOTIFY	BIT(0)
>   
>   #define MAX_PCC_SUBSPACES	256
> +#define PCC_ACK_FLAG_MASK	0x1
directly use the macro PCC_CMD_COMPLETION_NOTIF
>   
>   #ifdef CONFIG_PCC
>   extern struct pcc_mbox_chan *

Adam Young Nov. 1, 2024, 12:16 a.m. UTC | #2

On 10/30/24 05:45, lihuisong (C) wrote:
>> + check_and_ack(pchan, chan);
>>       pchan->chan_in_use = false;
>>         return IRQ_HANDLED;
>> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client *cl, 
>> int subspace_id)
>>       if (rc)
>>           return ERR_PTR(rc);
>>   +    pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
>> +                          pchan->chan.shmem_base_addr,
>> +                          pchan->chan.shmem_size);
> Currently, the PCC mbox client does ioremap after requesting PCC channel.
> Thus all current clients will ioremap twice. This is not good to me.
> How about add a new interface and give the type4 client the right to 
> decide whether to reply in rx_callback?


I do agree that is a cleaner implementation, but I don't have a way of 
testing the other drivers, and did not want to break them. I think your 
driver is the only that makes use of it, so we can certainly come up 
with a common approach.

The mailbox interface does not allow a return code from 
mbox_chan_received_data, which is what I originally wanted.  If that 
could return multiple status codes, one of them could indicate the need  
to send the interrupt back.  Otherwise, we need to query the driver to 
read the shared memory again.

lihuisong (C) Nov. 1, 2024, 1:30 a.m. UTC | #3

Hi Adam,

All modifications in the patch is done for pcc instead of mctp.
Suggest that use the prefix "mailbox: pcc: xxxx".
Please find my following reply.


在 2024/11/1 8:16, Adam Young 写道:
>
> On 10/30/24 05:45, lihuisong (C) wrote:
>>> + check_and_ack(pchan, chan);
>>>       pchan->chan_in_use = false;
>>>         return IRQ_HANDLED;
>>> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client *cl, 
>>> int subspace_id)
>>>       if (rc)
>>>           return ERR_PTR(rc);
>>>   +    pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
>>> +                          pchan->chan.shmem_base_addr,
>>> +                          pchan->chan.shmem_size);
>> Currently, the PCC mbox client does ioremap after requesting PCC 
>> channel.
>> Thus all current clients will ioremap twice. This is not good to me.
>> How about add a new interface and give the type4 client the right to 
>> decide whether to reply in rx_callback?
>
>
> I do agree that is a cleaner implementation, but I don't have a way of 
> testing the other drivers, and did not want to break them. I think 
> your driver is the only that makes use of it, so we can certainly come 
> up with a common approach.
I understand what you are concerned about.
But this duplicate ioremap also works for all PCC clients no matter 
which type they used. It has very wide influence.
My driver just uses type3 instead of type4. What's more, AFAICS, it 
doesn't seem there is type4 client driver in linux.
Therefore, determining whether type4 client driver needs to reply to 
platform has the minimum or even no impact in their rx_callback.
>
> The mailbox interface does not allow a return code from 
> mbox_chan_received_data, which is what I originally wanted.  If that 
> could return multiple status codes, one of them could indicate the 
> need  to send the interrupt back.  Otherwise, we need to query the 
> driver to read the shared memory again.
yes
>
> .

Adam Young Nov. 2, 2024, 3:34 p.m. UTC | #4

On 10/31/24 21:30, lihuisong (C) wrote:
>>
>> On 10/30/24 05:45, lihuisong (C) wrote:
>>>> + check_and_ack(pchan, chan);
>>>>       pchan->chan_in_use = false;
>>>>         return IRQ_HANDLED;
>>>> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client 
>>>> *cl, int subspace_id)
>>>>       if (rc)
>>>>           return ERR_PTR(rc);
>>>>   +    pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
>>>> +                          pchan->chan.shmem_base_addr,
>>>> +                          pchan->chan.shmem_size);
>>> Currently, the PCC mbox client does ioremap after requesting PCC 
>>> channel.
>>> Thus all current clients will ioremap twice. This is not good to me.
>>> How about add a new interface and give the type4 client the right to 
>>> decide whether to reply in rx_callback?
>>
>>
>> I do agree that is a cleaner implementation, but I don't have a way 
>> of testing the other drivers, and did not want to break them. I think 
>> your driver is the only that makes use of it, so we can certainly 
>> come up with a common approach.
> I understand what you are concerned about.
> But this duplicate ioremap also works for all PCC clients no matter 
> which type they used. It has very wide influence.
> My driver just uses type3 instead of type4. What's more, AFAICS, it 
> doesn't seem there is type4 client driver in linux.
> Therefore, determining whether type4 client driver needs to reply to 
> platform has the minimum or even no impact in their rx_callback. 

  I can move the place where we hold on to the shmem from struct 
pcc_chan_info in pcc.c, where it is local to the file, to struct 
pcc_mbox_chan in  include/acpi/pcc.h where it will be visible from both 
files.  With that change, we only need ioremap once for the segment.

I don't like adding the callback decision in the driver:  it is part of 
the protocol, and should be enforced  by the pcc layer. If we do it in 
the driver, the logic will be duplicated by each driver.

I could make a further  change and allow the driver to request the 
remapped memory segment from the pcc layer, and couple  to the 
memory-remap to the client/channel.  It seems like that code, too, 
should be in the common layer.  However most drivers would not know to 
use  this function yet, so the mechanism would have to be optional, and 
only clean up if called this way.

lihuisong (C) Nov. 4, 2024, 3:17 a.m. UTC | #5

在 2024/11/2 23:34, Adam Young 写道:
>
> On 10/31/24 21:30, lihuisong (C) wrote:
>>>
>>> On 10/30/24 05:45, lihuisong (C) wrote:
>>>>> + check_and_ack(pchan, chan);
>>>>>       pchan->chan_in_use = false;
>>>>>         return IRQ_HANDLED;
>>>>> @@ -352,6 +368,9 @@ pcc_mbox_request_channel(struct mbox_client 
>>>>> *cl, int subspace_id)
>>>>>       if (rc)
>>>>>           return ERR_PTR(rc);
>>>>>   +    pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
>>>>> +                          pchan->chan.shmem_base_addr,
>>>>> +                          pchan->chan.shmem_size);
>>>> Currently, the PCC mbox client does ioremap after requesting PCC 
>>>> channel.
>>>> Thus all current clients will ioremap twice. This is not good to me.
>>>> How about add a new interface and give the type4 client the right 
>>>> to decide whether to reply in rx_callback?
>>>
>>>
>>> I do agree that is a cleaner implementation, but I don't have a way 
>>> of testing the other drivers, and did not want to break them. I 
>>> think your driver is the only that makes use of it, so we can 
>>> certainly come up with a common approach.
>> I understand what you are concerned about.
>> But this duplicate ioremap also works for all PCC clients no matter 
>> which type they used. It has very wide influence.
>> My driver just uses type3 instead of type4. What's more, AFAICS, it 
>> doesn't seem there is type4 client driver in linux.
>> Therefore, determining whether type4 client driver needs to reply to 
>> platform has the minimum or even no impact in their rx_callback. 
>
>  I can move the place where we hold on to the shmem from struct 
> pcc_chan_info in pcc.c, where it is local to the file, to struct 
> pcc_mbox_chan in  include/acpi/pcc.h where it will be visible from 
> both files.  With that change, we only need ioremap once for the segment.
>
> I don't like adding the callback decision in the driver:  it is part 
> of the protocol, and should be enforced  by the pcc layer. If we do it 
> in the driver, the logic will be duplicated by each driver.
Yes
>
> I could make a further  change and allow the driver to request the 
> remapped memory segment from the pcc layer, and couple  to the 
> memory-remap to the client/channel.  It seems like that code, too, 
> should be in the common layer.  However most drivers would not know to 
> use  this function yet, so the mechanism would have to be optional, 
> and only clean up if called this way.
I agree this method.
Don't remap twice for one shared memory.
This remaping is reasonable in PCC layer. We can let PCC client to 
decide if PCC layer does remap and then they use it directly.
For new driver like the driver you are uploading, driver can give PCC 
one flag to tell PCC layer remap when request channel.
For old PCC client driver, do not send this flag, PPC layer do not 
remap. So no any impact on them.
>
>
>
>
>
>
> .

Adam Young Nov. 4, 2024, 6:44 p.m. UTC | #6

On 11/3/24 22:17, lihuisong (C) wrote:
>>
>> I could make a further  change and allow the driver to request the 
>> remapped memory segment from the pcc layer, and couple  to the 
>> memory-remap to the client/channel.  It seems like that code, too, 
>> should be in the common layer.  However most drivers would not know 
>> to use  this function yet, so the mechanism would have to be 
>> optional, and only clean up if called this way.
> I agree this method.
> Don't remap twice for one shared memory.
> This remaping is reasonable in PCC layer. We can let PCC client to 
> decide if PCC layer does remap and then they use it directly.
> For new driver like the driver you are uploading, driver can give PCC 
> one flag to tell PCC layer remap when request channel.
> For old PCC client driver, do not send this flag, PPC layer do not 
> remap. So no any impact on them. 


I think we are actually in agreement here.  No double mapping, but the 
driver MAY request the mapping happen in the PCC layer. No impact on 
existing drivers.

Patch

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 94885e411085..b2a66e8a6cd6 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -90,6 +90,7 @@  struct pcc_chan_reg {
  * @cmd_complete: PCC register bundle for the command complete check register
  * @cmd_update: PCC register bundle for the command complete update register
  * @error: PCC register bundle for the error status register
+ * @shmem_base_addr: the virtual memory address of the shared buffer
  * @plat_irq: platform interrupt
  * @type: PCC subspace type
  * @plat_irq_flags: platform interrupt flags
@@ -107,6 +108,7 @@  struct pcc_chan_info {
 	struct pcc_chan_reg cmd_complete;
 	struct pcc_chan_reg cmd_update;
 	struct pcc_chan_reg error;
+	void __iomem *shmem_base_addr;
 	int plat_irq;
 	u8 type;
 	unsigned int plat_irq_flags;
@@ -269,6 +271,27 @@  static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
 	return !!val;
 }
 
+static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
+{
+	struct pcc_extended_type_hdr pcc_hdr;
+
+	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+		return;
+	memcpy_fromio(&pcc_hdr, pchan->shmem_base_addr,
+		      sizeof(struct pcc_extended_type_hdr));
+	/*
+	 * The PCC slave subspace channel needs to set the command complete bit
+	 * after processing message. If the PCC_ACK_FLAG is set, it should also
+	 * ring the doorbell.
+	 *
+	 * The PCC master subspace channel clears chan_in_use to free channel.
+	 */
+	if (le32_to_cpup(&pcc_hdr.flags) & PCC_ACK_FLAG_MASK)
+		pcc_send_data(chan, NULL);
+	else
+		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
+}
+
 /**
  * pcc_mbox_irq - PCC mailbox interrupt handler
  * @irq:	interrupt number
@@ -306,14 +329,7 @@  static irqreturn_t pcc_mbox_irq(int irq, void *p)
 
 	mbox_chan_received_data(chan, NULL);
 
-	/*
-	 * The PCC slave subspace channel needs to set the command complete bit
-	 * and ring doorbell after processing message.
-	 *
-	 * The PCC master subspace channel clears chan_in_use to free channel.
-	 */
-	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
-		pcc_send_data(chan, NULL);
+	check_and_ack(pchan, chan);
 	pchan->chan_in_use = false;
 
 	return IRQ_HANDLED;
@@ -352,6 +368,9 @@  pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 	if (rc)
 		return ERR_PTR(rc);
 
+	pchan->shmem_base_addr = devm_ioremap(chan->mbox->dev,
+					      pchan->chan.shmem_base_addr,
+					      pchan->chan.shmem_size);
 	return &pchan->chan;
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 9b373d172a77..0bcb86dc4de7 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -18,6 +18,13 @@  struct pcc_mbox_chan {
 	u16 min_turnaround_time;
 };
 
+struct pcc_extended_type_hdr {
+	__le32 signature;
+	__le32 flags;
+	__le32 length;
+	char command[4];
+};
+
 /* Generic Communications Channel Shared Memory Region */
 #define PCC_SIGNATURE			0x50434300
 /* Generic Communications Channel Command Field */
@@ -31,6 +38,7 @@  struct pcc_mbox_chan {
 #define PCC_CMD_COMPLETION_NOTIFY	BIT(0)
 
 #define MAX_PCC_SUBSPACES	256
+#define PCC_ACK_FLAG_MASK	0x1
 
 #ifdef CONFIG_PCC
 extern struct pcc_mbox_chan *