Deadlock in spi_add_device() -- device core problem

Deadlock in spi_add_device() -- device core problem

[Uwe Kleine-König]

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

Hello,

On an ARM board with an spi-mux I observe a deadlock during boot. This
happens because spi_add_device() (in drivers/spi/spi.c) for the spi-mux
device calls device_add() while holding &spi_add_lock. The spi-mux
driver's probe routine than creates another controller and for the
devices on that bus spi_add_device() is called again, trying to grab
&spi_add_lock again.

The easy workaround would be to replace &spi_add_lock with a
per-controller lock, but I have the expectation that it should be
possible to not hold a lock while calling device_add().

The difficulty (and the reason that this lock exists) is that it should
be prevented that more than one device is added for a any chipselect.
My first guess was, this check isn't necessary, because the devices are
called $busname.$chipselect and a second device on the same bus with the
same chipselect would fail when device_add() tries to create the
duplicate name. However for devices instanciated by ACPI the naming is
different (see spi_dev_set_name(), and commit b6fb8d3a1f15). Also there
is a comment "We need to make sure there's no other device with this
chipselect **BEFORE** we call setup(), else we'll trash its
configuration." A problem there might be that spi_setup() might toggle
the chipselect line (which is a problem if the chipselect polarity
isn't the same for the two devices, or the earlier device is currently
busy). Anything else?

There is also a check:

	if (IS_ENABLED(CONFIG_SPI_DYNAMIC) &&
	    !device_is_registered(&ctlr->dev)) {
		return -NODEV;
	}

