Re: Deadlock in spi_add_device() -- device core problem

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

On Wed, Oct 13, 2021 at 08:52:35AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 12, 2021 at 09:30:05PM +0200, Uwe Kleine-König wrote:
> > Hi Mark,
> > 
> > On Fri, Oct 08, 2021 at 02:31:57PM +0100, Mark Brown wrote:
> > > On Thu, Oct 07, 2021 at 06:52:14PM +0200, Uwe Kleine-König wrote:
> > > > On Thu, Oct 07, 2021 at 06:19:46PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Thu, Oct 07, 2021 at 06:05:24PM +0200, Uwe Kleine-König wrote:
> > > 
> > > > > Drivers for a bus bind to the bus, they should not be creating new
> > > > > devices for that same bus, as that does not seem correct.
> > > 
> > > > That's not the culprit here. We have a spi-device (spi-mux) that is a
> > > > bus device on the SoC's bus and a bus master for it's own bus. And
> > > > spi_add_device for the spi-mux device triggers the probe function for
> > > > the spi-mux driver which creates a new bus controller which triggers
> > > > spi_add_device() for the devices on the inner bus.
> > > 
> > > I think we need to be arranging for spi_add_lock to be per bus
> > > rather than global - putting it into the controller ought to do
> > > the trick.
> > 
> > Yeah, that's what I consider the second best option that I already
> > mentioned in the initial mail of this thread.

I tested that a bit, I'll have to address an lockdep splat for it
(because the lock is used in a nested way), otherwise it at least fixes
the deadlock.

> > @Greg: Would you expect that it should be possible (and benificial) to
> > rework the code to not hold a lock at all during device_add()?
> 
> rework the driver core or the spi code?
> 
> /me is confused...

I expect the driver core to be fine. I meant the spi core. i.e. is it
bad enough that the spi core holds a lock during device_add() to
justify some effort to fix that; and is such a fix even possible?

> > This would then need something like:
> > 
> > 	lock() # either per controller or global
> > 	if bus already has a device for the same chipselect:
> > 	    error out;
> > 	register chipselect as busy
> > 	unlock();
> > 
> > 	ret = device_add(...)
> > 
> > 	if ret:
> > 	    lock()
> > 	    unregister chipselect
> > 	    unlock()
> > 
> > Is this worth the effort?
> 
> Watch out that you do not have probe() calls racing each other, we have
> guaranteed that they will be called serially in the past.

That won't happen because if there are really two devices to be
registered on the same CS line, only for one of them device_add() is
called. Ah, there is an ambiguity in my pseudo code, I guess this one is
better:

 	lock() # either per controller or global
 	if bus already has the CS marked as busy:
 	    error out;
 	mark chipselect as busy
 	unlock();
 
 	ret = device_add(...)
 
 	if ret:
 	    lock()
 	    mark chipselect as free
 	    unlock()

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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