[PATCH v1 0/2] spi: pxa2xx: Fix module reloading failure

[PATCH v1 0/2] spi: pxa2xx: Fix module reloading failure

[Andy Shevchenko]

After splitting platform and reworking PCI drivers the module
reloading became broken as reported by Hao. Here is the patch
to fix that along with another one that fixes PM runtime workflow
differences between the busses (PCI vs. platform).

This is material for v6.11 as the initial work landed there and we still
have time to fix it before the release.

Andy Shevchenko (2):
  spi: pxa2xx: Do not override dev->platform_data on probe
  spi: pxa2xx: Move PM runtime handling to the glue drivers

 drivers/spi/spi-pxa2xx-pci.c      | 15 ++++++++++++++-
 drivers/spi/spi-pxa2xx-platform.c | 26 +++++++++++++++++++++-----
 drivers/spi/spi-pxa2xx.c          | 20 +++-----------------
 drivers/spi/spi-pxa2xx.h          |  3 ++-
 4 files changed, 40 insertions(+), 24 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/2] spi: pxa2xx: Do not override dev->platform_data on probe

[Andy Shevchenko]

The platform_data field may be supplied by legacy board code.
In other cases we override it, and module remove and probe cycle
will crash the kernel since it will carry a stale pointer.

Fix this by supplying a third argument to the pxa2xx_spi_probe()
and avoid overriding dev->platform_data.

Reported-by: Hao Ma <hao.ma@intel.com>
Fixes: cc160697a576 ("spi: pxa2xx: Convert PCI driver to use spi-pxa2xx code directly")
Fixes: 3d8f037fbcab ("spi: pxa2xx: Move platform driver to a separate file")
Fixes: 20ade9b9771c ("spi: pxa2xx: Extract pxa2xx_spi_platform_*() callbacks")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx-pci.c      | 2 +-
 drivers/spi/spi-pxa2xx-platform.c | 6 ++----
 drivers/spi/spi-pxa2xx.c          | 5 ++---
 drivers/spi/spi-pxa2xx.h          | 3 ++-
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index 616d032f1a89..c98bb214b6ae 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -297,7 +297,7 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
 		return ret;
 	ssp->irq = pci_irq_vector(dev, 0);
 
-	return pxa2xx_spi_probe(&dev->dev, ssp);
+	return pxa2xx_spi_probe(&dev->dev, ssp, pdata);
 }
 
 static void pxa2xx_spi_pci_remove(struct pci_dev *dev)
diff --git a/drivers/spi/spi-pxa2xx-platform.c b/drivers/spi/spi-pxa2xx-platform.c
index 98a8ceb7db6f..f9504cddc7ba 100644
--- a/drivers/spi/spi-pxa2xx-platform.c
+++ b/drivers/spi/spi-pxa2xx-platform.c
@@ -63,7 +63,7 @@ static struct ssp_device *pxa2xx_spi_ssp_request(struct platform_device *pdev)
 
 	ssp = pxa_ssp_request(pdev->id, pdev->name);
 	if (!ssp)
-		return ssp;
+		return NULL;
 
 	status = devm_add_action_or_reset(&pdev->dev, pxa2xx_spi_ssp_release, ssp);
 	if (status)
@@ -148,8 +148,6 @@ static int pxa2xx_spi_platform_probe(struct platform_device *pdev)
 		platform_info = pxa2xx_spi_init_pdata(pdev);
 		if (IS_ERR(platform_info))
 			return dev_err_probe(dev, PTR_ERR(platform_info), "missing platform data\n");
-
-		dev->platform_data = platform_info;
 	}
 
 	ssp = pxa2xx_spi_ssp_request(pdev);
@@ -158,7 +156,7 @@ static int pxa2xx_spi_platform_probe(struct platform_device *pdev)
 	if (!ssp)
 		ssp = &platform_info->ssp;
 
-	return pxa2xx_spi_probe(dev, ssp);
+	return pxa2xx_spi_probe(dev, ssp, platform_info);
 }
 
 static void pxa2xx_spi_platform_remove(struct platform_device *pdev)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 16b96eb176cd..e3a95adc5279 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1277,16 +1277,15 @@ static size_t pxa2xx_spi_max_dma_transfer_size(struct spi_device *spi)
 	return MAX_DMA_LEN;
 }
 
