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

[Mukesh Savaliya <mukesh.savaliya@oss.qualcomm.com>]

Thanks Andi !

On 7/28/2026 2:12 AM, Andi Shyti wrote:
> Hi Mukesh,
> 
> thanks for the nice discussion!
> 
> On Mon, Jul 27, 2026 at 09:45:23AM +0530, Mukesh Savaliya wrote:
>> On 7/27/2026 1:41 AM, Andi Shyti wrote:
>>> 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.
>>>
>>
>> Can we make timeout handling a kernel responsibility and avoid placing this
>> burden on userspace ? Every userspace client already accesses the bus
>> through the adapter/core layer, so the kernel has full visibility into
>> factors such as bus frequency, transfer size, and controller characteristics
>> to compute a sensible default timeout.
>>
>> Also, if both a userspace provided timeout and a kernel-calculated timeout
>> are specified, which one should win? Having two independent timeout
>> mechanisms could easily lead to ambiguity and inconsistent behavior.
> 
> There is no explicit policy for this today, and I am not against
> a driver selecting a timeout according to its own requirements.
> 
> However, once userspace explicitly sets I2C_TIMEOUT, I would
> expect that value to take precedence. Otherwise, the ioctl
> succeeds but the value can later be silently overwritten by
> the driver.
> 
>>>> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
>>>
>>> ...
>>>
>>>> +/**
>>
>> [...]
>>
>>>> +}
>>>> +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.
>>>
>>
>> There may not always be a userspace application available to configure the
>> timeout.
> 

Agree.

> That is fine. I do not expect userspace to configure the timeout
> during normal operation. The ioctl is mostly useful for
> debugging
> 
> The driver can still provide its own default when userspace has
> not selected one.
> 

yes, agree.

>> My suggestion was that the I²C core could derive the timeout based on the
>> bus frequency, transfer length, and a reasonable safety margin. This would
>> provide a generic solution that works across all clients without requiring
>> userspace involvement.
> 
> Is there a standard formula which works for every I2C controller?
> 

My thinking was that we are trying to derive a timeout for transfer 
completion, so the transfer length and bus frequency should already give
us the theoretical on-the-wire transfer time.

For example, the transfer time could be estimated from the number of its 
transferred, including protocol overhead, and then converted into a 
timeout value:

bit_usec = mul_u64_u32_div(len * 9, USEC_PER_SEC, bus_freq_hz);

On top of that, we could add a fixed margin to account for interrupt and
system scheduling latency before converting the result to jiffies.

The exact margin is open for discussion. I was considering something on
the order of a few hundred milliseconds (e.g. 500 ms), but perhaps that
is still too optimistic on some systems?

Alternatively, the core could provide a calculated baseline timeout
(transfer time + fixed margin) and allow userspace to add an optional
extra offset when needed. That way the default behavior remains 
automatic and works for most clients, while systems with unusual latency
requirements can still increase the timeout without every userspace 
client having to determine an appropriate value itself.

Do you see cases where a transfer-time-based timeout with a generous
system-latency margin would still be insufficient?

> The timeout may depend on controller specific behaviour, hardware
> limitations, clock stretching, FIFO handling and whether the
> driver uses interrupts or polling. A calculation based only on
> the transfer length and bus frequency does not seem generic
> enough to cover all of that.
> 
>> To account for variations in system load, we could optionally expose a DT
>> property for an offset or scaling coefficient to tune the computed timeout.
>> Even with such tuning, I believe this approach would be significantly better
>> than the current fixed 1-second (HZ) timeout.
> 
> I do not think a safety margin or scaling coefficient belongs in
> devicetree. Devicetree should describe the hardware, not a
> software timeout policy.
> 
> If a controller has a real hardware timeout parameter, that can
> be described by the DT. Otherwise, I think the timeout policy
> should remain in the driver.
> 

Fully agree, this DT property option is not valid unless its controller 
implemented.

> Thanks,
> Andi
> 
>>> Thanks,
>>> Andi
>>>
>>>> +
>>