[PATCH 0/3] Intel SPI unbind fixes

[PATCH 0/3] Intel SPI unbind fixes

[Lukas Wunner]

Fix unbind ordering issues in the Designware and PXA2xx SPI controllers
built into Intel SoCs.

To test, you'll also need the following commit:
https://git.kernel.org/broonie/spi/c/84855678add8

This is compile-tested only, so please review and test thoroughly.

Pay particular attention to the IRQ handlers dw_spi_irq() and ssp_int():
Both drivers request the interrupt with IRQF_SHARED, so the IRQ handler
may run even though the SPI controller itself hasn't signalled an
interrupt.  The IRQ is requested before registering the controller and
freed after unregistering the controller.  Thus, the IRQ handler should
immediately bail out if the controller is not yet or no longer registered,
e.g. by checking in the controller's registers whether an interrupt is
actually pending.  The drivers may need to disable interrupts in the
controllers' registers upon unbinding and they need to make sure that
the registers are accessible until the IRQ is freed.

I don't have a datasheet for these SPI controllers but I hope someone
who is more familiar with them will be able to check for correctness
of the IRQ handling.

Lukas Wunner (3):
  spi: dw: Fix controller unregister order
  spi: pxa2xx: Fix controller unregister order
  spi: pxa2xx: Fix runtime PM ref imbalance on probe error

 drivers/spi/spi-dw.c     | 4 +++-
 drivers/spi/spi-pxa2xx.c | 5 +++--
 2 files changed, 6 insertions(+), 3 deletions(-)

-- 
2.25.0

[PATCH 1/3] spi: dw: Fix controller unregister order

[Lukas Wunner]

The Designware SPI driver uses devm_spi_register_controller() on bind.
As a consequence, on unbind, __device_release_driver() first invokes
dw_spi_remove_host() before unregistering the SPI controller via
devres_release_all().

This order is incorrect:  dw_spi_remove_host() shuts down the chip,
rendering the SPI bus inaccessible even though the SPI controller is
still registered.  When the SPI controller is subsequently unregistered,
it unbinds all its slave devices.  Because their drivers cannot access
the SPI bus, e.g. to quiesce interrupts, the slave devices may be left
in an improper state.

As a rule, devm_spi_register_controller() must not be used if the
->remove() hook performs teardown steps which shall be performed after
unregistering the controller and specifically after unbinding of slaves.

Fix by reverting to the non-devm variant of spi_register_controller().

An alternative approach would be to use device-managed functions for all
steps in dw_spi_remove_host(), e.g. by calling devm_add_action_or_reset()
on probe.  However that approach would add more LoC to the driver and
it wouldn't lend itself as well to backporting to stable.

Fixes: 04f421e7b0b1 ("spi: dw: use managed resources")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v3.14+
Cc: Baruch Siach <baruch@tkos.co.il>
---
 drivers/spi/spi-dw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 1edb8cdd11ee..9d6904d30104 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -520,7 +520,7 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 		}
 	}
 
