Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Peter Rosin <peda@axentia.se>]

On 2021-09-05 12:04, Jonathan Cameron wrote:
> On Tue, 31 Aug 2021 09:40:11 +0200
> Jacopo Mondi <jacopo@jmondi.org> wrote:
> 
>> Hi Jonathan,
>>    two more clarification requests, sorry to bother :)
> No problem.  First one: No idea +CC wolfram and i2c list.
>>
>> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
>>> On Mon, 30 Aug 2021 18:20:51 +0200  
>>>>>> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
>>>>>> +{
>>>>>> +	__be16 be_data = cpu_to_be16(data);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	sunrise_wakeup(sunrise);  
>>>>>
>>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
>>>>> between the wakeup and the following command.  That would make the device going back
>>>>> to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
>>>>> you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
>>>>> gives an example.  
>>>>
>>>> Right, there might be another call stealing the wakeup session!
>>>>
>>>> I should lock the underlying i2c bus, probably not root adapter like
>>>> mlx90614 does but only the segment.  
>>>
>>> Ideally only segment as you say as could be some muxes involved.  
>>
>> If not that i2c_smbus_xfer() which is called by my wakeup and by the
>> regmap_smbus_* calls tries to lock the segment as well, so we deadlock :)
>>
>> And actually, locking the underlying i2c segment seems even too
>> strict, what we have to guarantee is that no other read/write function
>> call from this driver interrupts a [wakeup-trasactions] sequence.
>>
>> Wouldn't it be better if I handle that with a dedicated mutex ?
> 
> I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c.
> 
> Short story here is we have a device which autonomously goes to sleep.
> Datasheet suggests waking it up with a failed xfer followed by what we
> actually want to do (sufficiently quickly).
> 
> Obviously we can't actually guarantee that will ever happen but it's a lot
> more likely to succeed if we briefly stop anything else using he i2c bus.
> 
> How should we handle this?

The way I read this is that interactions with other I2C devices that squeeze
in are not a fundamental problem. Not unless there are so many of these 3rd
party xfers that the device times out again. If my assessment is correct,
then I would suggest handling this in the driver by somehow making sure that
it doesn't clobber its own pairs of wakeup+work interactions. But see below.

Because there really is no way to protect against those extra I2C accesses.
With a parent-locked mux you can (ignoring arbitrators, where another
system may possibly take over the bus if too much time is spent between
two xfers that were supposed to be adjacent). But if there's a mux-locked
mux in the path it's simply not possible to lock out all other xfers on
the root adapter.

With a parent-locked I2C tree, "locking the segment" is equivalent to
locking everything all the way to the root adapter. But the whole point
of mux-locked muxes is that they can't operate if you do that. Mux-locked
muxes are allowed to depend on other (ignorant) drivers using other parts
of the I2C tree while the segment is locked. If you lock the root adapter
when there is a mux-locked mux involved, you simply kill that property
and sign up for a deadlock. Which also means that you cannot prevent 3rd
party xfers to other parts of the I2C tree.

However, if you worry about 3rd party xfers causing the wakeup to timeout
again and that only handling it in the driver as suggested above is
insufficient, then it's an option to lock the segment. For parent-locked I2C
trees, this should prevent (most) 3rd party actions and minimize the delay.
In the odd case that there are mux-locked muxes involved, there will simply
be a higher risk of failure, but there is little to do about that.

See Documentation/i2c/i2c-topology.rst for some discussion on the details
of mux-locked and parent-locked muxes.

I hope I make at least some sense...

Cheers,
Peter