linux-kernel - Re: [PATCH v3 6/7] i2c: designware: Implement I2C_M

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: <aR4inuzOitdpSDvd@smile.fi.intel.com>
Date: Wed, 19 Nov 2025 22:03:42 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Benoît Monin <benoit.monin@...tlin.com>
Cc: Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Jan Dabros <jsd@...ihalf.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Clark Williams <clrkwllms@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Gregory CLEMENT <gregory.clement@...tlin.com>,
	Théo Lebrun <theo.lebrun@...tlin.com>,
	Tawfik Bayouk <tawfik.bayouk@...ileye.com>,
	Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
	Dmitry Guzman <dmitry.guzman@...ileye.com>,
	linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-rt-devel@...ts.linux.dev
Subject: Re: [PATCH v3 6/7] i2c: designware: Implement I2C_M_STOP support

On Wed, Nov 19, 2025 at 04:05:35PM +0100, Benoît Monin wrote:
> Add the support of the I2C_M_STOP flag in i2c_msg by splitting
> i2c_dw_xfer() in two: __i2c_dw_xfer_one_part() for the core transfer logic
> and i2c_dw_xfer() for handling the high-level transaction management.
> 
> In detail __i2c_dw_xfer_one_part() starts a transaction and wait for its
> completion, either with a STOP on the bus or an error. i2c_dw_xfer()
> loops over the messages to search for the I2C_M_STOP flag and calls
> __i2c_dw_xfer_one_part() for each part of the messages up to a STOP or
> the end of the messages array.
> 
> i2c_dw_xfer() takes care of runtime PM and holds the hardware lock on
> the bus while calling __i2c_dw_xfer_one_part(), this allows grouping
> multiple accesses to device that support a STOP in a transaction when
> done via i2c_dev I2C_RDWR ioctl.
> 
> Also, now that we have a lookup of the messages in i2c_dw_xfer() prior
> to each transaction, we use it to make sure the messages are valid for
> the transaction. We check that the target address does not change before
> starting the transaction instead of aborting the transfer while it is
> happening, as it was done in i2c_dw_xfer_msg(). The target address can
> only be changed after an I2C_M_STOP flag, i.e after a STOP on the i2c bus.
> 
> The I2C_FUNC_PROTOCOL_MANGLING flag is added to the list of
> functionalities supported by the controller, except for the AMD NAVI
> i2c controller which uses its own xfer() function and is left untouched.

...

>  	ret = -EIO;
>  
> +done:
> +	return ret;

I don't see the point of keeping this label anymore. Is there a followup
that addresses this?

> +static inline bool
> +i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t idx)
> +{
> +	/* The first message of a transaction is valid */
> +	if (!idx)
> +		return true;
> +
> +	/*
> +	 * We cannot change the target address during a transaction, so make
> +	 * sure the address is identical to the one of the previous message.
> +	 */
> +	if (msgs[idx - 1].addr != msgs[idx].addr) {
> +		dev_err(dev->dev, "invalid target address\n");
> +		return false;
> +	}
> +
> +	return true;
> +}

Hmm... I would do it in the nested conditionals if nothing comes between
the lines in the future.

static inline bool
i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t idx)
{
	/*
	 * We cannot change the target address during a transaction, so make
	 * sure the address of the sequential messages is identical to the one
	 * of the previous message.
	 */
	if (idx && (msgs[idx - 1].addr != msgs[idx].addr) {
		dev_err(dev->dev, "invalid target address\n");
		return false;
	}

	/*
	 * Addresses match. For the first message return true as it sets the
	 * address for the whole transaction.
	 */
	return true;
}

...

> +static int
> +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +	struct i2c_msg *msgs_part;
> +	size_t cnt;
> +	int ret;
> +
> +	dev_dbg(dev->dev, "msgs: %d\n", num);
> +
> +	ACQUIRE(pm_runtime_active_auto_try, pm)(dev->dev);
> +	if (ACQUIRE_ERR(pm_runtime_active_auto_try, &pm))
> +		return -ENXIO;
> +
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If the I2C_M_STOP is present in some the messages,
> +	 * we do one transaction for each part up to the STOP.
> +	 */
> +	for (msgs_part = msgs; msgs_part < msgs + num; msgs_part += cnt) {
> +		/*
> +		 * Count the messages in a transaction, up to a STOP
> +		 * or the end of the msgs.
> +		 */
> +		for (cnt = 1; ; cnt++) {
> +			if (!i2c_dw_msg_is_valid(dev, msgs_part, cnt - 1)) {
> +				ret = -EINVAL;
> +				goto done;

What about

				break;

> +			}
> +
> +			if ((msgs_part[cnt - 1].flags & I2C_M_STOP) ||
> +			    (msgs_part + cnt == msgs + num))
> +				break;
> +		}
		if (ret < 0)
			break;

?

The motivation is to avoid mixing auto ptr with goto style
(it's not recommended as per cleanup.h).

> +		/* transfer one part up to a STOP */
> +		ret = __i2c_dw_xfer_one_part(dev, msgs_part, cnt);
> +		if (ret < 0)
> +			break;
> +	}
> +
>  done:
>  	i2c_dw_release_lock(dev);
>  
> -	return ret;
> +	if (ret < 0)
> +		return ret;
> +	return num;
>  }

-- 
With Best Regards,
Andy Shevchenko