| Message ID | 834c446b1cf3284d2660f1bee1ebe3e737cd02a9.1590408496.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 32e5b57232c0411e7dea96625c415510430ac079 |
| Headers | show |
| Series | Intel SPI unbind fixes | expand |
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 >
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
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 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
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
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 >
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
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
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
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
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);
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(-)