linux-kernel - Re: [PATCH] i2c: mux: pca954x: Support multiple devices on a single reset line

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] [day] [month] [year] [list]

Message-ID: <600b3bc1cdd570b0954f7a61865975f3f3158c90.camel@linux.ibm.com>
Date:   Tue, 27 Jul 2021 10:59:51 -0500
From:   Eddie James <eajames@...ux.ibm.com>
To:     Peter Rosin <peda@...ntia.se>, linux-i2c@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c: mux: pca954x: Support multiple devices on a single
 reset line

On Fri, 2021-05-07 at 00:08 +0200, Peter Rosin wrote:
> Hi!
> 
> On 2021-05-05 23:59, Eddie James wrote:
> > Some systems connect several PCA954x devices to a single reset
> > GPIO. For
> > these devices to get out of reset and probe successfully, only the
> > first
> > device probed should change the GPIO. Add this functionality by
> > checking
> > for EBUSY when getting the GPIO fails. Then, retry getting the GPIO
> > with
> > the non-exclusive flag and wait for the reset line to drop. This
> > prevents
> > the later probes from proceding while the device is still reset.
> 
> (nit: proceeding)
> 
> The patch assumes that all muxes with interconnected resets are only
> ever reset "in symphony". But there is no guarantee anywhere that
> this
> actually holds.

Thanks for your comments Peter, you are quite right, this won't do.
I've finally come back around to this and will send a new patch that
handles it differently - please let me know what you think.

Thanks!
Eddie

> 
> So, I don't see how this can be safe. Sure, it may very well work in
> the
> majority of cases, but it seems very dangerous. If one instance
> resets
> muxes controlled by other instances, any cached value is destroyed in
> those instances and anything can happen. Sure, if you have HW like
> this,
> then you have what you have. But I don't see any good way to handle
> this case in an elegant way. If this scheme is allowed the dangers of
> relying on it at minimum needs to be documented.
> 
> And what if the second instance reads the gpio just a few ns after
> the
> reset is released? The first instance waits for 1us before proceeding
> to give the chip some time to recover from the reset, but that
> respite
> may be lost to other instances.
> 
> What if the first instance does the reset but then fails the probe
> later,
> possibly because the chip isn't there, but then other instances
> manages
> to time their probe just so that the gpio is busy at the right point,
> and then proceeds without holding a reference to the gpio. Then the
> first
> instance also lets go of the gpio and you end up with a bunch of
> instances
> relying on a pin that noone holds a reference to. Or, yet another
> instance
> enters the picture and finds the gpio free and pulls a reset behind
> the
> back of the intermediate instances which have already proceeded.
> 
> Or am I reading something wrong?
> 
> Cheers,
> Peter
>