| 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: <0435e96d-6786-4adc-83eb-a6aa3766c2d5@quicinc.com>
Date: Wed, 5 Feb 2025 19:49: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
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;
}
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