Re: [PATCH RFC v3 10/14] i2c: doc: add section for I2C_RDWR_V2 ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] i2c: dev: NULL msgs with non-zero nmsgs accepted in i2cdev_ioctl()
- [Low] i2c: doc: inaccurate fault_msg_idx description in dev-interface.rst
- [Low] i2c: core: mismatched kernel-doc identifier above i2c_transfer()
--

commit 417441f5bb2c43e4da176d457da7a5caf43a4ed0
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: doc: add section for I2C_RDWR_V2 ioctl

This series introduces the I2C_RDWR_V2 ioctl and i2c_transfer_v2() API to
provide detailed fault reporting for I2C transfers, along with the
corresponding documentation in Documentation/i2c/dev-interface.rst.

> diff --git a/Documentation/i2c/dev-interface.rst b/Documentation/i2c/dev-interface.rst
> index c277a8e1202b5..95e60bdb2f4d0 100644
> --- a/Documentation/i2c/dev-interface.rst
> +++ b/Documentation/i2c/dev-interface.rst
> @@ -140,6 +140,54 @@ The following IOCTLs are defined:
>    The slave address and whether to use ten bit address mode has to be
>    set in each message, overriding the values set with the above ioctl's.
>  
> +``ioctl(file, I2C_RDWR_V2, struct i2c_rdwr_v2_ioctl_data *msgset)``
> +  Does the same combined read/write transaction as I2C_RDWR, but also
> +  provides detailed fault report. The argument is a pointer to a::
> +
> +    struct i2c_rdwr_v2_ioctl_data {
> +	    struct i2c_rdwr_ioctl_data rdwr_data;
> +	    struct i2c_transfer_report report;
> +    };
> +
> +  rdwr_data is the same structure as the argument for I2C_RDWR ioctl.
> +  report is the structure that the transfer report is written to::
> +
> +    struct i2c_transfer_report {
> +      __s32 fault_msg_idx;
> +      __s32 msgs_cplt;
> +      __s32 bytes_cplt;
> +    };
> +
> +  msgs_cplt is the number of messages that has been sent or received
> +  successfully. If there are read messages within this range, the returned
> +  data is guaranteed to be valid. If a message has been read from the
> +  device but the read data is lost (for example, FIFO is flushed before
> +  CPU read it), this message must not be counted.  If the controller cannot
> +  determine the number of completed messages, the value is -EOPNOTSUPP.
> +
> +  fault_msg_idx is the number of messages that caused a fault. In case of a
> +  fault, it is not necessary equal to msgs_cplt. For example, if the driver
> +  validates the whole batch before starting transmission, detects that it
> +  cannot send it, it returns -EOPNOTSUPP error immediately, so msgs_cplt is 0,
> +  while fault_msg_idx points to the message that cannot be sent. Another
> +  example when these two value may be different is I2C controller that
> +  flushes RX FIFO when an error is detected before CPU reads data from it.

[Severity: Low]
In Documentation/i2c/dev-interface.rst under the I2C_RDWR_V2 description, is
describing fault_msg_idx as "the number of messages that caused a fault"
accurate?

