[v2] spi: Don't have controller clean up spi device before driver unbind
diff mbox series

Message ID 20210505164734.175546-1-saravanak@google.com
State Accepted
Commit 27e7db56cf3dffd302bd7ddfacb1d405cf671a2a
Headers show
Series
  • [v2] spi: Don't have controller clean up spi device before driver unbind
Related show

Commit Message

Saravana Kannan May 5, 2021, 4:47 p.m. UTC
When a spi device is unregistered and triggers a driver unbind, the
driver might need to access the spi device. So, don't have the
controller clean up the spi device before the driver is unbound. Clean
up the spi device after the driver is unbound.

Fixes: c7299fea6769 ("spi: Fix spi device unregister flow")
Reported-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---

v1->v2:
- Made the clean up more symmetric. 

Lukas,

Can you test this one your end to make sure you don't have issues
anymore?

Thanks,
Saravana

 drivers/spi/spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko May 5, 2021, 5:53 p.m. UTC | #1
On Wed, May 5, 2021 at 8:26 PM Saravana Kannan <saravanak@google.com> wrote:
>
> When a spi device is unregistered and triggers a driver unbind, the
> driver might need to access the spi device. So, don't have the
> controller clean up the spi device before the driver is unbound. Clean
> up the spi device after the driver is unbound.
>
> Fixes: c7299fea6769 ("spi: Fix spi device unregister flow")
> Reported-by: Lukas Wunner <lukas@wunner.de>

And
Suggested-by: Lukas ...

> Signed-off-by: Saravana Kannan <saravanak@google.com>

...

> Can you test this one your end to make sure you don't have issues
> anymore?

Do you need a test on my setup?

...

> +       device_del(&spi->dev);
> +       spi_cleanup(spi);
> +       put_device(&spi->dev);

This block deserves a comment in the code.
Mark Brown May 11, 2021, 3:57 p.m. UTC | #2
On Wed, May 05, 2021 at 08:53:14PM +0300, Andy Shevchenko wrote:
> On Wed, May 5, 2021 at 8:26 PM Saravana Kannan <saravanak@google.com> wrote:

> > Can you test this one your end to make sure you don't have issues
> > anymore?

> Do you need a test on my setup?

It wouldn't hurt.
Andy Shevchenko May 11, 2021, 8:30 p.m. UTC | #3
On Tue, May 11, 2021 at 6:58 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, May 05, 2021 at 08:53:14PM +0300, Andy Shevchenko wrote:
> > On Wed, May 5, 2021 at 8:26 PM Saravana Kannan <saravanak@google.com> wrote:
>
> > > Can you test this one your end to make sure you don't have issues
> > > anymore?
>
> > Do you need a test on my setup?
>
> It wouldn't hurt.

Okay, I have reverted first the "spi: Fix spi device unregister flow"
to be sure I can reproduce the lockdep warning, indeed, it's there.

After applying the above mentioned patch it's gone.

On top I applied this ("spi: Don't have controller clean up spi device
before driver unbind") patch to see if there is any changes,
nope, seems everything is fine.

FWIW,
Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Mark Brown May 14, 2021, 3:22 p.m. UTC | #4
On Wed, 5 May 2021 09:47:34 -0700, Saravana Kannan wrote:
> When a spi device is unregistered and triggers a driver unbind, the
> driver might need to access the spi device. So, don't have the
> controller clean up the spi device before the driver is unbound. Clean
> up the spi device after the driver is unbound.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: Don't have controller clean up spi device before driver unbind
      commit: 27e7db56cf3dffd302bd7ddfacb1d405cf671a2a

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
Lukas Wunner May 21, 2021, 5:09 a.m. UTC | #5
On Wed, May 05, 2021 at 09:47:34AM -0700, Saravana Kannan wrote:
> When a spi device is unregistered and triggers a driver unbind, the
> driver might need to access the spi device. So, don't have the
> controller clean up the spi device before the driver is unbound. Clean
> up the spi device after the driver is unbound.
> 
> Fixes: c7299fea6769 ("spi: Fix spi device unregister flow")
> Reported-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> 
> v1->v2:
> - Made the clean up more symmetric. 
> 
> Lukas,
> 
> Can you test this one your end to make sure you don't have issues
> anymore?

Tested-by: Lukas Wunner <lukas@wunner.de>

My apologies for the delay, yesterday I was finally able to set up a RasPi
in my home office and test the patch.  I'm not seeing any issues when
unloading/reloading the SPI controller module, so this LGTM.

Thanks,

Lukas

Patch
diff mbox series

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 2350d131871b..f23e288e6498 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -714,8 +714,6 @@  void spi_unregister_device(struct spi_device *spi)
 	if (!spi)
 		return;
 
-	spi_cleanup(spi);
-
 	if (spi->dev.of_node) {
 		of_node_clear_flag(spi->dev.of_node, OF_POPULATED);
 		of_node_put(spi->dev.of_node);
@@ -723,7 +721,9 @@  void spi_unregister_device(struct spi_device *spi)
 	if (ACPI_COMPANION(&spi->dev))
 		acpi_device_clear_enumerated(ACPI_COMPANION(&spi->dev));
 	device_remove_software_node(&spi->dev);
-	device_unregister(&spi->dev);
+	device_del(&spi->dev);
+	spi_cleanup(spi);
+	put_device(&spi->dev);
 }
 EXPORT_SYMBOL_GPL(spi_unregister_device);