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

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

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.