-int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp)
+int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
+		     struct pxa2xx_spi_controller *platform_info)
 {
-	struct pxa2xx_spi_controller *platform_info;
 	struct spi_controller *controller;
 	struct driver_data *drv_data;
 	const struct lpss_config *config;
 	int status;
 	u32 tmp;
 
-	platform_info = dev_get_platdata(dev);
 	if (platform_info->is_target)
 		controller = devm_spi_alloc_target(dev, sizeof(*drv_data));
 	else
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index a470d3d634d3..447be0369384 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -132,7 +132,8 @@ extern void pxa2xx_spi_dma_stop(struct driver_data *drv_data);
 extern int pxa2xx_spi_dma_setup(struct driver_data *drv_data);
 extern void pxa2xx_spi_dma_release(struct driver_data *drv_data);
 
-int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp);
+int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
+		     struct pxa2xx_spi_controller *platform_info);
 void pxa2xx_spi_remove(struct device *dev);
 
 extern const struct dev_pm_ops pxa2xx_spi_pm_ops;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 2/2] spi: pxa2xx: Move PM runtime handling to the glue drivers

[Andy Shevchenko]

PCI and platform buses have different defaults for runtime PM.
In particular PCI probe is assumed to be called when PM runtime
is enabled by the PCI core. In this case if we try enable it again
the PM runtime complaints with

    pxa2xx_spi_pci 0000:00:07.0: Unbalanced pm_runtime_enable!

Fix this by moving PM runtime handling from the SPI PXA2xx core
to the glue drivers.

Fixes: cc160697a576 ("spi: pxa2xx: Convert PCI driver to use spi-pxa2xx code directly")
Fixes: 3d8f037fbcab ("spi: pxa2xx: Move platform driver to a separate file")
Fixes: 20ade9b9771c ("spi: pxa2xx: Extract pxa2xx_spi_platform_*() callbacks")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx-pci.c      | 15 ++++++++++++++-
 drivers/spi/spi-pxa2xx-platform.c | 22 ++++++++++++++++++++--
 drivers/spi/spi-pxa2xx.c          | 15 +--------------
 3 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index c98bb214b6ae..cc8dcf782399 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/sprintf.h>
 #include <linux/string.h>
 #include <linux/types.h>
@@ -297,11 +298,23 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
 		return ret;
 	ssp->irq = pci_irq_vector(dev, 0);
 
-	return pxa2xx_spi_probe(&dev->dev, ssp, pdata);
+	ret = pxa2xx_spi_probe(&dev->dev, ssp, pdata);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_autosuspend_delay(&dev->dev, 50);
+	pm_runtime_use_autosuspend(&dev->dev);
+	pm_runtime_put_autosuspend(&dev->dev);
+	pm_runtime_allow(&dev->dev);
+
+	return 0;
 }
 
 static void pxa2xx_spi_pci_remove(struct pci_dev *dev)
 {
+	pm_runtime_forbid(&dev->dev);
+	pm_runtime_get_noresume(&dev->dev);
+
 	pxa2xx_spi_remove(&dev->dev);
 }
 
diff --git a/drivers/spi/spi-pxa2xx-platform.c b/drivers/spi/spi-pxa2xx-platform.c
index f9504cddc7ba..595af9fa4e0f 100644
--- a/drivers/spi/spi-pxa2xx-platform.c
+++ b/drivers/spi/spi-pxa2xx-platform.c
@@ -7,6 +7,7 @@
 #include <linux/init.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/types.h>
 
@@ -142,6 +143,7 @@ static int pxa2xx_spi_platform_probe(struct platform_device *pdev)
 	struct pxa2xx_spi_controller *platform_info;
 	struct device *dev = &pdev->dev;
 	struct ssp_device *ssp;
+	int ret;
 
 	platform_info = dev_get_platdata(dev);
 	if (!platform_info) {
@@ -156,12 +158,28 @@ static int pxa2xx_spi_platform_probe(struct platform_device *pdev)
 	if (!ssp)
 		ssp = &platform_info->ssp;
 
-	return pxa2xx_spi_probe(dev, ssp, platform_info);
+	pm_runtime_set_autosuspend_delay(dev, 50);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	ret = pxa2xx_spi_probe(dev, ssp, platform_info);
+	if (ret)
+		pm_runtime_disable(dev);
+
+	return ret;
 }
 
 static void pxa2xx_spi_platform_remove(struct platform_device *pdev)
 {
-	pxa2xx_spi_remove(&pdev->dev);
+	struct device *dev = &pdev->dev;
+
+	pm_runtime_get_sync(dev);
+
+	pxa2xx_spi_remove(dev);
+
+	pm_runtime_put_noidle(dev);
+	pm_runtime_disable(dev);
 }
 
 static const struct acpi_device_id pxa2xx_spi_acpi_match[] = {
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index e3a95adc5279..bf1f34b0ffc8 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1449,24 +1449,16 @@ int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
 		}
 	}
 
