linux-kernel - Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 14 May 2022 14:43:18 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Peter Rosin <peda@...ntia.se>, devicetree@...r.kernel.org
Cc:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Eddie James <eajames@...ux.ibm.com>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, krzysztof.kozlowski+dt@...aro.org,
        robh+dt@...nel.org, lars@...afoo.de, miltonm@...ibm.com,
        linux-i2c@...r.kernel.org
Subject: Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for
 skipping reset in probe

On Sat, 14 May 2022 00:48:51 +0200
Peter Rosin <peda@...ntia.se> wrote:

> Hi!
> 
> 2022-05-13 at 18:45, Jonathan Cameron wrote:
> > On Thu, 12 May 2022 12:08:07 -0500
> > Eddie James <eajames@...ux.ibm.com> wrote:
> >   
> >> On 5/12/22 11:48, Jonathan Cameron wrote:  
> >>> On Thu, 12 May 2022 11:20:18 -0500
> >>> Eddie James <eajames@...ux.ibm.com> wrote:
> >>>    
> >>>> I2C commands issued after the SI7020 is starting up or after reset
> >>>> can potentially upset the startup sequence. Therefore, the host
> >>>> needs to wait for the startup sequence to finish before issuing
> >>>> further i2c commands. This is impractical in cases where the SI7020
> >>>> is on a shared bus or behind a mux, which may switch channels at
> >>>> any time (generating I2C traffic). Therefore, check for a device
> >>>> property that indicates that the driver should skip resetting the
> >>>> device when probing.    
> >>> Why not lock the bus?  It's not ideal, but then not resetting and hence
> >>> potentially ending up in an unknown state isn't great either.    
> >>
> >>
> >> Agreed, but locking the bus doesn't work in the case where the chip is 
> >> behind a mux. The mux core driver deselects the mux immediately after 
> >> the transfer to reset the si7020, causing some i2c traffic, breaking the 
> >> si7020. So it would also be a requirement to configure the mux to idle 
> >> as-is... That's why I went with the optional skipping of the reset. 
> >> Maybe I should add the bus lock too?
> >>  
> > 
> > +Cc Peter and linux-i2c for advice as we should resolve any potential
> > issue with the mux side of things rather than hiding it in the driver
> > (if possible!)  
> 
> IIUC, the chip in question cannot handle *any* action on the I2C bus
> for 15ms (or so) after a "soft reset", or something bad<tm> happens
> (or at least may happen).
> 
> If that's the case, then providing a means of skipping the reset is
> insufficient. If you don't lock the bus, you would need to *always*
> skip the reset, because you don't know for certain if something else
> does I2C xfers.
> 
> So, in order to make the soft reset not be totally dangerous even in
> a normal non-muxed environment, the bus must be locked for the 15ms.
> 
> However, Eddie is correct in that the I2C mux code may indeed do its
> muxing xfer right after the soft reset command. There is currently
> no way to avoid that muxing xfer. However, it should be noted that
> there are ways to mux an I2C bus without using xfers on the bus
> itself, so it's not problematic for *all* mux variants.
> 
> It can be debated if the problem should be worked around with extra
> dt properties like this, or if a capability should be added to delay
> a trailing muxing xfer.
> 
> I bet there are other broken chips that have drivers that do in fact
> lock the bus to give the chip a break, but then it all stumbles
> because of the unexpected noise if there's a (wrong kind of) mux in
> the mix.

Ok, so for now I think we need the bus lock for the reset + either
a work around similar to this series, or additions to the i2c mux code
to stop it doing a muxing xfer if the bus is locked?

Jonathan

> 
> Cheers,
> Peter
> 
> > 
> > Jonathan
> > 
> > 
> > 
> >   
> >>
> >> Thanks,
> >>
> >> Eddie
> >>
> >>  
> >>>
> >>> Jonathan
> >>>    
> >>>> Changes since v1:
> >>>>   - Fix dt binding document
> >>>>
> >>>> Eddie James (2):
> >>>>    dt-bindings: iio: humidity: Add si7020 bindings
> >>>>    iio: humidity: si7020: Check device property for skipping reset in probe
> >>>>
> >>>>   .../bindings/iio/humidity/silabs,si7020.yaml  | 47 +++++++++++++++++++
> >>>>   .../devicetree/bindings/trivial-devices.yaml  |  2 -
> >>>>   drivers/iio/humidity/si7020.c                 | 14 +++---
> >>>>   3 files changed, 55 insertions(+), 8 deletions(-)
> >>>>   create mode 100644 Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
> >>>>    
> >