Message-ID: <041ff396-c5b9-40e2-8028-2b3325a3f0e3@gmail.com>
Date: Tue, 22 Oct 2024 10:46:43 -0500
From: Denis Kenzior <denkenz@...il.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, linux-arm-msm@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1 00/10] QRTR Multi-endpoint support

Hi Mani,

On 10/22/24 10:39 AM, Manivannan Sadhasivam wrote:
> On Fri, Oct 18, 2024 at 01:18:18PM -0500, Denis Kenzior wrote:
>> The current implementation of QRTR assumes that each entity on the QRTR
>> IPC bus is uniquely identifiable by its node/port combination, with
>> node/port combinations being used to route messages between entities.
>>
>> However, this assumption of uniqueness is problematic in scenarios
>> where multiple devices with the same node/port combinations are
>> connected to the system.  A practical example is a typical consumer PC
>> with multiple PCIe-based devices, such as WiFi cards or 5G modems, where
>> each device could potentially have the same node identifier set.  In
>> such cases, the current QRTR protocol implementation does not provide a
>> mechanism to differentiate between these devices, making it impossible
>> to support communication with multiple identical devices.
>>
>> This patch series addresses this limitation by introducing support for
>> a concept of an 'endpoint.' Multiple devices with conflicting node/port
>> combinations can be supported by assigning a unique endpoint identifier
>> to each one.  Such endpoint identifiers can then be used to distinguish
>> between devices while sending and receiving messages over QRTR sockets.
>>
> 
> Thanks for your work on this! I'm yet to look into the details but wondering how
> all the patches have Reviewed-by tags provided that this series is 'RFC v1'.
> Also, it is quite surprising to see the review tag from Andy Gross who quit Qcom
> quite a while ago and I haven't seen him reviewing any Qcom patches for so long.
> 

I have very limited experience in kernel development, so the first few 
iterations were shared privately with a few folks to make sure I wasn't 
completely off base.  Andy was one of them :)

Regards,
-Denis

> - Mani
> 
>> The patch series maintains backward compatibility with existing clients:
>> the endpoint concept is added using auxiliary data that can be added to
>> recvmsg and sendmsg system calls.  The QRTR socket interface is extended
>> as follows:
>>
>> - Adds QRTR_ENDPOINT auxiliary data element that reports which endpoint
>>    generated a particular message.  This auxiliary data is only reported
>>    if the socket was explicitly opted in using setsockopt, enabling the
>>    QRTR_REPORT_ENDPOINT socket option.  SOL_QRTR socket level was added
>>    to facilitate this.  This requires QRTR clients to be updated to use
>>    recvmsg instead of the more typical recvfrom() or recv() use.
>>
>> - Similarly, QRTR_ENDPOINT auxiliary data element can be included in
>>    sendmsg() requests.  This will allow clients to route QRTR messages
>>    to the desired endpoint, even in cases of node/port conflict between
>>    multiple endpoints.
>>
>> - Finally, QRTR_BIND_ENDPOINT socket option is introduced.  This allows
>>    clients to bind to a particular endpoint (such as a 5G PCIe modem) if
>>    they're only interested in receiving or sending messages to this
>>    device.
>>
>> NOTE: There is 32-bit unsafe use of radix_tree_insert in this patch set.
>> This follows the existing usage inside net/qrtr/af_qrtr.c in
>> qrtr_tx_wait(), qrtr_tx_resume() and qrtr_tx_flow_failed().  This was
>> done deliberately in order to keep the changes as minimal as possible
>> until it is known whether the approach outlined is generally acceptable.
>>
>> Denis Kenzior (10):
>>    net: qrtr: ns: validate msglen before ctrl_pkt use
>>    net: qrtr: allocate and track endpoint ids
>>    net: qrtr: support identical node ids
>>    net: qrtr: Report sender endpoint in aux data
>>    net: qrtr: Report endpoint for locally generated messages
>>    net: qrtr: Allow sendmsg to target an endpoint
>>    net: qrtr: allow socket endpoint binding
>>    net: qrtr: Drop remote {NEW|DEL}_LOOKUP messages
>>    net: qrtr: ns: support multiple endpoints
>>    net: qrtr: mhi: Report endpoint id in sysfs
>>
>>   include/linux/socket.h    |   1 +
>>   include/uapi/linux/qrtr.h |   7 +
>>   net/qrtr/af_qrtr.c        | 297 +++++++++++++++++++++++++++++++------
>>   net/qrtr/mhi.c            |  14 ++
>>   net/qrtr/ns.c             | 299 +++++++++++++++++++++++---------------
>>   net/qrtr/qrtr.h           |   4 +
>>   6 files changed, 459 insertions(+), 163 deletions(-)
>>
>> -- 
>> 2.45.2
>>
> 

Powered by blists - more mailing lists