which catches the race that the controller (which is also the device's
parent) is about to go while we try to add a new device to it. Is this a
real problem? (The spi_unregister_controller() function unregisters all
children which might race with adding a new child without locking. This
check was implemented by Lukas Wunner (on Cc:) in commit ddf75be47ca7.)
Doesn't all children of a given device are unregistered automatically by
the device core somehow when said device is unregistered? (I checked,
but didn't find anything.)

Does this all sound about right? Greg, do you have a recommendation how
to properly fix this problem?

Best regards
Uwe

PS: For now the workaround is to have the spi-mux driver as a module.
This way device_add() for the spi-mux device doesn't bind the driver and
the function returns (but triggers a module load which then can bind the
spi-mux device and create the new controller without a locking issue).

For the record, the relevant device-tree snippet looks as follows:

/ {
	...
        spimux: mux-controller {
                compatible = "gpio-mux";
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_spimux>;
                #mux-control-cells = <0>;
                mux-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
        };
};

&ecspi2 {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_ecspi2>;
        status = "okay";
        #address-cells = <1>;
        #size-cells = <0>;

        spi@0 {
                compatible = "spi-mux";
                reg = <0>;
                #address-cells = <1>;
                #size-cells = <0>;
                spi-max-frequency = <100000000>;
                mux-controls = <&spimux>;

                adc@0 {
                        compatible = "ti,ads7953";
                        reg = <0>;
                        spi-max-frequency = <10000000>;
                };

                adc@1 {
                        compatible = "ti,ads7953";
                        reg = <1>;
                        spi-max-frequency = <10000000>;
                };
        };
};


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

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

[Greg Kroah-Hartman]

On Thu, Oct 07, 2021 at 06:05:24PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On an ARM board with an spi-mux I observe a deadlock during boot. This
> happens because spi_add_device() (in drivers/spi/spi.c) for the spi-mux
> device calls device_add() while holding &spi_add_lock. The spi-mux
> driver's probe routine than creates another controller and for the
> devices on that bus spi_add_device() is called again, trying to grab
> &spi_add_lock again.
> 
> The easy workaround would be to replace &spi_add_lock with a
> per-controller lock, but I have the expectation that it should be
> possible to not hold a lock while calling device_add().

No, busses should not be adding new devices to the same bus from within
a probe function.

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.

> The difficulty (and the reason that this lock exists) is that it should
> be prevented that more than one device is added for a any chipselect.
> My first guess was, this check isn't necessary, because the devices are
> called $busname.$chipselect and a second device on the same bus with the
> same chipselect would fail when device_add() tries to create the
> duplicate name. However for devices instanciated by ACPI the naming is
> different (see spi_dev_set_name(), and commit b6fb8d3a1f15). Also there
> is a comment "We need to make sure there's no other device with this
> chipselect **BEFORE** we call setup(), else we'll trash its
> configuration." A problem there might be that spi_setup() might toggle
> the chipselect line (which is a problem if the chipselect polarity
> isn't the same for the two devices, or the earlier device is currently
> busy). Anything else?
> 
> There is also a check:
> 
> 	if (IS_ENABLED(CONFIG_SPI_DYNAMIC) &&
> 	    !device_is_registered(&ctlr->dev)) {
> 		return -NODEV;
> 	}
> 
> which catches the race that the controller (which is also the device's
> parent) is about to go while we try to add a new device to it. Is this a
> real problem? (The spi_unregister_controller() function unregisters all
> children which might race with adding a new child without locking. This
> check was implemented by Lukas Wunner (on Cc:) in commit ddf75be47ca7.)
> Doesn't all children of a given device are unregistered automatically by
> the device core somehow when said device is unregistered? (I checked,
> but didn't find anything.)
> 
> Does this all sound about right? Greg, do you have a recommendation how
> to properly fix this problem?

Don't allow it :)

Since when do spi devices find new spi devices on their same bus?  This
feels very wrong...

thanks,

greg k-h

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

[Uwe Kleine-König]

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

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:
> > On an ARM board with an spi-mux I observe a deadlock during boot. This
> > happens because spi_add_device() (in drivers/spi/spi.c) for the spi-mux
> > device calls device_add() while holding &spi_add_lock. The spi-mux
> > driver's probe routine than creates another controller and for the
> > devices on that bus spi_add_device() is called again, trying to grab
> > &spi_add_lock again.
> > 
> > The easy workaround would be to replace &spi_add_lock with a
> > per-controller lock, but I have the expectation that it should be
> > possible to not hold a lock while calling device_add().
> 
> No, busses should not be adding new devices to the same bus from within
> a probe function.
> 
> 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.

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

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

[Lukas Wunner]

On Thu, Oct 07, 2021 at 06:05:24PM +0200, Uwe Kleine-König wrote:
> On an ARM board with an spi-mux I observe a deadlock during boot. This
> happens because spi_add_device() (in drivers/spi/spi.c) for the spi-mux
> device calls device_add() while holding &spi_add_lock. The spi-mux
> driver's probe routine than creates another controller and for the
> devices on that bus spi_add_device() is called again, trying to grab
> &spi_add_lock again.
> 
> The easy workaround would be to replace &spi_add_lock with a
> per-controller lock,

Either that or register the controller from spi-mux asynchronously
by way of a work item.


> There is also a check:
> 
> 	if (IS_ENABLED(CONFIG_SPI_DYNAMIC) &&
> 	    !device_is_registered(&ctlr->dev)) {
> 		return -NODEV;
> 	}
> 
> which catches the race that the controller (which is also the device's
> parent) is about to go while we try to add a new device to it. Is this a
> real problem? (The spi_unregister_controller() function unregisters all
> children which might race with adding a new child without locking. This
> check was implemented by Lukas Wunner (on Cc:) in commit ddf75be47ca7.)
> Doesn't all children of a given device are unregistered automatically by
> the device core somehow when said device is unregistered? (I checked,
> but didn't find anything.)

No, the device core makes no such guarantee.

Thanks,

Lukas

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

[Mark Brown]

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

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.  We are only concerned about collisions on the same
bus, we don't care about other buses, so we don't need a global
lock and if the lock is per controller then we should't have any
issue adding a new bus in the middle of adding a device on
another bus.  Does the below totally untested patch work for you?

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index aea037c65985..412a10586233 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -478,12 +478,6 @@ static LIST_HEAD(spi_controller_list);
  */
 static DEFINE_MUTEX(board_lock);
 
-/*
- * Prevents addition of devices with same chip select and
- * addition of devices below an unregistering controller.
- */
-static DEFINE_MUTEX(spi_add_lock);
-
 /**
  * spi_alloc_device - Allocate a new SPI device
  * @ctlr: Controller to which device is connected
@@ -636,9 +630,9 @@ int spi_add_device(struct spi_device *spi)
 	 * chipselect **BEFORE** we call setup(), else we'll trash
 	 * its configuration.  Lock against concurrent add() calls.
 	 */
-	mutex_lock(&spi_add_lock);
+	mutex_lock(&ctlr->add_lock);
 	status = __spi_add_device(spi);
-	mutex_unlock(&spi_add_lock);
+	mutex_unlock(&ctlr->add_lock);
 	return status;
 }
 EXPORT_SYMBOL_GPL(spi_add_device);
@@ -658,7 +652,7 @@ static int spi_add_device_locked(struct spi_device *spi)
 	/* Set the bus ID string */
 	spi_dev_set_name(spi);
 
-	WARN_ON(!mutex_is_locked(&spi_add_lock));
+	WARN_ON(!mutex_is_locked(&ctlr->add_lock));
 	return __spi_add_device(spi);
 }
 
@@ -2830,6 +2824,7 @@ int spi_register_controller(struct spi_controller *ctlr)
 	spin_lock_init(&ctlr->bus_lock_spinlock);
 	mutex_init(&ctlr->bus_lock_mutex);
 	mutex_init(&ctlr->io_mutex);
+	mutex_init(&ctlr->add_lock);
 	ctlr->bus_lock_flag = 0;
 	init_completion(&ctlr->xfer_completion);
 	if (!ctlr->max_dma_len)
@@ -2966,7 +2961,7 @@ void spi_unregister_controller(struct spi_controller *ctlr)
 
 	/* Prevent addition of new devices, unregister existing ones */
 	if (IS_ENABLED(CONFIG_SPI_DYNAMIC))
-		mutex_lock(&spi_add_lock);
+		mutex_lock(&ctlr->add_lock);
 
 	device_for_each_child(&ctlr->dev, NULL, __unregister);
 
@@ -2997,7 +2992,7 @@ void spi_unregister_controller(struct spi_controller *ctlr)
 	mutex_unlock(&board_lock);
 
 	if (IS_ENABLED(CONFIG_SPI_DYNAMIC))
-		mutex_unlock(&spi_add_lock);
+		mutex_unlock(&ctlr->add_lock);
 }
 EXPORT_SYMBOL_GPL(spi_unregister_controller);
 
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 8371bca13729..6b0b686f6f90 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -531,6 +531,9 @@ struct spi_controller {
 	/* I/O mutex */
 	struct mutex		io_mutex;
 
+	/* Used to avoid adding the same CS twice */
+	struct mutex		add_lock;
+
 	/* lock and mutex for SPI bus locking */
 	spinlock_t		bus_lock_spinlock;
 	struct mutex		bus_lock_mutex;

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

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

[Uwe Kleine-König]

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

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()?

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?

Anyhow, will try the suggested patch next.

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

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

[Greg Kroah-Hartman]

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

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

[Uwe Kleine-König]

[-- 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 --]