Message-ID: <6eba7c8b-000f-7be0-4a8a-53bf8d6dc25f@huawei.com>
Date: Mon, 4 Nov 2024 11:17:44 +0800
From: "lihuisong (C)" <lihuisong@...wei.com>
To: Adam Young <admiyo@...eremail.onmicrosoft.com>,
	<admiyo@...amperecomputing.com>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Jeremy Kerr
	<jk@...econstruct.com.au>, Matt Johnston <matt@...econstruct.com.au>, "David
 S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, "Len
 Brown" <lenb@...nel.org>, "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Robert Moore <robert.moore@...el.com>, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>, Jonathan Cameron
	<Jonathan.Cameron@...wei.com>, Jassi Brar <jassisinghbrar@...il.com>, "Sudeep
 Holla" <sudeep.holla@....com>
Subject: Re: [PATCH v6 1/2] mctp pcc: Check before sending MCTP PCC response
 ACK


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

Powered by blists - more mailing lists