-	pm_runtime_set_autosuspend_delay(dev, 50);
-	pm_runtime_use_autosuspend(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-
 	/* Register with the SPI framework */
 	dev_set_drvdata(dev, drv_data);
 	status = spi_register_controller(controller);
 	if (status) {
 		dev_err_probe(dev, status, "problem registering SPI controller\n");
-		goto out_error_pm_runtime_enabled;
+		goto out_error_clock_enabled;
 	}
 
 	return status;
 
-out_error_pm_runtime_enabled:
-	pm_runtime_disable(dev);
-
 out_error_clock_enabled:
 	clk_disable_unprepare(ssp->clk);
 
@@ -1483,8 +1475,6 @@ void pxa2xx_spi_remove(struct device *dev)
 	struct driver_data *drv_data = dev_get_drvdata(dev);
 	struct ssp_device *ssp = drv_data->ssp;
 
-	pm_runtime_get_sync(dev);
-
 	spi_unregister_controller(drv_data->controller);
 
 	/* Disable the SSP at the peripheral and SOC level */
@@ -1495,9 +1485,6 @@ void pxa2xx_spi_remove(struct device *dev)
 	if (drv_data->controller_info->enable_dma)
 		pxa2xx_spi_dma_release(drv_data);
 
-	pm_runtime_put_noidle(dev);
-	pm_runtime_disable(dev);
-
 	/* Release IRQ */
 	free_irq(ssp->irq, drv_data);
 }
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 0/2] spi: pxa2xx: Fix module reloading failure

[Mark Brown]

On Thu, 22 Aug 2024 14:30:52 +0300, Andy Shevchenko wrote:
> After splitting platform and reworking PCI drivers the module
> reloading became broken as reported by Hao. Here is the patch
> to fix that along with another one that fixes PM runtime workflow
> differences between the busses (PCI vs. platform).
> 
> This is material for v6.11 as the initial work landed there and we still
> have time to fix it before the release.
> 
> [...]

Applied to

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

Thanks!

[1/2] spi: pxa2xx: Do not override dev->platform_data on probe
      commit: 9a8fc292dd93b93db30e01c94c0da4c944852f28
[2/2] spi: pxa2xx: Move PM runtime handling to the glue drivers
      commit: e17465f78eb92ebb4be17e35d6c0584406f643a0

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 v1 1/2] spi: pxa2xx: Do not override dev->platform_data on probe

[Ma, Hao]

Hi,Andriy,

I merged following patches and tested it works.

      [PATCH v1 1/2] spi: pxa2xx: Do not override dev->platform_data on probe
      [PATCH v1 2/2] spi: pxa2xx: Move PM runtime handling to the glue drivers


Tested-by: Hao Ma <hao.ma@intel.com >



