| 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: <19ea209c-cf55-4ec4-adae-16033fc7d819@starfivetech.com>
Date: Wed, 5 Feb 2025 06:30:49 +0000
From: Joshua Yeong <joshua.yeong@...rfivetech.com>
To: Frank Li <Frank.li@....com>
CC: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.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
On 04-Feb-2025 11:38 PM, Frank Li wrote:
> On Tue, Feb 04, 2025 at 03:17:36AM +0000, Joshua Yeong wrote:
>> Hi Frank,
>>
>> On 03-Feb-2025 11:52 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.
>>>
>>>>
>>>> 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?
>>
>> I guess mainly the terminology. The specification differentiate HDR from private transfer.
>> We can have the hdr interface to have length indicating for u16 rather than u8 in private trasnfer.
>
> It should be equivalence with check length is even.
>
> The key is how client driver will easy to use HDR if they already have SDR
> support.
>
> suppose client:
>
> struct i3c_priv_xfer xfer[2] = {{ .rnw = 0, buf=p1, size = 4, addr=0xb },
> { .rnw = 1, buf=p2, size = 4, addr=0xb }}
>
> priv_xfer(..., xfer, 2};
>
> if support HDR
>
> if (hdr) {
> xfer[0].cmd = 0x80;
> xfer[1].cmd = 0x;
> priv_xfer(..., xfer, 2, HDR);
> } else {
> priv_xfer(..., xfer, 2, SDR);
> }
>
> client driver needn't distingiush if buff is u8* or u16*.
>
I fine with the overall structure `.rnw`, `buf`, `size` and `addr`.
I prefer if we could use `hdr_xfer` rather than `priv_xfer` for HDR transfer and we can have
distinguish mode for DDR/TSP/TSL/BT. In case we are supporting for ML in the future
we dont have to lump the function into `priv_xfer`, and we can go `ml_xfer` instead.
The issue with length indicating both u8 and u16 here is that we would get confuse how the
buffer length is interpreted, when length is 1 does it mean 2 bytes or 1 byte? Splitting HDR
interface will ensure that length always indicate u16.
Joshua
>> I think the framework should check if device whether it supports HDR command before sending it.
>> I have code here that do some handling on that.
>
> Yes, I can add such check. The key point is API design.
>
> Frank
>>
>>>
>>>>
>>>>>> 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
>>
>> Yes, I think it is better if we split xfer transfer interface for SDR and HDR.
>> The interface should assume respective driver to decide how to handle ENTHDR CCC + Exit Pattern.
>> Else we will need additional 2 interface for start and stop for HDR.
>>
>> Joshua
>>
>>
>>>>>> {
>>>>>> 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