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

From: Adam Young
Date: Sat Nov 02 2024 - 11:35:17 EST



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.