Best Regards!
==========================================

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Thursday, August 22, 2024 7:31 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Mark Brown <broonie@kernel.org>; Linus Walleij <linus.walleij@linaro.org>;
> linux-arm-kernel@lists.infradead.org; linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Daniel Mack <daniel@zonque.org>; Haojian Zhuang <haojian.zhuang@gmail.com>; Robert Jarzmik <robert.jarzmik@free.fr>; Ma, Hao
> <hao.ma@intel.com>
> Subject: [PATCH v1 1/2] spi: pxa2xx: Do not override dev->platform_data on probe
> 
> The platform_data field may be supplied by legacy board code.
> In other cases we override it, and module remove and probe cycle will crash the kernel since it will carry a stale pointer.
> 
> Fix this by supplying a third argument to the pxa2xx_spi_probe() and avoid overriding dev->platform_data.
> 
> Reported-by: Hao Ma <hao.ma@intel.com>
> Fixes: cc160697a576 ("spi: pxa2xx: Convert PCI driver to use spi-pxa2xx code directly")
> Fixes: 3d8f037fbcab ("spi: pxa2xx: Move platform driver to a separate file")
> Fixes: 20ade9b9771c ("spi: pxa2xx: Extract pxa2xx_spi_platform_*() callbacks")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/spi/spi-pxa2xx-pci.c      | 2 +-
>  drivers/spi/spi-pxa2xx-platform.c | 6 ++----
>  drivers/spi/spi-pxa2xx.c          | 5 ++---
>  drivers/spi/spi-pxa2xx.h          | 3 ++-
>  4 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c index 616d032f1a89..c98bb214b6ae 100644
> --- a/drivers/spi/spi-pxa2xx-pci.c
> +++ b/drivers/spi/spi-pxa2xx-pci.c
> @@ -297,7 +297,7 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
>  		return ret;
>  	ssp->irq = pci_irq_vector(dev, 0);
> 
> -	return pxa2xx_spi_probe(&dev->dev, ssp);
> +	return pxa2xx_spi_probe(&dev->dev, ssp, pdata);
>  }
> 
>  static void pxa2xx_spi_pci_remove(struct pci_dev *dev) diff --git a/drivers/spi/spi-pxa2xx-platform.c b/drivers/spi/spi-pxa2xx-platform.c
> index 98a8ceb7db6f..f9504cddc7ba 100644
> --- a/drivers/spi/spi-pxa2xx-platform.c
> +++ b/drivers/spi/spi-pxa2xx-platform.c
> @@ -63,7 +63,7 @@ static struct ssp_device *pxa2xx_spi_ssp_request(struct platform_device *pdev)
> 
>  	ssp = pxa_ssp_request(pdev->id, pdev->name);
>  	if (!ssp)
> -		return ssp;
> +		return NULL;
> 
>  	status = devm_add_action_or_reset(&pdev->dev, pxa2xx_spi_ssp_release, ssp);
>  	if (status)
> @@ -148,8 +148,6 @@ static int pxa2xx_spi_platform_probe(struct platform_device *pdev)
>  		platform_info = pxa2xx_spi_init_pdata(pdev);
>  		if (IS_ERR(platform_info))
>  			return dev_err_probe(dev, PTR_ERR(platform_info), "missing platform data\n");
> -
> -		dev->platform_data = platform_info;
>  	}
> 
>  	ssp = pxa2xx_spi_ssp_request(pdev);
> @@ -158,7 +156,7 @@ static int pxa2xx_spi_platform_probe(struct platform_device *pdev)
>  	if (!ssp)
>  		ssp = &platform_info->ssp;
> 
> -	return pxa2xx_spi_probe(dev, ssp);
> +	return pxa2xx_spi_probe(dev, ssp, platform_info);
>  }
> 
>  static void pxa2xx_spi_platform_remove(struct platform_device *pdev) diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index
> 16b96eb176cd..e3a95adc5279 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -1277,16 +1277,15 @@ static size_t pxa2xx_spi_max_dma_transfer_size(struct spi_device *spi)
>  	return MAX_DMA_LEN;
>  }
> 
> -int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp)
> +int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
> +		     struct pxa2xx_spi_controller *platform_info)
>  {
> -	struct pxa2xx_spi_controller *platform_info;
>  	struct spi_controller *controller;
>  	struct driver_data *drv_data;
>  	const struct lpss_config *config;
>  	int status;
>  	u32 tmp;
> 
> -	platform_info = dev_get_platdata(dev);
>  	if (platform_info->is_target)
>  		controller = devm_spi_alloc_target(dev, sizeof(*drv_data));
>  	else
> diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h index a470d3d634d3..447be0369384 100644
> --- a/drivers/spi/spi-pxa2xx.h
> +++ b/drivers/spi/spi-pxa2xx.h
> @@ -132,7 +132,8 @@ extern void pxa2xx_spi_dma_stop(struct driver_data *drv_data);  extern int pxa2xx_spi_dma_setup(struct
> driver_data *drv_data);  extern void pxa2xx_spi_dma_release(struct driver_data *drv_data);
> 
> -int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp);
> +int pxa2xx_spi_probe(struct device *dev, struct ssp_device *ssp,
> +		     struct pxa2xx_spi_controller *platform_info);
>  void pxa2xx_spi_remove(struct device *dev);
> 
>  extern const struct dev_pm_ops pxa2xx_spi_pm_ops;
> --
> 2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/2] spi: pxa2xx: Do not override dev->platform_data on probe

[Andy Shevchenko]

On Fri, Aug 30, 2024 at 04:48:26AM +0000, Ma, Hao wrote:
> Hi,Andriy,
> 
> I merged following patches and tested it works.
> 
>       [PATCH v1 1/2] spi: pxa2xx: Do not override dev->platform_data on probe
>       [PATCH v1 2/2] spi: pxa2xx: Move PM runtime handling to the glue drivers
> 
> Tested-by: Hao Ma <hao.ma@intel.com >

Thank you for confirming that it works!

-- 
With Best Regards,
Andy Shevchenko