Re: [PATCH RFC v3 12/14] i2c: nomadik: add support for I2C_XFER_V2 - detailed fault reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] i2c: nomadik: unpopulated transfer report on pm_runtime_get_sync() early exit
--

commit 22d1a25cd553d39ed6c3ad6c9998ec471c05f948
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: nomadik: add support for I2C_XFER_V2 - detailed fault reporting

This commit converts the i2c-nomadik driver to the xfer_v2 callback so it can
populate struct i2c_transfer_report with the number of completed messages,
completed bytes, and the faulting message index when a transfer fails.

> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 958adc51615e4..a8ccd863a6edf 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c

[ ... ]

> @@ -739,8 +755,9 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *priv, u16 flags)
>   * please use the i2c_smbus_read_i2c_block_data()
>   * or i2c_smbus_write_i2c_block_data() API
>   */
> -static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
> -		struct i2c_msg msgs[], int num_msgs)
> +static int nmk_i2c_xfer_v2(struct i2c_adapter *i2c_adap,
> +		struct i2c_msg msgs[], int num_msgs,
> +		struct i2c_transfer_report *report)
>  {
>  	int status = 0;
>  	int i;
> @@ -750,6 +767,8 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	if (i)
>  		return i;
>  
> +	priv->bytes_cplt = 0;
> +
>  	/* setup the i2c controller */
>  	setup_i2c_controller(priv);
>  
> @@ -768,10 +787,15 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	pm_runtime_put_sync(&priv->adev->dev);
>  
>  	/* return the no. messages processed */
> -	if (status)
> +	if (status) {
> +		report->msgs_cplt = i;
> +		report->bytes_cplt = priv->bytes_cplt;
> +		report->fault_msg_idx = i;
>  		return status;
> -	else
> +	} else {
> +		i2c_transfer_report_success(report, num_msgs);
>  		return num_msgs;
> +	}
>  }

[Severity: Medium]
What happens to the transfer report in drivers/i2c/busses/i2c-nomadik.c when
nmk_i2c_xfer_v2() takes the early return on pm_runtime_get_sync()?

Before invoking adap->algo->xfer_v2(), __i2c_transfer_v2() in
drivers/i2c/i2c-core-base.c initializes all fields of struct
i2c_transfer_report to -EOPNOTSUPP via i2c_transfer_report_init(report):

drivers/i2c/i2c-core-base.c:__i2c_transfer_v2() {
	...
	if (adap->algo->xfer_v2) {
		i2c_transfer_report_init(report);
		...
		ret = adap->algo->xfer_v2(adap, msgs, num, report);
	...
}

In nmk_i2c_xfer_v2(), if pm_runtime_get_sync(&priv->adev->dev) returns a
non-zero value, the function returns immediately before reaching the block at
the end of nmk_i2c_xfer_v2() that populates report:

	i = pm_runtime_get_sync(&priv->adev->dev);
	if (i)
		return i;

If pm_runtime_get_sync() fails with a negative error code, 0 messages and 0
bytes have been transferred before failing on message 0, but report->msgs_cplt,
report->bytes_cplt, and report->fault_msg_idx remain -EOPNOTSUPP instead of 0,
telling callers of i2c_transfer_v2() and the I2C_RDWR_V2 ioctl that the
controller does not support fault reporting.

In addition, if pm_runtime_get_sync() returns 1 because the device was already
active, nmk_i2c_xfer_v2() returns 1 while leaving all fields of report set to
-EOPNOTSUPP, resulting in a positive return value paired with a negative
report->msgs_cplt.

Should nmk_i2c_xfer_v2() populate report (or use pm_runtime_resume_and_get()
and set msgs_cplt, bytes_cplt, and fault_msg_idx to 0 on error) before
returning when runtime resume fails?

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