In include/uapi/linux/i2c.h, struct i2c_transfer_report defines fault_msg_idx
as the 0-based index of the message that caused the fault, or a negative error
code (-EOPNOTSUPP via i2c_transfer_report_init()) if the bus driver cannot
determine it:

    struct i2c_transfer_report {
        __s32 fault_msg_idx; /* In case of a fault, index of the message that caused
                              * the fault. If the bus driver cannot determine it, it
                              * puts a negative error code. If there is no fault, the
                              * value is equal to number of messages transferred.
                              */

Should this paragraph state that fault_msg_idx is the index of the message
that caused a fault, and also mention that its value is -EOPNOTSUPP if the
controller cannot determine which message caused the fault?

[ ... ]

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 165ec14260c88..e8449f55f0452 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c

[ ... ]

> +/**
> + * i2c_transfer_v2 - execute a single or combined I2C message
>   * @adap: Handle to I2C bus
>   * @msgs: One or more messages to execute before STOP is issued to
>   *	terminate the operation; each message begins with a START.
> @@ -2371,35 +2453,18 @@ EXPORT_SYMBOL(__i2c_transfer);
>   */
>  int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  {
> -	int ret;
> -
> -	/* REVISIT the fault reporting model here is weak:
> -	 *
> -	 *  - When we get an error after receiving N bytes from a slave,
> -	 *    there is no way to report "N".
> -	 *
> -	 *  - When we get a NAK after transmitting N bytes to a slave,
> -	 *    there is no way to report "N" ... or to let the master
> -	 *    continue executing the rest of this combined message, if
> -	 *    that's the appropriate response.
> -	 *
> -	 *  - When for example "num" is two and we successfully complete
> -	 *    the first message but get an error part way through the
> -	 *    second, it's unclear whether that should be reported as
> -	 *    one (discarding status on the second message) or errno
> -	 *    (discarding status on the first one).
> -	 */
> -	ret = __i2c_lock_bus_helper(adap);
> -	if (ret)
> -		return ret;
> -
> -	ret = __i2c_transfer(adap, msgs, num);
> -	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
> -
> -	return ret;
> +	return i2c_transfer_v2(adap, msgs, num, NULL);
>  }

[Severity: Low]
This isn't a runtime bug, but the kernel-doc comment immediately above
i2c_transfer() in drivers/i2c/i2c-core-base.c was renamed to i2c_transfer_v2,
which causes a kernel-doc warning when building with W=1. Should the
identifier in the comment header remain i2c_transfer?

> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index d219eb89f978b..14fe1dd7eecf4 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c

[ ... ]

> @@ -439,18 +482,39 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		funcs = i2c_get_functionality(client->adapter);
>  		return put_user(funcs, (unsigned long __user *)arg);
>  
> -	case I2C_RDWR: {
> +	case I2C_RDWR:
> +	case I2C_RDWR_V2:
> +	{
> +		struct i2c_rdwr_ioctl_data __user *user_arg;
> +		struct i2c_transfer_report __user *user_rep;
>  		struct i2c_rdwr_ioctl_data rdwr_arg;
>  		struct i2c_msg *rdwr_pa;
>  		int res;
>  
> -		if (copy_from_user(&rdwr_arg,
> -				   (struct i2c_rdwr_ioctl_data __user *)arg,
> -				   sizeof(rdwr_arg)))
> +		if (cmd == I2C_RDWR_V2) {
> +			user_arg = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->rdwr_data;
> +			user_rep = &((struct i2c_rdwr_v2_ioctl_data __user *)arg)->report;
> +		} else {
> +			user_arg = (struct i2c_rdwr_ioctl_data __user *)arg;
> +			user_rep = NULL;
> +		}
> +
> +		if (copy_from_user(&rdwr_arg, user_arg, sizeof(rdwr_arg)))
>  			return -EFAULT;
>  
> -		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0)
> -			return -EINVAL;
> +		if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0) {
> +			/*
> +			 * I2C_RDWR_V2 ioctl with nmsgs == 0 is used for
> +			 * discovering of the controller capability to return
> +			 * detailed fault reports.
> +			 */
> +			if (cmd == I2C_RDWR)
> +				return -EINVAL;
> +			if (client->adapter->algo->xfer_v2)
> +				return 0;
> +			else
> +				return -EOPNOTSUPP;
> +		}

[Severity: Medium]
In i2cdev_ioctl() (and the identical check in compat_i2cdev_ioctl()) in
drivers/i2c/i2c-dev.c, what happens if user space invokes I2C_RDWR_V2 with
rdwr_arg.msgs == NULL and rdwr_arg.nmsgs > 0?

Because both conditions are combined into:

    if (!rdwr_arg.msgs || rdwr_arg.nmsgs == 0)

a call with a NULL msgs pointer and non-zero nmsgs enters the capability
discovery branch and returns 0 when client->adapter->algo->xfer_v2 is set,
without transferring any messages or initializing the user-space report
structure.

Should capability discovery only trigger when rdwr_arg.nmsgs == 0, while
!rdwr_arg.msgs with non-zero nmsgs returns -EINVAL in both i2cdev_ioctl() and
compat_i2cdev_ioctl()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-i2c-fault-reporting-v3-0-515ad74ac4c9@mobileye.com?part=10