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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 3249 bytes --]

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.

> 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?

> 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?

> 
> 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.

> > > 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.

> 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...

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]