| Message ID | 20211013133710.2679703-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] spi: Make spi_add_lock a per-controller lock | expand |
On Wed, Oct 13, 2021 at 03:37:09PM +0200, Uwe Kleine-König wrote: > The spi_add_lock that is removed with this change was held when > spi_add_device() called device_add() (via __spi_add_device()). > > In the case where the added device is an spi-mux calling device_add() > might result in calling the spi-mux's probe function which adds another > controller and for that spi_add_device() might be called. This results > in a dead-lock. > > To circumvent this deadlock replace the global spi_add_lock with a lock > per controller. > > The biggest part of this patch was authored by Mark Brown. I'll go ahead with my copy of this (partly as I've already got it ready queued).
Hello Mark, On Wed, Oct 13, 2021 at 04:21:37PM +0100, Mark Brown wrote: > On Wed, Oct 13, 2021 at 03:37:09PM +0200, Uwe Kleine-König wrote: > > The spi_add_lock that is removed with this change was held when > > spi_add_device() called device_add() (via __spi_add_device()). > > > > In the case where the added device is an spi-mux calling device_add() > > might result in calling the spi-mux's probe function which adds another > > controller and for that spi_add_device() might be called. This results > > in a dead-lock. > > > > To circumvent this deadlock replace the global spi_add_lock with a lock > > per controller. > > > > The biggest part of this patch was authored by Mark Brown. > > I'll go ahead with my copy of this (partly as I've already got it > ready queued). That's what I expected, I just sent it that the base for patch 2 is properly available e.g. for the autobuilders. Did you modify your patch since you sent it? If you resend or apply to your tree I can rebase patch 2 on top of it. Best regards Uwe
On Wed, Oct 13, 2021 at 06:10:33PM +0200, Uwe Kleine-König wrote: > On Wed, Oct 13, 2021 at 04:21:37PM +0100, Mark Brown wrote: > > I'll go ahead with my copy of this (partly as I've already got it > > ready queued). > That's what I expected, I just sent it that the base for patch 2 is > properly available e.g. for the autobuilders. Did you modify your patch > since you sent it? If you resend or apply to your tree I can rebase > patch 2 on top of it. It's rebased (actually the public post was rebased). I'll let you know if yours fails.
On Wed, 13 Oct 2021 15:37:09 +0200, Uwe Kleine-König wrote: > The spi_add_lock that is removed with this change was held when > spi_add_device() called device_add() (via __spi_add_device()). > > In the case where the added device is an spi-mux calling device_add() > might result in calling the spi-mux's probe function which adds another > controller and for that spi_add_device() might be called. This results > in a dead-lock. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [2/2] spi-mux: Fix false-positive lockdep splats commit: 16a8e2fbb2d49111004efc1c7342e083eafabeb0 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 50591de16e0f..193e3264f7eb 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -477,12 +477,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 @@ -635,9 +629,9 @@ static int spi_add_device(struct spi_device *spi) /* Set the bus ID string */ spi_dev_set_name(spi); - 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; } @@ -656,7 +650,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); } @@ -2909,6 +2903,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) @@ -3045,7 +3040,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); @@ -3076,7 +3071,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 29e21d49aafc..5b61abe679f1 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -349,6 +349,7 @@ extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 ch * @max_message_size: function that returns the max message size for * a &spi_device; may be %NULL, so the default %SIZE_MAX will be used. * @io_mutex: mutex for physical bus access + * @add_lock: mutex to avoid adding two devices on the same CS * @bus_lock_spinlock: spinlock for SPI bus locking * @bus_lock_mutex: mutex for exclusion of multiple callers * @bus_lock_flag: indicates that the SPI bus is locked for exclusive use @@ -527,6 +528,9 @@ struct spi_controller { /* I/O mutex */ struct mutex io_mutex; + /* Used to avoid adding two devices on the same CS line */ + struct mutex add_lock; + /* lock and mutex for SPI bus locking */ spinlock_t bus_lock_spinlock; struct mutex bus_lock_mutex;
The spi_add_lock that is removed with this change was held when spi_add_device() called device_add() (via __spi_add_device()). In the case where the added device is an spi-mux calling device_add() might result in calling the spi-mux's probe function which adds another controller and for that spi_add_device() might be called. This results in a dead-lock. To circumvent this deadlock replace the global spi_add_lock with a lock per controller. The biggest part of this patch was authored by Mark Brown. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/spi/spi.c | 17 ++++++----------- include/linux/spi/spi.h | 4 ++++ 2 files changed, 10 insertions(+), 11 deletions(-) base-commit: da21fde0fdb393c2fbe0ae0735cc826cd55fd46f