Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <600b3bc1cdd570b0954f7a61865975f3f3158c90.camel@linux.ibm.com>
Subject: Re: [PATCH] i2c: mux: pca954x: Support multiple devices on a single
 reset line
From:   Eddie James <eajames@linux.ibm.com>
To:     Peter Rosin <peda@axentia.se>, linux-i2c@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org
Date:   Tue, 27 Jul 2021 10:59:51 -0500
In-Reply-To: <997d20b4-457c-17ba-e9bf-193132768afa@axentia.se>
References: <20210505215918.45720-1-eajames@linux.ibm.com>
         <997d20b4-457c-17ba-e9bf-193132768afa@axentia.se>
Organization: IBM
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>