All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mukesh Savaliya <mukesh.savaliya@oss.qualcomm.com>
To: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Andi Shyti <andi.shyti@kernel.org>,
	Aniket Randive <aniket.randive@oss.qualcomm.com>,
	Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, Wolfram Sang <wsa@kernel.org>
Subject: Re: [PATCH v6 1/2] i2c: core: Add i2c_update_timeout() helper for dynamic transfer timeouts
Date: Mon, 3 Aug 2026 17:22:59 +0530	[thread overview]
Message-ID: <9dcc47cd-2470-45b2-9106-51d4636cf60e@oss.qualcomm.com> (raw)
In-Reply-To: <amoaEGX8Q4zwHF2i@shikoro>



On 7/29/2026 8:49 PM, Wolfram Sang wrote:
> Hi,
> 
>> I agree that the precise timeout is platform dependent and cannot be derived
>> exactly from the transfer parameters alone. My intention is not to determine
>> the perfect value, but rather to provide a reasonable kernel-side default
>> for cases where no timeout has been configured explicitly.
> 
> We have that already. From the I2C core:
> 
> 1572         /* Set default timeout to 1 second if not already set */
> 1573         if (adap->timeout == 0)
> 1574                 adap->timeout = HZ;
> 
> This may not meet your definition of 'reasonable', though, I understand
> that. But you need to be aware that you immediately enter
> regression-area if you change this behaviour.
> 
Yes, 1HZ it's not reasonable for smaller transfers. Agree too that 
changing this may cause regression to few others.

>> Since kernel-space clients have no generic mechanism to tune adapter
>> timeouts on a per-system basis, deriving a baseline from the transfer length
> 
> This would be easy to add. We could introduce
> i2c_client_request_timeout_margin(client, desired_timeout) or something alike
> with basically doing:
> 
>    client->adapter->timeout = max(client->adapter->timeout, desired_timeout);
> 
> Or? Then we would get the theoretical value of a client. Which is maybe
> exceeded by the board specific timeout set by the board designer. It
> gets tricky, though, with userspace. Who has precedence then?
> 

Thinking to give precedence to user space here in such case. if no 
userspace setting timeout, then default will continue with core set timeout.

>> I am also suggesting let userspace add something on top of this if the core
>> derived final timeout is not sufficient.
> 
> Why can't userspace set an absolute value like now?
> 

So, does it mean user space can override kernel/core calculated timeout 
? if yes, i agree to this idea.

>>
>> This is an option for userspace. Should we expose device attributes for
>> kernel space ?
> 
> See above. adap->timeout is easily accessible.
> 
>> Yes, and I fully support keeping I2C_TIMEOUT as the mechanism for userspace
>> adjustment. What I am proposing is complementary rather than a replacement.
>> The core could calculate a baseline timeout from the transfer
>> characteristics and apply a conservative margin, while I2C_TIMEOUT would
>> remain available for systems that require additional headroom beyond the
>> default calculation.
> 
> If you have two ways of setting a timeout, people might get confused.
> 

In that case, let's decide if userspace configured timeout wins. If not 
set by user, then set calculated timeout by core layer.

I was thinking, user space may not always set the timeout but core layer 
will always need some timeout value based on formulae aniket has kept.

>>>> Do you see cases where a transfer-time-based timeout with a generous
>>>> system-latency margin would still be insufficient?
>>>
>>> Regressions. You could time out too early on boards which worked before.
>>
>> That is a valid concern. My assumption is that any calculated timeout would
>> include a sufficiently conservative margin, based on measurements across a
>> range of systems, so that existing working platforms would not regress.
> 
> You simply cannot guarantee this.
> 

Understood now, it may cause regression.

>> platform still requires significantly larger values due to exceptional
>> latency characteristics, I would expect that requirement to be addressed
>> through the existing timeout override mechanism rather than by forcing every
>> client to use a large fixed timeout.
> 
> The only way to deal with this is 'opt_in', not 'opt_out'. If you want
> to provide different defaults than the existing ones, I think you should
> make this available via a kernel config option, so somebody has to make
> an active decision "I want that and I know it can regress".
> 
> I am still not convinced this is all worth the hazzle, but let's keep
> discussing...
> 

This looks like a reasonable compromise to discuss and converge on. A 
polished version:

1. Userspace-configured timeout takes precedence over any timeout 
configured by the kernel.
2. When userspace does not configure a timeout, the kernel-computed 
timeout is used.
3. To avoid regressions, introduce a Kconfig option for formula-based 
timeout calculation:
	A. If the Kconfig option is enabled, derive the timeout using
            the proposed formula-based approach.
	B. If the Kconfig option is disabled, retain the existing
            default timeout behavior (currently 1 second) to preserve
            backward compatibility.


> Happy hacking,
> 
>     Wolfram
> 


  parent reply	other threads:[~2026-08-03 11:53 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-20 11:41 [PATCH v6 0/2] i2c: Add dynamic transfer timeout based on message length and frequency Aniket Randive
2026-07-20 11:41 ` [PATCH v6 1/2] i2c: core: Add i2c_update_timeout() helper for dynamic transfer timeouts Aniket Randive
2026-07-22  5:25   ` Mukesh Savaliya
2026-07-24 11:46     ` Aniket RANDIVE
2026-07-26 20:11   ` Andi Shyti
2026-07-27  4:15     ` Mukesh Savaliya
2026-07-27 20:42       ` Andi Shyti
2026-07-28  4:56         ` Mukesh Savaliya
2026-07-28  9:42           ` Wolfram Sang
2026-07-28 10:14             ` Mukesh Savaliya
2026-07-29 15:19               ` Wolfram Sang
2026-07-30 11:53                 ` Aniket RANDIVE
2026-07-30 20:23                   ` Wolfram Sang
2026-08-01 21:02                     ` Wolfram Sang
2026-08-03 10:53                       ` Aniket RANDIVE
2026-08-03 11:52                 ` Mukesh Savaliya [this message]
2026-07-20 11:41 ` [PATCH v6 2/2] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency Aniket Randive
2026-07-22  5:26   ` Mukesh Savaliya
2026-07-24 11:51     ` Aniket RANDIVE
2026-07-22  5:25 ` [PATCH v6 0/2] i2c: Add dynamic transfer timeout based on message " Mukesh Savaliya
2026-07-24 11:56   ` Aniket RANDIVE

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9dcc47cd-2470-45b2-9106-51d4636cf60e@oss.qualcomm.com \
    --to=mukesh.savaliya@oss.qualcomm.com \
    --cc=andi.shyti@kernel.org \
    --cc=aniket.randive@oss.qualcomm.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=viken.dadhaniya@oss.qualcomm.com \
    --cc=wsa+renesas@sang-engineering.com \
    --cc=wsa@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.