| Message ID | 20211013122628.1369702-1-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: Fix deadlock when adding SPI controllers on SPI buses | expand |
On Wed, Oct 13, 2021 at 01:26:28PM +0100, Mark Brown wrote: > Currently we have a global spi_add_lock which we take when adding new > devices so that we can check that we're not trying to reuse a chip > select that's already controlled. This means that if the SPI device is > itself a SPI controller and triggers the instantiation of further SPI > devices we trigger a deadlock as we try to register and instantiate > those devices while in the process of doing so for the parent controller > and hence already holding the global spi_add_lock. Since we only care > about concurrency within a single SPI bus move the lock to be per > controller, avoiding the deadlock. > > This can be easily triggered in the case of spi-mux. > > Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > drivers/spi/spi.c | 21 ++++++++++----------- > include/linux/spi/spi.h | 3 +++ > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 401f62f6f5b5..71d061132ada 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -518,12 +518,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 > @@ -676,9 +670,13 @@ static int spi_add_device(struct spi_device *spi) > /* Set the bus ID string */ > spi_dev_set_name(spi); > > - mutex_lock(&spi_add_lock); > + /* We need to make sure there's no other device with this > + * chipselect **BEFORE** we call setup(), else we'll trash > + * its configuration. Lock against concurrent add() calls. > + */ This comment is already there, it seems you duplicated it with your patch? Maybe a merge issue with 6bfb15f34dd8c8a073e03a31c485ef5774b127df? > + mutex_lock(&ctlr->add_lock); > status = __spi_add_device(spi); > - mutex_unlock(&spi_add_lock); > + mutex_unlock(&ctlr->add_lock); > return status; > } > > @@ -697,7 +695,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); > } > > @@ -2950,6 +2948,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) > @@ -3086,7 +3085,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); > > @@ -3117,7 +3116,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..eb7ac8a1e03c 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -527,6 +527,9 @@ struct spi_controller { > /* I/O mutex */ > struct mutex io_mutex; > > + /* Used to avoid adding the same CS twice */ > + struct mutex add_lock; > + Kernel-doc is missing for that one. (In the local patch I have for testing here, I wrote: * @add_lock: mutex to avoid adding two devices on the same CS .) Best regards Uwe
On Wed, Oct 13, 2021 at 03:00:05PM +0200, Uwe Kleine-König wrote: > > - mutex_lock(&spi_add_lock); > > + /* We need to make sure there's no other device with this > > + * chipselect **BEFORE** we call setup(), else we'll trash > > + * its configuration. Lock against concurrent add() calls. > > + */ > This comment is already there, it seems you duplicated it with your > patch? Maybe a merge issue with > 6bfb15f34dd8c8a073e03a31c485ef5774b127df? It's not there in -next which is what this is against but it looks like a mistake to remove it. It'll actually get applied on the fixes branch where that'll rebase out anyway.
On Wed, 13 Oct 2021 13:26:28 +0100, Mark Brown wrote: > Currently we have a global spi_add_lock which we take when adding new > devices so that we can check that we're not trying to reuse a chip > select that's already controlled. This means that if the SPI device is > itself a SPI controller and triggers the instantiation of further SPI > devices we trigger a deadlock as we try to register and instantiate > those devices while in the process of doing so for the parent controller > and hence already holding the global spi_add_lock. Since we only care > about concurrency within a single SPI bus move the lock to be per > controller, avoiding the deadlock. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: Fix deadlock when adding SPI controllers on SPI buses commit: 6098475d4cb48d821bdf453c61118c56e26294f0 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 401f62f6f5b5..71d061132ada 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -518,12 +518,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 @@ -676,9 +670,13 @@ static int spi_add_device(struct spi_device *spi) /* Set the bus ID string */ spi_dev_set_name(spi); - mutex_lock(&spi_add_lock); + /* We need to make sure there's no other device with this + * chipselect **BEFORE** we call setup(), else we'll trash + * its configuration. Lock against concurrent add() calls. + */ + mutex_lock(&ctlr->add_lock); status = __spi_add_device(spi); - mutex_unlock(&spi_add_lock); + mutex_unlock(&ctlr->add_lock); return status; } @@ -697,7 +695,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); } @@ -2950,6 +2948,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) @@ -3086,7 +3085,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); @@ -3117,7 +3116,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..eb7ac8a1e03c 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -527,6 +527,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;
Currently we have a global spi_add_lock which we take when adding new devices so that we can check that we're not trying to reuse a chip select that's already controlled. This means that if the SPI device is itself a SPI controller and triggers the instantiation of further SPI devices we trigger a deadlock as we try to register and instantiate those devices while in the process of doing so for the parent controller and hence already holding the global spi_add_lock. Since we only care about concurrency within a single SPI bus move the lock to be per controller, avoiding the deadlock. This can be easily triggered in the case of spi-mux. Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/spi/spi.c | 21 ++++++++++----------- include/linux/spi/spi.h | 3 +++ 2 files changed, 13 insertions(+), 11 deletions(-)