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

[Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>]

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.
> 
> @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...

> 
> 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.

thanks,

greg k-h