Deadlock in spi_add_device() -- device core problem

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

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