| 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: <11695367-f9cb-46be-9f83-b6f6ec5e42f4@quicinc.com>
Date: Mon, 3 Feb 2025 10:52:53 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Joshua Yeong <joshua.yeong@...rfivetech.com>, Frank Li <Frank.Li@....com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Miquel Raynal
<miquel.raynal@...tlin.com>,
Conor Culhane <conor.culhane@...vaco.com>
CC: "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 Joshua,
On 2/3/2025 8:02 AM, 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 think the 'private transfers' are limited to SDR communications,
> would it be better if we have a different interface/naming for hdr transfers?
>
>>> 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.
Yes, that would be fine too as we can clearly differentiate by
inteface/hook up name.
>>> {
>>> 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