linux-kernel - Re: [PATCH v6 1/5] i3c: Add HDR API support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aPnmCwwZVZ5egqkP@smile.fi.intel.com>
Date: Thu, 23 Oct 2025 11:23:39 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Frank Li <Frank.Li@....com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Miquel Raynal <miquel.raynal@...tlin.com>,
	Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-i3c@...ts.infradead.org,
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev,
	linux-iio@...r.kernel.org, joshua.yeong@...rfivetech.com,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v6 1/5] i3c: Add HDR API support

On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:
> Rename struct i3c_priv_xfer to struct i3c_xfer, since private xfer in the
> I3C spec refers only to SDR transfers. Ref: i3c spec ver1.2, section 3,
> Technical Overview.
> 
> i3c_xfer will be used for both SDR and HDR.
> 
> Rename enum i3c_hdr_mode to i3c_xfer_mode. Previous definition need match
> CCC GET_CAP1 bit position. Use 31 as SDR transfer mode.
> 
> Add i3c_device_do_xfers() with an xfer mode argument, while keeping
> i3c_device_do_priv_xfers() as a wrapper that calls i3c_device_do_xfers()
> with I3C_SDR for backward compatibility.
> 
> Introduce a 'cmd' field in struct i3c_xfer as an anonymous union with
> 'rnw', since HDR mode uses read/write commands instead of the SDR address
> bit.
> 
> Add .i3c_xfers() callback for master controllers. If not implemented, fall
> back to SDR with .priv_xfers(). The .priv_xfers() API can be removed once
> all controllers switch to .i3c_xfers().
> 
> Add 'mode_mask' bitmask to advertise controller capability.

...

>  static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
>  {
> -	if (!ops || !ops->bus_init || !ops->priv_xfers ||
> +	if (!ops || !ops->bus_init ||
>  	    !ops->send_ccc_cmd || !ops->do_daa || !ops->i2c_xfers)
>  		return -EINVAL;
>  
> +	if (!ops->priv_xfers && !ops->i3c_xfers)
> +		return -EINVAL;

I would find the logically coupled proto-based xfers:

	if (!ops->i2c_xfers && !ops->i3c_xfers)
		return -EINVAL;


>  	if (ops->request_ibi &&
>  	    (!ops->enable_ibi || !ops->disable_ibi || !ops->free_ibi ||
>  	     !ops->recycle_ibi_slot))

>  }

...

> -enum i3c_hdr_mode {
> +enum i3c_xfer_mode {
> +	/* The below 3 value (I3C_HDR*) must match GETCAP1 Byte bit position */
>  	I3C_HDR_DDR,
>  	I3C_HDR_TSP,
>  	I3C_HDR_TSL,
> +	/* Use for default SDR transfer mode */
> +	I3C_SDR = 0x31,

Why has this a specific value, while the rest have not? If it's HW mandated,
the all of them has to be assigned properly to avoid potential bugs.

>  };

...

>  /**
> - * struct i3c_priv_xfer - I3C SDR private transfer
> + * struct i3c_xfer - I3C data 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

>   * @data.out: output buffer. Must point to a DMA-able buffer
>   * @err: I3C error code
>   */
> -struct i3c_priv_xfer {
> -	u8 rnw;
> +struct i3c_xfer {
> +	union {
> +		u8 rnw;
> +		u8 cmd;
> +	};

What field is used to distinguish the union member in current use?
In another word, union must be accompanied with a selector.

>  	u16 len;
>  	u16 actual_len;
>  	union {

>  	enum i3c_error_code err;
>  };

...

> +/* keep back compatible */
> +#define i3c_priv_xfer i3c_xfer

How many of the current users do this? Can't we just rename treewide?

-- 
With Best Regards,
Andy Shevchenko