-	ret = devm_spi_register_controller(dev, master);
+	ret = spi_register_controller(master);
 	if (ret) {
 		dev_err(&master->dev, "problem registering spi master\n");
 		goto err_dma_exit;
@@ -544,6 +544,8 @@ void dw_spi_remove_host(struct dw_spi *dws)
 {
 	dw_spi_debugfs_remove(dws);
 
+	spi_unregister_controller(dws->master);
+
 	if (dws->dma_ops && dws->dma_ops->dma_exit)
 		dws->dma_ops->dma_exit(dws);
 
-- 
2.25.0

[PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Lukas Wunner]

The PXA2xx SPI driver uses devm_spi_register_controller() on bind.
As a consequence, on unbind, __device_release_driver() first invokes
pxa2xx_spi_remove() before unregistering the SPI controller via
devres_release_all().

This order is incorrect:  pxa2xx_spi_remove() disables the chip,
rendering the SPI bus inaccessible even though the SPI controller is
still registered.  When the SPI controller is subsequently unregistered,
it unbinds all its slave devices.  Because their drivers cannot access
the SPI bus, e.g. to quiesce interrupts, the slave devices may be left
in an improper state.

As a rule, devm_spi_register_controller() must not be used if the
->remove() hook performs teardown steps which shall be performed after
unregistering the controller and specifically after unbinding of slaves.

Fix by reverting to the non-devm variant of spi_register_controller().

An alternative approach would be to use device-managed functions for all
steps in pxa2xx_spi_remove(), e.g. by calling devm_add_action_or_reset()
on probe.  However that approach would add more LoC to the driver and
it wouldn't lend itself as well to backporting to stable.

The improper use of devm_spi_register_controller() was introduced in 2013
by commit a807fcd090d6 ("spi: pxa2xx: use devm_spi_register_master()"),
but all earlier versions of the driver going back to 2006 were likewise
broken because they invoked spi_unregister_master() at the end of
pxa2xx_spi_remove(), rather than at the beginning.

Fixes: e0c9905e87ac ("[PATCH] SPI: add PXA2xx SSP SPI Driver")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206403#c1
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v2.6.17+
Cc: Tsuchiya Yuto <kitakar@gmail.com>
---
 drivers/spi/spi-pxa2xx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 20dcbd35611a..2ecf0d8cd9f7 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1885,7 +1885,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 
 	/* Register with the SPI framework */
 	platform_set_drvdata(pdev, drv_data);
-	status = devm_spi_register_controller(&pdev->dev, controller);
+	status = spi_register_controller(controller);
 	if (status != 0) {
 		dev_err(&pdev->dev, "problem registering spi controller\n");
 		goto out_error_pm_runtime_enabled;
@@ -1917,6 +1917,8 @@ static int pxa2xx_spi_remove(struct platform_device *pdev)
 
 	pm_runtime_get_sync(&pdev->dev);
 
+	spi_unregister_controller(drv_data->controller);
+
 	/* Disable the SSP at the peripheral and SOC level */
 	pxa2xx_spi_write(drv_data, SSCR0, 0);
 	clk_disable_unprepare(ssp->clk);
-- 
2.25.0

[PATCH 3/3] spi: pxa2xx: Fix runtime PM ref imbalance on probe error

[Lukas Wunner]

The PXA2xx SPI driver releases a runtime PM ref in the probe error path
even though it hasn't acquired a ref earlier.

Apparently commit e2b714afee32 ("spi: pxa2xx: Disable runtime PM if
controller registration fails") sought to copy-paste the invocation of
pm_runtime_disable() from pxa2xx_spi_remove(), but erroneously copied
the call to pm_runtime_put_noidle() as well.  Drop it.

Fixes: e2b714afee32 ("spi: pxa2xx: Disable runtime PM if controller registration fails")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v4.17+
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/spi/spi-pxa2xx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 2ecf0d8cd9f7..6721910e5f2a 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1894,7 +1894,6 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 	return status;
 
 out_error_pm_runtime_enabled:
-	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 out_error_clock_enabled:
-- 
2.25.0

Re: [PATCH 3/3] spi: pxa2xx: Fix runtime PM ref imbalance on probe error

[Andy Shevchenko]

On Mon, May 25, 2020 at 02:25:03PM +0200, Lukas Wunner wrote:
> The PXA2xx SPI driver releases a runtime PM ref in the probe error path
> even though it hasn't acquired a ref earlier.
> 
> Apparently commit e2b714afee32 ("spi: pxa2xx: Disable runtime PM if
> controller registration fails") sought to copy-paste the invocation of
> pm_runtime_disable() from pxa2xx_spi_remove(), but erroneously copied
> the call to pm_runtime_put_noidle() as well.  Drop it.

Looks like correct fix to me.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Fixes: e2b714afee32 ("spi: pxa2xx: Disable runtime PM if controller registration fails")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.17+
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
>  drivers/spi/spi-pxa2xx.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index 2ecf0d8cd9f7..6721910e5f2a 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -1894,7 +1894,6 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
>  	return status;
>  
>  out_error_pm_runtime_enabled:
> -	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
>  out_error_clock_enabled:
> -- 
> 2.25.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] spi: dw: Fix controller unregister order

[Andy Shevchenko]

On Mon, May 25, 2020 at 02:25:01PM +0200, Lukas Wunner wrote:
> The Designware SPI driver uses devm_spi_register_controller() on bind.
> As a consequence, on unbind, __device_release_driver() first invokes
> dw_spi_remove_host() before unregistering the SPI controller via
> devres_release_all().
> 
> This order is incorrect:  dw_spi_remove_host() shuts down the chip,
> rendering the SPI bus inaccessible even though the SPI controller is
> still registered.  When the SPI controller is subsequently unregistered,
> it unbinds all its slave devices.  Because their drivers cannot access
> the SPI bus, e.g. to quiesce interrupts, the slave devices may be left
> in an improper state.
> 
> As a rule, devm_spi_register_controller() must not be used if the
> ->remove() hook performs teardown steps which shall be performed after
> unregistering the controller and specifically after unbinding of slaves.
> 
> Fix by reverting to the non-devm variant of spi_register_controller().
> 
> An alternative approach would be to use device-managed functions for all
> steps in dw_spi_remove_host(), e.g. by calling devm_add_action_or_reset()
> on probe.  However that approach would add more LoC to the driver and
> it wouldn't lend itself as well to backporting to stable.

I think it's, unregistering controller first, proper way to quiescence it,
otherwise it can be a surprise transfer started while we are in removal stage.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Fixes: 04f421e7b0b1 ("spi: dw: use managed resources")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v3.14+
> Cc: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/spi/spi-dw.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index 1edb8cdd11ee..9d6904d30104 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -520,7 +520,7 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>  		}
>  	}
>  
> -	ret = devm_spi_register_controller(dev, master);
> +	ret = spi_register_controller(master);
>  	if (ret) {
>  		dev_err(&master->dev, "problem registering spi master\n");
>  		goto err_dma_exit;
> @@ -544,6 +544,8 @@ void dw_spi_remove_host(struct dw_spi *dws)
>  {
>  	dw_spi_debugfs_remove(dws);
>  
> +	spi_unregister_controller(dws->master);
> +
>  	if (dws->dma_ops && dws->dma_ops->dma_exit)
>  		dws->dma_ops->dma_exit(dws);
>  
> -- 
> 2.25.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Andy Shevchenko]

On Mon, May 25, 2020 at 02:25:02PM +0200, Lukas Wunner wrote:
> The PXA2xx SPI driver uses devm_spi_register_controller() on bind.
> As a consequence, on unbind, __device_release_driver() first invokes
> pxa2xx_spi_remove() before unregistering the SPI controller via
> devres_release_all().
> 
> This order is incorrect:  pxa2xx_spi_remove() disables the chip,
> rendering the SPI bus inaccessible even though the SPI controller is
> still registered.  When the SPI controller is subsequently unregistered,
> it unbinds all its slave devices.  Because their drivers cannot access
> the SPI bus, e.g. to quiesce interrupts, the slave devices may be left
> in an improper state.
> 
> As a rule, devm_spi_register_controller() must not be used if the
> ->remove() hook performs teardown steps which shall be performed after
> unregistering the controller and specifically after unbinding of slaves.
> 
> Fix by reverting to the non-devm variant of spi_register_controller().
> 
> An alternative approach would be to use device-managed functions for all
> steps in pxa2xx_spi_remove(), e.g. by calling devm_add_action_or_reset()
> on probe.  However that approach would add more LoC to the driver and
> it wouldn't lend itself as well to backporting to stable.

I think it's, unregistering controller first, proper way to quiescence it,
otherwise it can be a surprise transfer started while we are in removal stage.

Yes, there are still some bugs in state machine and remove ordering, but
perhaps they can be fixed separately.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Tsuchiya Yuto, I'm going to apply this series as preparatory to my WIP patch in
topic/spi/reload branch in my kernel tree on GitHub, so, it would be possible
to see if this + my patch fixes crashes on removal. Though, please test this
separately from my stuff to clarify if it fixes or not issue you have seen.

> 
> The improper use of devm_spi_register_controller() was introduced in 2013
> by commit a807fcd090d6 ("spi: pxa2xx: use devm_spi_register_master()"),
> but all earlier versions of the driver going back to 2006 were likewise
> broken because they invoked spi_unregister_master() at the end of
> pxa2xx_spi_remove(), rather than at the beginning.
> 
> Fixes: e0c9905e87ac ("[PATCH] SPI: add PXA2xx SSP SPI Driver")

I'm not sure it's a good idea to mix devm_*() with so old patch, though I
understand your point. Let's leave it up to Mark to decide what would be
correct commit reference here.

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206403#c1
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v2.6.17+
> Cc: Tsuchiya Yuto <kitakar@gmail.com>
> ---
>  drivers/spi/spi-pxa2xx.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index 20dcbd35611a..2ecf0d8cd9f7 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -1885,7 +1885,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
>  
>  	/* Register with the SPI framework */
>  	platform_set_drvdata(pdev, drv_data);
> -	status = devm_spi_register_controller(&pdev->dev, controller);
> +	status = spi_register_controller(controller);
>  	if (status != 0) {
>  		dev_err(&pdev->dev, "problem registering spi controller\n");
>  		goto out_error_pm_runtime_enabled;
> @@ -1917,6 +1917,8 @@ static int pxa2xx_spi_remove(struct platform_device *pdev)
>  
>  	pm_runtime_get_sync(&pdev->dev);
>  
> +	spi_unregister_controller(drv_data->controller);
> +
>  	/* Disable the SSP at the peripheral and SOC level */
>  	pxa2xx_spi_write(drv_data, SSCR0, 0);
>  	clk_disable_unprepare(ssp->clk);
> -- 
> 2.25.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] Intel SPI unbind fixes

[Andy Shevchenko]

On Mon, May 25, 2020 at 02:25:00PM +0200, Lukas Wunner wrote:
> Fix unbind ordering issues in the Designware and PXA2xx SPI controllers
> built into Intel SoCs.
> 
> To test, you'll also need the following commit:
> https://git.kernel.org/broonie/spi/c/84855678add8
> 
> This is compile-tested only, so please review and test thoroughly.
> 
> Pay particular attention to the IRQ handlers dw_spi_irq() and ssp_int():
> Both drivers request the interrupt with IRQF_SHARED, so the IRQ handler
> may run even though the SPI controller itself hasn't signalled an
> interrupt.  The IRQ is requested before registering the controller and
> freed after unregistering the controller.  Thus, the IRQ handler should
> immediately bail out if the controller is not yet or no longer registered,
> e.g. by checking in the controller's registers whether an interrupt is
> actually pending.  The drivers may need to disable interrupts in the
> controllers' registers upon unbinding and they need to make sure that
> the registers are accessible until the IRQ is freed.
> 
> I don't have a datasheet for these SPI controllers but I hope someone
> who is more familiar with them will be able to check for correctness
> of the IRQ handling.

Thank you for this!
I reviewed all three, but I have no means to reliably test it. From the
description and logic point of view this sounds correct. So, let's wait for
Tsuchiya Yuto to test and confirm that it's right way to go.

> 
> Lukas Wunner (3):
>   spi: dw: Fix controller unregister order
>   spi: pxa2xx: Fix controller unregister order
>   spi: pxa2xx: Fix runtime PM ref imbalance on probe error
> 
>  drivers/spi/spi-dw.c     | 4 +++-
>  drivers/spi/spi-pxa2xx.c | 5 +++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> -- 
> 2.25.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Lukas Wunner]

On Mon, May 25, 2020 at 04:21:43PM +0300, Andy Shevchenko wrote:
> Tsuchiya Yuto, I'm going to apply this series as preparatory to my
> WIP patch in topic/spi/reload branch in my kernel tree on GitHub,
> so, it would be possible to see if this + my patch fixes crashes
> on removal. Though, please test this separately from my stuff to
> clarify if it fixes or not issue you have seen.

You also need to cherry-pick commit 84855678add8 ("spi: Fix controller
unregister order") from spi/for-next onto your topic/spi/reload branch
for reloading to work correctly.

Alternatively, rebase your topic/spi/reload branch and redo the merge
from spi/for-next.  (You've merged spi/for-next into your branch on
May 14, but the commit was applied by Mark on May 20.)

Thanks,

Lukas

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Andy Shevchenko]

On Tue, May 26, 2020 at 09:39:13AM +0200, Lukas Wunner wrote:
> On Mon, May 25, 2020 at 04:21:43PM +0300, Andy Shevchenko wrote:
> > Tsuchiya Yuto, I'm going to apply this series as preparatory to my
> > WIP patch in topic/spi/reload branch in my kernel tree on GitHub,
> > so, it would be possible to see if this + my patch fixes crashes
> > on removal. Though, please test this separately from my stuff to
> > clarify if it fixes or not issue you have seen.
> 
> You also need to cherry-pick commit 84855678add8 ("spi: Fix controller
> unregister order") from spi/for-next onto your topic/spi/reload branch
> for reloading to work correctly.
> 
> Alternatively, rebase your topic/spi/reload branch and redo the merge
> from spi/for-next.  (You've merged spi/for-next into your branch on
> May 14, but the commit was applied by Mark on May 20.)

Ah, right. Will do it soon.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] spi: pxa2xx: Fix runtime PM ref imbalance on probe error

[Jarkko Nikula]

On 5/25/20 4:12 PM, Andy Shevchenko wrote:
> On Mon, May 25, 2020 at 02:25:03PM +0200, Lukas Wunner wrote:
>> The PXA2xx SPI driver releases a runtime PM ref in the probe error path
>> even though it hasn't acquired a ref earlier.
>>
>> Apparently commit e2b714afee32 ("spi: pxa2xx: Disable runtime PM if
>> controller registration fails") sought to copy-paste the invocation of
>> pm_runtime_disable() from pxa2xx_spi_remove(), but erroneously copied
>> the call to pm_runtime_put_noidle() as well.  Drop it.
> 
> Looks like correct fix to me.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
Yes, I think it was blind copy-paste from pxa2xx_spi_remove() where 
pm_runtime_put_noidle() is needed due the pm_runtime_get_sync() call but 
not in pxa2xx_spi_probe().

Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH 0/3] Intel SPI unbind fixes

[Mark Brown]

On Mon, 25 May 2020 14:25:00 +0200, Lukas Wunner wrote:
> Fix unbind ordering issues in the Designware and PXA2xx SPI controllers
> built into Intel SoCs.
> 
> To test, you'll also need the following commit:
> https://git.kernel.org/broonie/spi/c/84855678add8
> 
> This is compile-tested only, so please review and test thoroughly.
> 
> [...]

Applied to

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

Thanks!

[1/3] spi: dw: Fix controller unregister order
      commit: ca8b19d61e3fce5d2d7790cde27a0b57bcb3f341
[2/3] spi: pxa2xx: Fix controller unregister order
      commit: 32e5b57232c0411e7dea96625c415510430ac079
[3/3] spi: pxa2xx: Fix runtime PM ref imbalance on probe error
      commit: 65e318e17358a3fd4fcb5a69d89b14016dee2f06

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

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Tsuchiya Yuto]

I tried a kernel built with the prerequisite patch to this series + all
of patches in this series on top of v5.7-rc7 (with Arch Linux config
+ olddefconfig).

Current situations on 5.7-rc7 with Arch Linux config + olddefconfig
(without applying this series):
- I can reproduce the touch input crashing (surface3-spi) I mentioned
  in bugzilla [1] only after s2idle.
- all the other situations are the same as described in that bugzilla;
  I see NULL pointer dereference [2] after touch input crashing then try
  to unload only spi_pxa2xx_platform module.

So, the steps to test that I did with this series applied are:
1. go into s2idle then resume from s2idle
2. make a touch input then surface3-spi reports that "SPI transfer
   timed out" repeatedly and no longer responds to any touch input
3. try to unload only spi_pxa2xx_platform module and see if the NULL
   pointer dereference no longer occurs

and I can confirm that I no longer see the NULL pointer dereference.
Thanks!

On 5/26/20 5:22 PM, Andy Shevchenko wrote:
> On Tue, May 26, 2020 at 09:39:13AM +0200, Lukas Wunner wrote:
>> On Mon, May 25, 2020 at 04:21:43PM +0300, Andy Shevchenko wrote:
>>> Tsuchiya Yuto, I'm going to apply this series as preparatory to my
>>> WIP patch in topic/spi/reload branch in my kernel tree on GitHub,
>>> so, it would be possible to see if this + my patch fixes crashes
>>> on removal. Though, please test this separately from my stuff to
>>> clarify if it fixes or not issue you have seen.
>> You also need to cherry-pick commit 84855678add8 ("spi: Fix controller
>> unregister order") from spi/for-next onto your topic/spi/reload branch
>> for reloading to work correctly.
>>
>> Alternatively, rebase your topic/spi/reload branch and redo the merge
>> from spi/for-next.  (You've merged spi/for-next into your branch on
>> May 14, but the commit was applied by Mark on May 20.)
> Ah, right. Will do it soon.

I also built a kernel against your branch topic/spi/reload
(permalink: https://github.com/andy-shev/linux/tree/55cb78d5a752). The
result is the same as only applying this series; so, to fix the NULL pointer
dereference that I mentioned in bugzilla [2], only this series is required.

Also, I want to make sure that what you tried in that branch is fixing
the NULL pointer dereference on spi_pxa2xx_platform module removal when
touch input crashed, not fixing the touch input crashing itself?

[1] https://bugzilla.kernel.org/show_bug.cgi?id=206403
[2] https://bugzilla.kernel.org/show_bug.cgi?id=206403#c1

Thanks,

Tsuchiya Yuto

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Andy Shevchenko]

On Wed, May 27, 2020 at 09:09:17PM +0900, Tsuchiya Yuto wrote:
> I tried a kernel built with the prerequisite patch to this series + all
> of patches in this series on top of v5.7-rc7 (with Arch Linux config
> + olddefconfig).
> 
> Current situations on 5.7-rc7 with Arch Linux config + olddefconfig
> (without applying this series):
> - I can reproduce the touch input crashing (surface3-spi) I mentioned
>   in bugzilla [1] only after s2idle.
> - all the other situations are the same as described in that bugzilla;
>   I see NULL pointer dereference [2] after touch input crashing then try
>   to unload only spi_pxa2xx_platform module.
> 
> So, the steps to test that I did with this series applied are:
> 1. go into s2idle then resume from s2idle
> 2. make a touch input then surface3-spi reports that "SPI transfer
>    timed out" repeatedly and no longer responds to any touch input
> 3. try to unload only spi_pxa2xx_platform module and see if the NULL
>    pointer dereference no longer occurs
> 
> and I can confirm that I no longer see the NULL pointer dereference.
> Thanks!

Thank you very much for testing!

> On 5/26/20 5:22 PM, Andy Shevchenko wrote:
> > On Tue, May 26, 2020 at 09:39:13AM +0200, Lukas Wunner wrote:
> >> On Mon, May 25, 2020 at 04:21:43PM +0300, Andy Shevchenko wrote:
> >>> Tsuchiya Yuto, I'm going to apply this series as preparatory to my
> >>> WIP patch in topic/spi/reload branch in my kernel tree on GitHub,
> >>> so, it would be possible to see if this + my patch fixes crashes
> >>> on removal. Though, please test this separately from my stuff to
> >>> clarify if it fixes or not issue you have seen.
> >> You also need to cherry-pick commit 84855678add8 ("spi: Fix controller
> >> unregister order") from spi/for-next onto your topic/spi/reload branch
> >> for reloading to work correctly.
> >>
> >> Alternatively, rebase your topic/spi/reload branch and redo the merge
> >> from spi/for-next.  (You've merged spi/for-next into your branch on
> >> May 14, but the commit was applied by Mark on May 20.)
> > Ah, right. Will do it soon.
> 
> I also built a kernel against your branch topic/spi/reload
> (permalink: https://github.com/andy-shev/linux/tree/55cb78d5a752). The
> result is the same as only applying this series; so, to fix the NULL pointer
> dereference that I mentioned in bugzilla [2], only this series is required.
> 
> Also, I want to make sure that what you tried in that branch is fixing
> the NULL pointer dereference on spi_pxa2xx_platform module removal when
> touch input crashed, not fixing the touch input crashing itself?

Yes, my aim was to fix the SPI module reload issue. While the applied patch
from Lukas does a huge improvement, there are still issues with ordering (you
probably will never see them, though it's still possible based on the code).

So, as far as I understood, the touch still able to come into position where
it's not anymore responsive. Is it correct?

> [1] https://bugzilla.kernel.org/show_bug.cgi?id=206403
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=206403#c1

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Tsuchiya Yuto]

On 5/27/20 9:27 PM, Andy Shevchenko wrote:
> On Wed, May 27, 2020 at 09:09:17PM +0900, Tsuchiya Yuto wrote:
>> I tried a kernel built with the prerequisite patch to this series + all
>> of patches in this series on top of v5.7-rc7 (with Arch Linux config
>> + olddefconfig).
>>
>> Current situations on 5.7-rc7 with Arch Linux config + olddefconfig
>> (without applying this series):
>> - I can reproduce the touch input crashing (surface3-spi) I mentioned
>>   in bugzilla [1] only after s2idle.
>> - all the other situations are the same as described in that bugzilla;
>>   I see NULL pointer dereference [2] after touch input crashing then try
>>   to unload only spi_pxa2xx_platform module.
>>
>> So, the steps to test that I did with this series applied are:
>> 1. go into s2idle then resume from s2idle
>> 2. make a touch input then surface3-spi reports that "SPI transfer
>>    timed out" repeatedly and no longer responds to any touch input
>> 3. try to unload only spi_pxa2xx_platform module and see if the NULL
>>    pointer dereference no longer occurs
>>
>> and I can confirm that I no longer see the NULL pointer dereference.
>> Thanks!
>
> Thank you very much for testing!
>
>> On 5/26/20 5:22 PM, Andy Shevchenko wrote:
>>> On Tue, May 26, 2020 at 09:39:13AM +0200, Lukas Wunner wrote:
>>>> On Mon, May 25, 2020 at 04:21:43PM +0300, Andy Shevchenko wrote:
>>>>> Tsuchiya Yuto, I'm going to apply this series as preparatory to my
>>>>> WIP patch in topic/spi/reload branch in my kernel tree on GitHub,
>>>>> so, it would be possible to see if this + my patch fixes crashes
>>>>> on removal. Though, please test this separately from my stuff to
>>>>> clarify if it fixes or not issue you have seen.
>>>> You also need to cherry-pick commit 84855678add8 ("spi: Fix controller
>>>> unregister order") from spi/for-next onto your topic/spi/reload branch
>>>> for reloading to work correctly.
>>>>
>>>> Alternatively, rebase your topic/spi/reload branch and redo the merge
>>>> from spi/for-next.  (You've merged spi/for-next into your branch on
>>>> May 14, but the commit was applied by Mark on May 20.)
>>> Ah, right. Will do it soon.
>>
>> I also built a kernel against your branch topic/spi/reload
>> (permalink: https://github.com/andy-shev/linux/tree/55cb78d5a752). The
>> result is the same as only applying this series; so, to fix the NULL pointer
>> dereference that I mentioned in bugzilla [2], only this series is required.
>>
>> Also, I want to make sure that what you tried in that branch is fixing
>> the NULL pointer dereference on spi_pxa2xx_platform module removal when
>> touch input crashed, not fixing the touch input crashing itself?
>
> Yes, my aim was to fix the SPI module reload issue. While the applied patch
> from Lukas does a huge improvement, there are still issues with ordering (you
> probably will never see them, though it's still possible based on the code).
>
> So, as far as I understood, the touch still able to come into position where
> it's not anymore responsive. Is it correct?

Yes, the touch still able to come into non-working state after every s2idle,
but always can be resurrected by reloading spi_pxa2xx_platform.

What this series fixed is the following thing:
- without this series: reloading spi_pxa2xx_platform resurrects touch
  input with causing NULL pointer dereference (system still operational
  after this anyway)
- with this series: reloading spi_pxa2xx_platform resurrects touch input
  *without* causing NULL pointer dereference

Let me know if any further info is required.
>
>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=206403
>> [2] https://bugzilla.kernel.org/show_bug.cgi?id=206403#c1
>

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Tsuchiya Yuto]

Correction (obvious but just in case)

On 5/27/20 10:14 PM, Tsuchiya Yuto wrote:
>
> On 5/27/20 9:27 PM, Andy Shevchenko wrote:
>> On Wed, May 27, 2020 at 09:09:17PM +0900, Tsuchiya Yuto wrote:
>>> [...]
>>>
>>> I also built a kernel against your branch topic/spi/reload
>>> (permalink: https://github.com/andy-shev/linux/tree/55cb78d5a752). The
>>> result is the same as only applying this series; so, to fix the NULL pointer
>>> dereference that I mentioned in bugzilla [2], only this series is required.

*I also built a kernel from your branch topic/spi/reload

>>> Also, I want to make sure that what you tried in that branch is fixing
>>> the NULL pointer dereference on spi_pxa2xx_platform module removal when
>>> touch input crashed, not fixing the touch input crashing itself?
>>
>> Yes, my aim was to fix the SPI module reload issue. While the applied patch
>> from Lukas does a huge improvement, there are still issues with ordering (you
>> probably will never see them, though it's still possible based on the code).
>>
>> So, as far as I understood, the touch still able to come into position where
>> it's not anymore responsive. Is it correct?
>
> Yes, the touch still able to come into non-working state after every s2idle,
> but always can be resurrected by reloading spi_pxa2xx_platform.

This is true for both this series and branch topic/spi/reload

> What this series fixed is the following thing:
> - without this series: reloading spi_pxa2xx_platform resurrects touch
>   input with causing NULL pointer dereference (system still operational
>   after this anyway)
> - with this series: reloading spi_pxa2xx_platform resurrects touch input
>   *without* causing NULL pointer dereference
>
> Let me know if any further info is required.

*What this series (and branch topic/spi/reload) fixed is the following thing:

>>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=206403
>>> [2] https://bugzilla.kernel.org/show_bug.cgi?id=206403#c1

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Andy Shevchenko]

On Thu, May 28, 2020 at 10:03 AM Tsuchiya Yuto <kitakar@gmail.com> wrote:
> On 5/27/20 10:14 PM, Tsuchiya Yuto wrote:
> > On 5/27/20 9:27 PM, Andy Shevchenko wrote:
> >> On Wed, May 27, 2020 at 09:09:17PM +0900, Tsuchiya Yuto wrote:

...

> This is true for both this series and branch topic/spi/reload
>
> > What this series fixed is the following thing:
> > - without this series: reloading spi_pxa2xx_platform resurrects touch
> >   input with causing NULL pointer dereference (system still operational
> >   after this anyway)
> > - with this series: reloading spi_pxa2xx_platform resurrects touch input
> >   *without* causing NULL pointer dereference
> >
> > Let me know if any further info is required.

Thank you very much for testing, I will figure out what can be done
more there, but it's minor now.
For input and touchscreen I guess you may ask Dmitry (input subsystem
maintainer) and Benjamin (HID, but he might have an idea as well).

> *What this series (and branch topic/spi/reload) fixed is the following thing:
>
> >>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=206403
> >>> [2] https://bugzilla.kernel.org/show_bug.cgi?id=206403#c1


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Lukas Wunner]

On Thu, May 28, 2020 at 11:41:21AM +0300, Andy Shevchenko wrote:
> Thank you very much for testing, I will figure out what can be done
> more there, but it's minor now.
> For input and touchscreen I guess you may ask Dmitry (input subsystem
> maintainer) and Benjamin (HID, but he might have an idea as well).

This might not be an input issue, perhaps the spi-pxa2xx.c driver
cannot cope with s2idle on this particular platform.

E.g., pxa2xx_spi_suspend() zeroes the SSCR0 register.  It seems this
disables or resets the controller.  But pxa2xx_spi_resume() isn't
touching the register at all.  Maybe the register contains crap when
coming out of s2idle, so needs to be set to a sane value on resume?

Tsuchiya Yuto says that reloading the SPI controller driver makes
the touch driver work again, so I'd check what's done on ->remove()
and ->probe() both in the touch driver as well as in the SPI controller
driver that fixes the problem.  The SSCR0 register is zeroed on
->remove() and re-initialized on ->probe(), so that register may
indeed play a role.

Since the SPI controller seems to be on a PCI device, I'd also check
if that PCI device has trouble coming out of s2idle.  If its BAR
isn't accessible (MMIO reads return "all ones"), then the SPI controller
and consequently the touch controller won't be accessible as well.

Thanks,

Lukas

Re: [PATCH 2/3] spi: pxa2xx: Fix controller unregister order

[Tsuchiya Yuto]

On 5/28/20 6:31 PM, Lukas Wunner wrote:
> On Thu, May 28, 2020 at 11:41:21AM +0300, Andy Shevchenko wrote:
>> Thank you very much for testing, I will figure out what can be done
>> more there, but it's minor now.
>> For input and touchscreen I guess you may ask Dmitry (input subsystem
>> maintainer) and Benjamin (HID, but he might have an idea as well).
>
> This might not be an input issue, perhaps the spi-pxa2xx.c driver
> cannot cope with s2idle on this particular platform.
>
> E.g., pxa2xx_spi_suspend() zeroes the SSCR0 register.  It seems this
> disables or resets the controller.  But pxa2xx_spi_resume() isn't
> touching the register at all.  Maybe the register contains crap when
> coming out of s2idle, so needs to be set to a sane value on resume?

Thanks everyone. I later found that touch input (surface3_spi) crashing
is reproducible by just putting display off/on on recent kernels. So,
this is rather not related to s2idle. Also it seems that runtime_pm is
not related, too.

> Tsuchiya Yuto says that reloading the SPI controller driver makes
> the touch driver work again, so I'd check what's done on ->remove()
> and ->probe() both in the touch driver as well as in the SPI controller
> driver that fixes the problem.  The SSCR0 register is zeroed on
> ->remove() and re-initialized on ->probe(), so that register may
> indeed play a role.

I looked into what is done on probe()/remove() in the SPI controller
and it seems that release/setup DMA helps to get surface3_spi driver
working again with DMA mode.

I added details to that bugzilla [1], since this crashing itself is not
relevant to this series.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=206403#c4