[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d9969244-8f4c-4f66-9ab1-064be665495d@amperemail.onmicrosoft.com>
Date: Sat, 2 Nov 2024 11:34:59 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: "lihuisong (C)" <lihuisong@...wei.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
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.
Powered by blists - more mailing lists