Re: [PATCH v6 1/2] i2c: core: Add i2c_update_timeout() helper for dynamic transfer timeouts

[Andi Shyti <andi.shyti@kernel.org>]

Hi Aniket,

On Mon, Jul 20, 2026 at 05:11:19PM +0530, Aniket Randive wrote:
> The transfer timeout for an I2C controller should reflect the actual
> message length and bus frequency rather than a static 1-second value.
> A static timeout causes unnecessary delays on error paths for short
> messages, and may be insufficient for very long transfers.
> 
> Add i2c_update_timeout() to i2c-core which computes a transfer-specific
> timeout and stores it directly in the standard adap->timeout field.  The
> formula accounts for 9 bits per byte (8 data + 1 ACK) at the configured
> bus frequency.  The caller supplies a safety multiplier and a minimum
> floor so that each driver retains full control over its timing policy
> without those values becoming public API.
> 
> Storing the result in adap->timeout makes it visible to all consumers of
> that field, including the arbitration-loss retry loop in __i2c_transfer().

this patch does not take into account a timeout set by userspace
through the I2C_TIMEOUT ioctl. That value would be silently
overwritten by the driver.

> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>

...

> +/**
> + * i2c_update_timeout - compute and set a dynamic transfer timeout on an adapter
> + * @adap: the i2c_adapter whose timeout field will be updated
> + * @bus_freq_hz: I2C bus clock frequency in Hz
> + * @len: transfer length in bytes
> + * @safety_coeff: multiplier applied over the theoretical wire time
> + * @min_usec: minimum timeout floor in microseconds
> + *
> + * Computes a transfer-specific timeout from the message length and bus
> + * frequency, applies a safety multiplier and a minimum floor, then stores
> + * the result in adap->timeout (in jiffies).  The caller supplies the policy
> + * constants so they remain internal to the driver.
> + */
> +void i2c_update_timeout(struct i2c_adapter *adap, u32 bus_freq_hz,
> +			size_t len, unsigned int safety_coeff,
> +			unsigned int min_usec)
> +{
> +	u64 bit_usec = mul_u64_u32_div(len * 9, USEC_PER_SEC, bus_freq_hz);

bus_freq_hz must be checked before the division, otherwise a zero
value will cause a division by zero.

> +	adap->timeout = usecs_to_jiffies(bit_usec * safety_coeff + min_usec);

If min_usec is a minimum timeout, why is it added to the
calculated value?

I would expect something like:

   max(bit_usec * safety_coeff, min_usec)

Otherwise, min_usec is an additional margin, not a minimum.

> +}
> +EXPORT_SYMBOL_GPL(i2c_update_timeout);

I'm wondering whether changing the adapter timeout from the I2C
core is the right approach.

Perhaps i2c_update_timeout() could return the calculated value
instead, but then  do not see much value in exposing such a small
calculation through the I2C core.

I know you were advised to move this into the core, but it does
not seem particularly useful there and, as it stands, it looks
somewhat controversial.

Unless there is another good reason for keeping it in the core,
I would drop this helper and keep the calculation in the driver.

Thanks,
Andi

> +