| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7ba5acb0-be7d-492f-9f86-bb76c927437c@quicinc.com>
Date: Thu, 6 Feb 2025 15:14:20 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Frank Li <Frank.li@....com>
CC: Joshua Yeong <joshua.yeong@...rfivetech.com>,
Alexandre Belloni
<alexandre.belloni@...tlin.com>,
Miquel Raynal <miquel.raynal@...tlin.com>,
Conor Culhane <conor.culhane@...vaco.com>,
"linux-i3c@...ts.infradead.org"
<linux-i3c@...ts.infradead.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: Re: [PATCH 1/2] i3c: Add basic HDR support
Hi Frank,
On 2/5/2025 9:14 PM, Frank Li wrote:
> On Wed, Feb 05, 2025 at 07:49:20PM +0530, Mukesh Kumar Savaliya wrote:
>> Thanks Frank ! please review below.
>>
>> On 2/4/2025 9:13 PM, Frank Li wrote:
>>> On Tue, Feb 04, 2025 at 11:24:03AM +0530, Mukesh Kumar Savaliya wrote:
>>>>
>>>>
>>>> On 2/3/2025 9:22 PM, Frank Li wrote:
>>>>> On Mon, Feb 03, 2025 at 02:32:51AM +0000, Joshua Yeong wrote:
>>>>>> Hi Mukesh and Frank,
>>>>>>
>>>>>> On 02-Feb-2025 1:54 AM, Mukesh Kumar Savaliya wrote:
>>>>>>> Hi Frank,
>>>>>>>
>>>>>>> On 1/30/2025 1:35 AM, Frank Li wrote:
>>>>>>>> I3C HDR requires enter/exit patterns during each I3C transfer. Add a new
>>>>>>>> API i3c_device_do_priv_xfers_mode(). The existing
>>>>>>>> i3c_device_do_priv_xfers() now calls i3c_device_do_priv_xfers_mode(I3C_SDR)
>>>>>>>> to maintain backward compatibility.
>>>>>>>>
>>>>>>>> Introduce a 'cmd' field in 'struct i3c_priv_xfer', using an anonymous union
>>>>>>>> with 'rnw' since HDR mode relies on read/write commands instead of the
>>>>>>>> 'rnw' bit in the address as in SDR mode.
>>>>>>>>
>>>>>>>> Add a priv_xfers_mode callback for I3C master drivers. If priv_xfers_mode
>>>>>>>> is not implemented, fallback to SDR mode using the existing priv_xfers
>>>>>>>> callback.
>>>>>>>>
>>>>>>>> Signed-off-by: Frank Li <Frank.Li@....com>
>>>>>>>> ---
>>>>>>>> Why not add hdr mode in struct i3c_priv_xfer because mode can't be mixed in
>>>>>>>> one i3c transfer. for example, can't send a HDR follow one SDR between
>>>>>>>> START and STOP.
>>>>>>>>
>>>>>>> Is it wise to add two function inside main funcion and separate SDR vs HDR ? so we don't need to change current function arguments.
>>>>>
>>>>> I am not sure if understand your means. HDR have difference mode, anyway
>>>>> need add new argument.
>>>>>
>>>> let me clarify, We can have main entry/hookup function as it is.
>>>> From priv_xfer , we can call sdr_priv_xfer and hdr_priv_xfer ? based on the
>>>> structure information if its enum SDR or enum HDR.
>>>>
>>>>>>
>>>>>> I think the 'private transfers' are limited to SDR communications,
>>>>>> would it be better if we have a different interface/naming for hdr transfers?
>>>>>
>>>>> The key is what's difference between hdr and private transfers. So far only
>>>>> command vs rnw, any other differences I missed?
>>>>>
>>>> yes, but can we make it priv_xfer() = sdr_priv_xfer() + hdr_priv_xfer() ?
>>>
>>> struct i3c_priv_xfer is array, each i3c_priv_xfer item is indepent. Can you
>>> write a simple code to demostrate your idea.
>>>
>>> Frank
>>
>> ======
>> Please see below. My only intention is to add hdr/sdr decision inside
>> i3c_priv_xfer and rest of the functions should remain as it is.
>>
>> If you still need, may be we can add local function but no need to change
>> prototype of function being explosed to many vendors.
>>
>> I hope i am not missing any major thing.
>>
>> struct i3c_priv_xfer {
>> u8 rnw;
>> u16 len;
>> union {
>> void *in;
>> const void *out;
>> } data;
>> enum i3c_error_code err;
>> // Just Add below two members and rest can be passed till end point/function
>> and can process as required.
>> enum i3c_hdr_mode mode;
>> union {
>> u8 rnw;
>> u8 cmd;
>> };
>> };
>>
>>
>> device.c :
>> i3c_device_do_priv_xfers => i3c_dev_do_priv_xfers_locked
>>
>> int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer
>> *xfers, int nxfers) {
>>
>> ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, xfers);
>>
>> }
>>
>>
>> master.c :
>> int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, struct
>> i3c_priv_xfer *xfers, int nxfers) {
>>
>> if (master->ops->priv_xfers_mode)
>> return master->ops->priv_xfers_mode(dev, xfers, nxfers);
>>
>> if (xfers->mode != I3C_SDR)
>> return -EINVAL;
>> }
>>
>
> The problem is
>
> struct i3c_priv_xfer xfer[3] = {
> { .mode = SDR ...},
> { .mode = HDR ...},
> { .mode = SDR ...},
> }
>
> i3c_device_do_priv_xfers(dev, xfer, 3);
>
> How to handle this case?
>
> I think i3c_device_do_priv_xfers() means one whole i3c transcation:
> START, xfer[0], xfer[1], xfer[2], STOP.
>
> Frank
>
I think they are separate transfers, it means one call but multiple
buffer to send with separate start/stop.
Two options :
1. When multiple transfers in a single call, driver internally itrerates
and should take care of start-xfer-stop. I think HW should be taking
care unless using the restart intentionally.
2. if any xfer[i] has HDR mode, internally it can call local function
OR should diverge based on mode wherever it suits better to do.
>>
>> svc-i3c-master.c
>> //process everything using xfers.mode and xfer.cmd.
>>
>> I hope you can add all rest of the changes inside this function itself.
>> static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, struct
>> i3c_priv_xfer *xfers, int nxfers)
>> ==========
>>>
>>>>>>
>>>>>>>> i3c_priv_xfer should be treat as whole i3c transactions. If user want send
>>>>>>>> HDR follow SDR, should be call i3c_device_do_priv_xfers_mode() twice,
>>>>>>>> instead put into a big i3c_priv_xfer[n].
>>>>>>>> ---
>>>>>>>> drivers/i3c/device.c | 19 +++++++++++++------
>>>>>>>> drivers/i3c/internals.h | 2 +-
>>>>>>>> drivers/i3c/master.c | 8 +++++++-
>>>>>>>> include/linux/i3c/device.h | 12 +++++++++++-
>>>>>>>> include/linux/i3c/master.h | 3 +++
>>>>>>>> 5 files changed, 35 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
>>>>>>>> index e80e487569146..e3db3a6a9e4f6 100644
>>>>>>>> --- a/drivers/i3c/device.c
>>>>>>>> +++ b/drivers/i3c/device.c
>>>>>>>> @@ -15,12 +15,13 @@
>>>>>>>> #include "internals.h"
>>>>>>>> /**
>>>>>>>> - * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a
>>>>>>>> - * specific device
>>>>>>>> + * i3c_device_do_priv_xfers_mode() - do I3C SDR private transfers directed to a
>>>>>>>> + * specific device
>>>>>>>> *
>>>>>>>> * @dev: device with which the transfers should be done
>>>>>>>> * @xfers: array of transfers
>>>>>>>> * @nxfers: number of transfers
>>>>>>>> + * @mode: transfer mode
>>>>>>>> *
>>>>>>>> * Initiate one or several private SDR transfers with @dev.
>>>>>>>> *
>>>>>>>> @@ -32,9 +33,9 @@
>>>>>>>> * driver needs to resend the 'xfers' some time later.
>>>>>>>> * See I3C spec ver 1.1.1 09-Jun-2021. Section: 5.1.2.2.3.
>>>>>>>> */
>>>>>>>> -int i3c_device_do_priv_xfers(struct i3c_device *dev,
>>>>>>>> - struct i3c_priv_xfer *xfers,
>>>>>>>> - int nxfers)
>>>>>>>> +int i3c_device_do_priv_xfers_mode(struct i3c_device *dev,
>>>>>>>> + struct i3c_priv_xfer *xfers,
>>>>>>>> + int nxfers, enum i3c_hdr_mode mode)
>>>>>>> why can't we pass mode as another structure member inside struct i3c_priv_xfer ? Adding function agrument parameter impacts existing drivers.
>>>>>
>>>>> If that, it will be possible, xfers[0] is sdr, xfers[1] is hdr. Actually,
>>>>> I3C can't do that. we assume finish whole xfers between one whole traction
>>>>> (between START and STOP).
>>>>>
>>>>> Frank
>>>>>>>> {
>>>>>>>> int ret, i;
>>>>>>>> @@ -47,11 +48,17 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
>>>>>>>> }
>>>>>>>> i3c_bus_normaluse_lock(dev->bus);
>>>>>>>> - ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
>>>>>>>> + ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers, mode);
>>>>>>>> i3c_bus_normaluse_unlock(dev->bus);
>>>>>>>> return ret;
>>>>>>>> }
>>>>>>>> +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers_mode);
>>>>>>>> +
>>>>>>>> +int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers, int nxfers)
>>>>>>>> +{
>>>>>>>> + return i3c_device_do_priv_xfers_mode(dev, xfers, nxfers, I3C_SDR);
>>>>>>>> +}
>>>>>>>> EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
>>>>>>>> /**
>>>>>>>> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
>>>>>>>> index 433f6088b7cec..553edc9846ac0 100644
>>>>>>>> --- a/drivers/i3c/internals.h
>>>>>>>> +++ b/drivers/i3c/internals.h
>>>>>>>> @@ -16,7 +16,7 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
>>>>>>>> int i3c_dev_setdasa_locked(struct i3c_dev_desc *dev);
>>>>>>>> int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>>>>>>>> struct i3c_priv_xfer *xfers,
>>>>>>>> - int nxfers);
>>>>>>>> + int nxfers, enum i3c_hdr_mode mode);
>>>>>>>> int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev);
>>>>>>>> int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
>>>>>>>> int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
>>>>>>>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>>>>>>>> index d5dc4180afbcf..67aaba0a38db2 100644
>>>>>>>> --- a/drivers/i3c/master.c
>>>>>>>> +++ b/drivers/i3c/master.c
>>>>>>>> @@ -2945,7 +2945,7 @@ int i3c_dev_setdasa_locked(struct i3c_dev_desc *dev)
>>>>>>>> int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>>>>>>>> struct i3c_priv_xfer *xfers,
>>>>>>>> - int nxfers)
>>>>>>>> + int nxfers, enum i3c_hdr_mode mode)
>>>>>>>> {
>>>>>>>> struct i3c_master_controller *master;
>>>>>>>> @@ -2956,9 +2956,15 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>>>>>>>> if (!master || !xfers)
>>>>>>>> return -EINVAL;
>>>>>>>> + if (master->ops->priv_xfers_mode)
>>>>>>>> + return master->ops->priv_xfers_mode(dev, xfers, nxfers, mode);
>>>>>>>> +
>>>>>>>> if (!master->ops->priv_xfers)
>>>>>>>> return -ENOTSUPP;
>>>>>>>> + if (mode != I3C_SDR)
>>>>>>>> + return -EINVAL;
>>>>>>>> +
>>>>>>>> return master->ops->priv_xfers(dev, xfers, nxfers);
>>>>>>>> }
>>>>>>>> diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
>>>>>>>> index b674f64d0822e..7ce70d0967e27 100644
>>>>>>>> --- a/include/linux/i3c/device.h
>>>>>>>> +++ b/include/linux/i3c/device.h
>>>>>>>> @@ -40,11 +40,13 @@ enum i3c_error_code {
>>>>>>>> /**
>>>>>>>> * enum i3c_hdr_mode - HDR mode ids
>>>>>>>> + * @I3C_SDR: SDR mode (NOT HDR mode)
>>>>>>>> * @I3C_HDR_DDR: DDR mode
>>>>>>>> * @I3C_HDR_TSP: TSP mode
>>>>>>>> * @I3C_HDR_TSL: TSL mode
>>>>>>>> */
>>>>>>>> enum i3c_hdr_mode {
>>>>>>>> + I3C_SDR,
>>>>>>>> I3C_HDR_DDR,
>>>>>>>> I3C_HDR_TSP,
>>>>>>>> I3C_HDR_TSL,
>>>>>>>> @@ -53,6 +55,7 @@ enum i3c_hdr_mode {
>>>>>>>> /**
>>>>>>>> * struct i3c_priv_xfer - I3C SDR private transfer
>>>>>>>> * @rnw: encodes the transfer direction. true for a read, false for a write
>>>>>>>> + * @cmd: Read/Write command in HDR mode, read: 0x80 - 0xff, write: 0x00 - 0x7f
>>>>>>>> * @len: transfer length in bytes of the transfer
>>>>>>>> * @actual_len: actual length in bytes are transferred by the controller
>>>>>>>> * @data: input/output buffer
>>>>>>>> @@ -61,7 +64,10 @@ enum i3c_hdr_mode {
>>>>>>>> * @err: I3C error code
>>>>>>>> */
>>>>>>>> struct i3c_priv_xfer {
>>>>>>>> - u8 rnw;
>>>>>>>> + union {
>>>>>>>> + u8 rnw;
>>>>>>>> + u8 cmd;
>>>>>>>> + };
>>>>>>>> u16 len;
>>>>>>>> u16 actual_len;
>>>>>>>> union {
>>>>>>>> @@ -301,6 +307,10 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
>>>>>>>> struct i3c_priv_xfer *xfers,
>>>>>>>> int nxfers);
>>>>>>>> +int i3c_device_do_priv_xfers_mode(struct i3c_device *dev,
>>>>>>>> + struct i3c_priv_xfer *xfers,
>>>>>>>> + int nxfers, enum i3c_hdr_mode mode);
>>>>>>>> +
>>>>>>>> int i3c_device_do_setdasa(struct i3c_device *dev);
>>>>>>>> void i3c_device_get_info(const struct i3c_device *dev, struct i3c_device_info *info);
>>>>>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>>>>>>>> index 12d532b012c5a..352bd41139569 100644
>>>>>>>> --- a/include/linux/i3c/master.h
>>>>>>>> +++ b/include/linux/i3c/master.h
>>>>>>>> @@ -472,6 +472,9 @@ struct i3c_master_controller_ops {
>>>>>>>> int (*priv_xfers)(struct i3c_dev_desc *dev,
>>>>>>>> struct i3c_priv_xfer *xfers,
>>>>>>>> int nxfers);
>>>>>>>> + int (*priv_xfers_mode)(struct i3c_dev_desc *dev,
>>>>>>>> + struct i3c_priv_xfer *xfers,
>>>>>>>> + int nxfers, enum i3c_hdr_mode mode);
>>>>>>>> int (*attach_i2c_dev)(struct i2c_dev_desc *dev);
>>>>>>>> void (*detach_i2c_dev)(struct i2c_dev_desc *dev);
>>>>>>>> int (*i2c_xfers)(struct i2c_dev_desc *dev,
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
Powered by blists - more mailing lists