linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] pci: Fix some section mismatches
@ 2023-10-01 17:02 Uwe Kleine-König
  2023-10-01 17:02 ` [PATCH 1/4] PCI: exynos: Don't put .remove callback in .exit.text section Uwe Kleine-König
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2023-10-01 17:02 UTC (permalink / raw)
  To: Jingoo Han, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Bjorn Helgaas, Krzysztof Kozlowski, Kukjin Kim, Siva Reddy Kallam,
	Surendranath Gurivireddy Balla, Xiaowei Song, Binghui Wang,
	Kishon Vijay Abraham I, Mauro Carvalho Chehab, Santosh Shilimkar,
	Murali Karicheri
  Cc: Rob Herring, Alim Akhtar, linux-pci, linux-arm-kernel,
	linux-samsung-soc, kernel

Hello,

modpost checks about section mismatches are about to get stronger, see
https://lore.kernel.org/linux-kbuild/20230930165204.2478282-1-u.kleine-koenig@pengutronix.de
.

With the above patch applied, enabling the exynos and kirin drivers as
modules result in a warning about their remove functions that is fixed
here. The keystone driver is a bit special as it can only be enabled
built-in and used __refdata on its driver struct. It also had a similar
issue for .probe fixed in the last patch.

IMHO all four patches qualify for backporting to stable.

Best regards
Uwe

Uwe Kleine-König (4):
  PCI: exynos: Don't put .remove callback in .exit.text section
  PCI: kirin: Don't put .remove callback in .exit.text section
  PCI: keystone: Don't put .remove callback in .exit.text section
  PCI: keystone: Don't put .probe callback in .init.text section

 drivers/pci/controller/dwc/pci-exynos.c   | 4 ++--
 drivers/pci/controller/dwc/pci-keystone.c | 8 ++++----
 drivers/pci/controller/dwc/pcie-kirin.c   | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

base-commit: 6465e260f48790807eef06b583b38ca9789b6072
-- 
2.40.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] PCI: exynos: Don't put .remove callback in .exit.text section
  2023-10-01 17:02 [PATCH 0/4] pci: Fix some section mismatches Uwe Kleine-König
@ 2023-10-01 17:02 ` Uwe Kleine-König
  2023-10-02  1:38   ` Alim Akhtar
  2023-10-01 17:02 ` [PATCH 2/4] PCI: kirin: " Uwe Kleine-König
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2023-10-01 17:02 UTC (permalink / raw)
  To: Jingoo Han, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Bjorn Helgaas, Krzysztof Kozlowski, Kukjin Kim, Siva Reddy Kallam,
	Surendranath Gurivireddy Balla
  Cc: Rob Herring, Alim Akhtar, linux-pci, linux-arm-kernel,
	linux-samsung-soc, kernel

With CONFIG_PCI_EXYNOS=y and exynos_pcie_remove() marked with __exit,
the function is discarded from the driver. In this case a bound device
can still get unbound, e.g via sysfs. Then no cleanup code is run
resulting in resource leaks or worse.

The right thing to do is do always have the remove callback available.
This fixes the following warning by modpost:

	WARNING: modpost: drivers/pci/controller/dwc/pci-exynos: section mismatch in reference: exynos_pcie_driver+0x8 (section: .data) -> exynos_pcie_remove (section: .exit.text)

(with ARCH=x86_64 W=1 allmodconfig).

Fixes: 340cba6092c2 ("pci: Add PCIe driver for Samsung Exynos")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-exynos.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index 6319082301d6..c6bede346932 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -375,7 +375,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int __exit exynos_pcie_remove(struct platform_device *pdev)
+static int exynos_pcie_remove(struct platform_device *pdev)
 {
 	struct exynos_pcie *ep = platform_get_drvdata(pdev);
 
@@ -431,7 +431,7 @@ static const struct of_device_id exynos_pcie_of_match[] = {
 
 static struct platform_driver exynos_pcie_driver = {
 	.probe		= exynos_pcie_probe,
-	.remove		= __exit_p(exynos_pcie_remove),
+	.remove		= exynos_pcie_remove,
 	.driver = {
 		.name	= "exynos-pcie",
 		.of_match_table = exynos_pcie_of_match,
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] PCI: kirin: Don't put .remove callback in .exit.text section
  2023-10-01 17:02 [PATCH 0/4] pci: Fix some section mismatches Uwe Kleine-König
  2023-10-01 17:02 ` [PATCH 1/4] PCI: exynos: Don't put .remove callback in .exit.text section Uwe Kleine-König
@ 2023-10-01 17:02 ` Uwe Kleine-König
  2023-10-02 22:12   ` Bjorn Helgaas
  2023-10-01 17:02 ` [PATCH 3/4] PCI: keystone: " Uwe Kleine-König
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2023-10-01 17:02 UTC (permalink / raw)
  To: Xiaowei Song, Binghui Wang, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Bjorn Helgaas, Kishon Vijay Abraham I,
	Mauro Carvalho Chehab
  Cc: Rob Herring, linux-pci, kernel

With CONFIG_PCIE_KIRIN=y and kirin_pcie_remove() marked with __exit, the
function is discarded from the driver. In this case a bound device can
still get unbound, e.g via sysfs. Then no cleanup code is run resulting
in resource leaks or worse.

The right thing to do is do always have the remove callback available.
This fixes the following warning by modpost:

	drivers/pci/controller/dwc/pcie-kirin: section mismatch in reference: kirin_pcie_driver+0x8 (section: .data) -> kirin_pcie_remove (section: .exit.text)

(with ARCH=x86_64 W=1 allmodconfig).

Fixes: 000f60db784b ("PCI: kirin: Add support for a PHY layer")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pci/controller/dwc/pcie-kirin.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index d93bc2906950..2ee146767971 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -741,7 +741,7 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
 	return ret;
 }
 
-static int __exit kirin_pcie_remove(struct platform_device *pdev)
+static int kirin_pcie_remove(struct platform_device *pdev)
 {
 	struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
 
@@ -818,7 +818,7 @@ static int kirin_pcie_probe(struct platform_device *pdev)
 
 static struct platform_driver kirin_pcie_driver = {
 	.probe			= kirin_pcie_probe,
-	.remove	        	= __exit_p(kirin_pcie_remove),
+	.remove	        	= kirin_pcie_remove,
 	.driver			= {
 		.name			= "kirin-pcie",
 		.of_match_table		= kirin_pcie_match,
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] PCI: keystone: Don't put .remove callback in .exit.text section
  2023-10-01 17:02 [PATCH 0/4] pci: Fix some section mismatches Uwe Kleine-König
  2023-10-01 17:02 ` [PATCH 1/4] PCI: exynos: Don't put .remove callback in .exit.text section Uwe Kleine-König
  2023-10-01 17:02 ` [PATCH 2/4] PCI: kirin: " Uwe Kleine-König
@ 2023-10-01 17:02 ` Uwe Kleine-König
  2023-10-01 17:02 ` [PATCH 4/4] PCI: keystone: Don't put .probe callback in .init.text section Uwe Kleine-König
  2023-10-10 17:23 ` [PATCH 0/4] pci: Fix some section mismatches Bjorn Helgaas
  4 siblings, 0 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2023-10-01 17:02 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas,
	Santosh Shilimkar, Murali Karicheri
  Cc: Rob Herring, linux-pci, kernel

With CONFIG_PCIE_KEYSTONE=y and ks_pcie_remove() marked with __exit, the
function is discarded from the driver. In this case a bound device can
still get unbound, e.g via sysfs. Then no cleanup code is run resulting
in resource leaks or worse.

The right thing to do is do always have the remove callback available.
Note that this driver cannot be compiled as a module, so
ks_pcie_remove() was always discarded before this change and modpost
couldn't warn about this issue. Further more the __ref annotation also
prevents a warning.

Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-keystone.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 49aea6ce3e87..eb3fa17b243f 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -1302,7 +1302,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int __exit ks_pcie_remove(struct platform_device *pdev)
+static int ks_pcie_remove(struct platform_device *pdev)
 {
 	struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);
 	struct device_link **link = ks_pcie->link;
@@ -1320,7 +1320,7 @@ static int __exit ks_pcie_remove(struct platform_device *pdev)
 
 static struct platform_driver ks_pcie_driver __refdata = {
 	.probe  = ks_pcie_probe,
-	.remove = __exit_p(ks_pcie_remove),
+	.remove = ks_pcie_remove,
 	.driver = {
 		.name	= "keystone-pcie",
 		.of_match_table = ks_pcie_of_match,
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] PCI: keystone: Don't put .probe callback in .init.text section
  2023-10-01 17:02 [PATCH 0/4] pci: Fix some section mismatches Uwe Kleine-König
                   ` (2 preceding siblings ...)
  2023-10-01 17:02 ` [PATCH 3/4] PCI: keystone: " Uwe Kleine-König
@ 2023-10-01 17:02 ` Uwe Kleine-König
  2023-10-10 17:23 ` [PATCH 0/4] pci: Fix some section mismatches Bjorn Helgaas
  4 siblings, 0 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2023-10-01 17:02 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas,
	Santosh Shilimkar, Murali Karicheri
  Cc: Rob Herring, linux-pci, kernel

The __init annotation makes the ks_pcie_probe() function disappear after
booting completes. However a device can also be bound later. In that
case ks_pcie_probe() is tried to be called but the backing memory is
likely already overwritten.

The right thing to do is do always have the probe callback available.
Note that the (wrong) __refdata annotation prevented this issue to be
noticed by modpost.

Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-keystone.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index eb3fa17b243f..0def919f89fa 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -1100,7 +1100,7 @@ static const struct of_device_id ks_pcie_of_match[] = {
 	{ },
 };
 
-static int __init ks_pcie_probe(struct platform_device *pdev)
+static int ks_pcie_probe(struct platform_device *pdev)
 {
 	const struct dw_pcie_host_ops *host_ops;
 	const struct dw_pcie_ep_ops *ep_ops;
@@ -1318,7 +1318,7 @@ static int ks_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct platform_driver ks_pcie_driver __refdata = {
+static struct platform_driver ks_pcie_driver = {
 	.probe  = ks_pcie_probe,
 	.remove = ks_pcie_remove,
 	.driver = {
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* RE: [PATCH 1/4] PCI: exynos: Don't put .remove callback in .exit.text section
  2023-10-01 17:02 ` [PATCH 1/4] PCI: exynos: Don't put .remove callback in .exit.text section Uwe Kleine-König
@ 2023-10-02  1:38   ` Alim Akhtar
  0 siblings, 0 replies; 12+ messages in thread
From: Alim Akhtar @ 2023-10-02  1:38 UTC (permalink / raw)
  To: 'Uwe Kleine-König', 'Jingoo Han',
	'Lorenzo Pieralisi', 'Krzysztof Wilczyński',
	'Bjorn Helgaas', 'Krzysztof Kozlowski',
	'Kukjin Kim', 'Siva Reddy Kallam',
	'Surendranath Gurivireddy Balla'
  Cc: 'Rob Herring', linux-pci, linux-arm-kernel,
	linux-samsung-soc, kernel



> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Sunday, October 1, 2023 10:33 PM
> To: Jingoo Han <jingoohan1@gmail.com>; Lorenzo Pieralisi
> <lpieralisi@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas
> <bhelgaas@google.com>; Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org>; Kukjin Kim <kgene.kim@samsung.com>; Siva
> Reddy Kallam <siva.kallam@samsung.com>; Surendranath Gurivireddy Balla
> <suren.reddy@samsung.com>
> Cc: Rob Herring <robh@kernel.org>; Alim Akhtar <alim.akhtar@samsung.com>;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-
> soc@vger.kernel.org; kernel@pengutronix.de
> Subject: [PATCH 1/4] PCI: exynos: Don't put .remove callback in .exit.text
> section
> 
> With CONFIG_PCI_EXYNOS=y and exynos_pcie_remove() marked with __exit,
> the function is discarded from the driver. In this case a bound device can still get
> unbound, e.g via sysfs. Then no cleanup code is run resulting in resource leaks or
> worse.
> 
> The right thing to do is do always have the remove callback available.
> This fixes the following warning by modpost:
> 
> 	WARNING: modpost: drivers/pci/controller/dwc/pci-exynos: section
> mismatch in reference: exynos_pcie_driver+0x8 (section: .data) ->
> exynos_pcie_remove (section: .exit.text)
> 
> (with ARCH=x86_64 W=1 allmodconfig).
> 
> Fixes: 340cba6092c2 ("pci: Add PCIe driver for Samsung Exynos")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
Thanks!

Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

>  drivers/pci/controller/dwc/pci-exynos.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-exynos.c
> b/drivers/pci/controller/dwc/pci-exynos.c
> index 6319082301d6..c6bede346932 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -375,7 +375,7 @@ static int exynos_pcie_probe(struct platform_device
> *pdev)
>  	return ret;
>  }
> 
> -static int __exit exynos_pcie_remove(struct platform_device *pdev)
> +static int exynos_pcie_remove(struct platform_device *pdev)
>  {
>  	struct exynos_pcie *ep = platform_get_drvdata(pdev);
> 
> @@ -431,7 +431,7 @@ static const struct of_device_id
> exynos_pcie_of_match[] = {
> 
>  static struct platform_driver exynos_pcie_driver = {
>  	.probe		= exynos_pcie_probe,
> -	.remove		= __exit_p(exynos_pcie_remove),
> +	.remove		= exynos_pcie_remove,
>  	.driver = {
>  		.name	= "exynos-pcie",
>  		.of_match_table = exynos_pcie_of_match,
> --
> 2.40.1




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] PCI: kirin: Don't put .remove callback in .exit.text section
  2023-10-01 17:02 ` [PATCH 2/4] PCI: kirin: " Uwe Kleine-König
@ 2023-10-02 22:12   ` Bjorn Helgaas
  2023-10-03 10:15     ` Uwe Kleine-König
  0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2023-10-02 22:12 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Xiaowei Song, Binghui Wang, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Bjorn Helgaas, Kishon Vijay Abraham I,
	Mauro Carvalho Chehab, Rob Herring, linux-pci, kernel

On Sun, Oct 01, 2023 at 07:02:52PM +0200, Uwe Kleine-König wrote:
> With CONFIG_PCIE_KIRIN=y and kirin_pcie_remove() marked with __exit, the
> function is discarded from the driver. In this case a bound device can
> still get unbound, e.g via sysfs. Then no cleanup code is run resulting
> in resource leaks or worse.

kirin_pcie_driver sets .suppress_bind_attrs = true.

Doesn't that mean that we can't unbind a device via sysfs in this
case?

I don't expect modpost to know about .suppress_bind_attrs, so maybe we
should remove the __exit annotation even if it would be safe to keep
it in this case.  It's a tiny function anyway.

> The right thing to do is do always have the remove callback available.
> This fixes the following warning by modpost:
> 
> 	drivers/pci/controller/dwc/pcie-kirin: section mismatch in reference: kirin_pcie_driver+0x8 (section: .data) -> kirin_pcie_remove (section: .exit.text)
> 
> (with ARCH=x86_64 W=1 allmodconfig).
> 
> Fixes: 000f60db784b ("PCI: kirin: Add support for a PHY layer")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pci/controller/dwc/pcie-kirin.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index d93bc2906950..2ee146767971 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -741,7 +741,7 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
>  	return ret;
>  }
>  
> -static int __exit kirin_pcie_remove(struct platform_device *pdev)
> +static int kirin_pcie_remove(struct platform_device *pdev)
>  {
>  	struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
>  
> @@ -818,7 +818,7 @@ static int kirin_pcie_probe(struct platform_device *pdev)
>  
>  static struct platform_driver kirin_pcie_driver = {
>  	.probe			= kirin_pcie_probe,
> -	.remove	        	= __exit_p(kirin_pcie_remove),
> +	.remove	        	= kirin_pcie_remove,
>  	.driver			= {
>  		.name			= "kirin-pcie",
>  		.of_match_table		= kirin_pcie_match,
> -- 
> 2.40.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] PCI: kirin: Don't put .remove callback in .exit.text section
  2023-10-02 22:12   ` Bjorn Helgaas
@ 2023-10-03 10:15     ` Uwe Kleine-König
  2023-10-03 20:23       ` Uwe Kleine-König
  0 siblings, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2023-10-03 10:15 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Krzysztof Wilczyński, Rob Herring, Mauro Carvalho Chehab,
	linux-pci, Lorenzo Pieralisi, Binghui Wang, kernel, Xiaowei Song,
	Bjorn Helgaas

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

Hello,

[dropped Kishon Vijay Abraham  from the list of recipients, their email
address doesn't work.]

On Mon, Oct 02, 2023 at 05:12:18PM -0500, Bjorn Helgaas wrote:
> On Sun, Oct 01, 2023 at 07:02:52PM +0200, Uwe Kleine-König wrote:
> > With CONFIG_PCIE_KIRIN=y and kirin_pcie_remove() marked with __exit, the
> > function is discarded from the driver. In this case a bound device can
> > still get unbound, e.g via sysfs. Then no cleanup code is run resulting
> > in resource leaks or worse.
> 
> kirin_pcie_driver sets .suppress_bind_attrs = true.
> 
> Doesn't that mean that we can't unbind a device via sysfs in this
> case?

Oh indeed, that's something I missed.
 
> I don't expect modpost to know about .suppress_bind_attrs, so maybe we
> should remove the __exit annotation even if it would be safe to keep
> it in this case.  It's a tiny function anyway.

the right thing to do then is something like:
https://lore.kernel.org/linux-rtc/20231002080529.2535610-7-u.kleine-koenig@pengutronix.de

And then it would be consequent to also switch to
module_platform_driver_probe and move .probe to __init. Or drop
.suppress_bind_attrs and keep/put .probe() and .remove() in .text.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] PCI: kirin: Don't put .remove callback in .exit.text section
  2023-10-03 10:15     ` Uwe Kleine-König
@ 2023-10-03 20:23       ` Uwe Kleine-König
  2023-10-03 20:40         ` Bjorn Helgaas
  0 siblings, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2023-10-03 20:23 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Krzysztof Wilczyński, Rob Herring, Mauro Carvalho Chehab,
	linux-pci, Lorenzo Pieralisi, Binghui Wang, kernel, Xiaowei Song,
	Bjorn Helgaas

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

On Tue, Oct 03, 2023 at 12:15:24PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> [dropped Kishon Vijay Abraham  from the list of recipients, their email
> address doesn't work.]
> 
> On Mon, Oct 02, 2023 at 05:12:18PM -0500, Bjorn Helgaas wrote:
> > On Sun, Oct 01, 2023 at 07:02:52PM +0200, Uwe Kleine-König wrote:
> > > With CONFIG_PCIE_KIRIN=y and kirin_pcie_remove() marked with __exit, the
> > > function is discarded from the driver. In this case a bound device can
> > > still get unbound, e.g via sysfs. Then no cleanup code is run resulting
> > > in resource leaks or worse.
> > 
> > kirin_pcie_driver sets .suppress_bind_attrs = true.
> > 
> > Doesn't that mean that we can't unbind a device via sysfs in this
> > case?
> 
> Oh indeed, that's something I missed.
>  
> > I don't expect modpost to know about .suppress_bind_attrs, so maybe we
> > should remove the __exit annotation even if it would be safe to keep
> > it in this case.  It's a tiny function anyway.
> 
> the right thing to do then is something like:
> https://lore.kernel.org/linux-rtc/20231002080529.2535610-7-u.kleine-koenig@pengutronix.de
> 
> And then it would be consequent to also switch to
> module_platform_driver_probe and move .probe to __init. Or drop
> .suppress_bind_attrs and keep/put .probe() and .remove() in .text.

The other three patches in this series don't suffer from this oversight
and so are (from my POV) ready to go in.

If you tell me, which option you prefer for the kirin driver, I'll
follow up with a matching patch. (If you don't know, my preference would
be to drop .suppress_bind_attrs and move .probe() and .remove() to
.text.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] PCI: kirin: Don't put .remove callback in .exit.text section
  2023-10-03 20:23       ` Uwe Kleine-König
@ 2023-10-03 20:40         ` Bjorn Helgaas
  2023-10-04  8:16           ` Uwe Kleine-König
  0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2023-10-03 20:40 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Krzysztof Wilczyński, Rob Herring, Mauro Carvalho Chehab,
	linux-pci, Lorenzo Pieralisi, Binghui Wang, kernel, Xiaowei Song,
	Bjorn Helgaas

On Tue, Oct 03, 2023 at 10:23:30PM +0200, Uwe Kleine-König wrote:
> On Tue, Oct 03, 2023 at 12:15:24PM +0200, Uwe Kleine-König wrote:
> > On Mon, Oct 02, 2023 at 05:12:18PM -0500, Bjorn Helgaas wrote:
> > > On Sun, Oct 01, 2023 at 07:02:52PM +0200, Uwe Kleine-König wrote:
> > > > With CONFIG_PCIE_KIRIN=y and kirin_pcie_remove() marked with __exit, the
> > > > function is discarded from the driver. In this case a bound device can
> > > > still get unbound, e.g via sysfs. Then no cleanup code is run resulting
> > > > in resource leaks or worse.
> > > 
> > > kirin_pcie_driver sets .suppress_bind_attrs = true.
> > > 
> > > Doesn't that mean that we can't unbind a device via sysfs in this
> > > case?
> > 
> > Oh indeed, that's something I missed.
> >  
> > > I don't expect modpost to know about .suppress_bind_attrs, so maybe we
> > > should remove the __exit annotation even if it would be safe to keep
> > > it in this case.  It's a tiny function anyway.
> > 
> > the right thing to do then is something like:
> > https://lore.kernel.org/linux-rtc/20231002080529.2535610-7-u.kleine-koenig@pengutronix.de
> > 
> > And then it would be consequent to also switch to
> > module_platform_driver_probe and move .probe to __init. Or drop
> > .suppress_bind_attrs and keep/put .probe() and .remove() in .text.
> 
> The other three patches in this series don't suffer from this oversight
> and so are (from my POV) ready to go in.

Agreed.  My first impression was that this would be v6.7 material, but
based on
https://lore.kernel.org/linux-kbuild/CAK7LNATyRg6Hc-fnTETERj-tdMFGaBDt0Fyhy9+jKCzAvzQ6Pg@mail.gmail.com/,
I guess that modpost change must be headed for v6.6?

And while I haven't seen problem reports, branching into the weeds
because of a sysfs "remove" would be a pretty bad outcome, so I can
see a case for v6.6 and stable tags as well.  Is that your thought
process, too?

> If you tell me, which option you prefer for the kirin driver, I'll
> follow up with a matching patch. (If you don't know, my preference would
> be to drop .suppress_bind_attrs and move .probe() and .remove() to
> .text.)

I agree, dropping .suppress_bind_attrs would be desirable, although I
would hope for some kind of assurance that it's not there because of
an issue with removal or something.

Bjorn

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] PCI: kirin: Don't put .remove callback in .exit.text section
  2023-10-03 20:40         ` Bjorn Helgaas
@ 2023-10-04  8:16           ` Uwe Kleine-König
  0 siblings, 0 replies; 12+ messages in thread
From: Uwe Kleine-König @ 2023-10-04  8:16 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Krzysztof Wilczyński, Rob Herring, Mauro Carvalho Chehab,
	linux-pci, Lorenzo Pieralisi, Binghui Wang, kernel, Xiaowei Song,
	Bjorn Helgaas

[-- Attachment #1: Type: text/plain, Size: 3444 bytes --]

Hello Bjorn,

On Tue, Oct 03, 2023 at 03:40:56PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 03, 2023 at 10:23:30PM +0200, Uwe Kleine-König wrote:
> > On Tue, Oct 03, 2023 at 12:15:24PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Oct 02, 2023 at 05:12:18PM -0500, Bjorn Helgaas wrote:
> > > > On Sun, Oct 01, 2023 at 07:02:52PM +0200, Uwe Kleine-König wrote:
> > > > > With CONFIG_PCIE_KIRIN=y and kirin_pcie_remove() marked with __exit, the
> > > > > function is discarded from the driver. In this case a bound device can
> > > > > still get unbound, e.g via sysfs. Then no cleanup code is run resulting
> > > > > in resource leaks or worse.
> > > > 
> > > > kirin_pcie_driver sets .suppress_bind_attrs = true.
> > > > 
> > > > Doesn't that mean that we can't unbind a device via sysfs in this
> > > > case?
> > > 
> > > Oh indeed, that's something I missed.
> > >  
> > > > I don't expect modpost to know about .suppress_bind_attrs, so maybe we
> > > > should remove the __exit annotation even if it would be safe to keep
> > > > it in this case.  It's a tiny function anyway.
> > > 
> > > the right thing to do then is something like:
> > > https://lore.kernel.org/linux-rtc/20231002080529.2535610-7-u.kleine-koenig@pengutronix.de
> > > 
> > > And then it would be consequent to also switch to
> > > module_platform_driver_probe and move .probe to __init. Or drop
> > > .suppress_bind_attrs and keep/put .probe() and .remove() in .text.
> > 
> > The other three patches in this series don't suffer from this oversight
> > and so are (from my POV) ready to go in.
> 
> Agreed.  My first impression was that this would be v6.7 material, but
> based on
> https://lore.kernel.org/linux-kbuild/CAK7LNATyRg6Hc-fnTETERj-tdMFGaBDt0Fyhy9+jKCzAvzQ6Pg@mail.gmail.com/,
> I guess that modpost change must be headed for v6.6?

No, the modpost change was softend and only triggers (for now) on W=1
builds. There is no urge.

> And while I haven't seen problem reports, branching into the weeds
> because of a sysfs "remove" would be a pretty bad outcome, so I can
> see a case for v6.6 and stable tags as well.  Is that your thought
> process, too?

The change is obviously a fix and a crash after sysfs interaction is
bad. It depends on the usecase if this is urgent. I'd say it's not a big
deal if it doesn't make it into v6.6, the problem (in this driver)
exists since 5.19-rc1 and there are still quite a few similar issues.
I'd go for marking for stable and applying for v6.6 if it's convenient.

> > If you tell me, which option you prefer for the kirin driver, I'll
> > follow up with a matching patch. (If you don't know, my preference would
> > be to drop .suppress_bind_attrs and move .probe() and .remove() to
> > .text.)
> 
> I agree, dropping .suppress_bind_attrs would be desirable, although I
> would hope for some kind of assurance that it's not there because of
> an issue with removal or something.

If there is a problem with removal, it's relevant for modular drivers at
rmmod time already now. I didn't check if there is a problem (and maybe
I cannot judge this objectively) but for sure such a problem would be
relevant already now. So I'd say this isn't an argument that should stop
us.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] pci: Fix some section mismatches
  2023-10-01 17:02 [PATCH 0/4] pci: Fix some section mismatches Uwe Kleine-König
                   ` (3 preceding siblings ...)
  2023-10-01 17:02 ` [PATCH 4/4] PCI: keystone: Don't put .probe callback in .init.text section Uwe Kleine-König
@ 2023-10-10 17:23 ` Bjorn Helgaas
  4 siblings, 0 replies; 12+ messages in thread
From: Bjorn Helgaas @ 2023-10-10 17:23 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jingoo Han, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Bjorn Helgaas, Krzysztof Kozlowski, Kukjin Kim, Siva Reddy Kallam,
	Surendranath Gurivireddy Balla, Xiaowei Song, Binghui Wang,
	Kishon Vijay Abraham I, Mauro Carvalho Chehab, Santosh Shilimkar,
	Murali Karicheri, Rob Herring, Alim Akhtar, linux-pci,
	linux-arm-kernel, linux-samsung-soc, kernel

On Sun, Oct 01, 2023 at 07:02:50PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> modpost checks about section mismatches are about to get stronger, see
> https://lore.kernel.org/linux-kbuild/20230930165204.2478282-1-u.kleine-koenig@pengutronix.de
> .
> 
> With the above patch applied, enabling the exynos and kirin drivers as
> modules result in a warning about their remove functions that is fixed
> here. The keystone driver is a bit special as it can only be enabled
> built-in and used __refdata on its driver struct. It also had a similar
> issue for .probe fixed in the last patch.
> 
> IMHO all four patches qualify for backporting to stable.

I added stable tags and applied to pci/enumeration for v6.7, thanks!

> Uwe Kleine-König (4):
>   PCI: exynos: Don't put .remove callback in .exit.text section
>   PCI: kirin: Don't put .remove callback in .exit.text section
>   PCI: keystone: Don't put .remove callback in .exit.text section
>   PCI: keystone: Don't put .probe callback in .init.text section

I updated the subjects to be "Don't discard ... callback" to try to
give a little more semantic context.

>  drivers/pci/controller/dwc/pci-exynos.c   | 4 ++--
>  drivers/pci/controller/dwc/pci-keystone.c | 8 ++++----
>  drivers/pci/controller/dwc/pcie-kirin.c   | 4 ++--
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> base-commit: 6465e260f48790807eef06b583b38ca9789b6072
> -- 
> 2.40.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-10-10 17:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-01 17:02 [PATCH 0/4] pci: Fix some section mismatches Uwe Kleine-König
2023-10-01 17:02 ` [PATCH 1/4] PCI: exynos: Don't put .remove callback in .exit.text section Uwe Kleine-König
2023-10-02  1:38   ` Alim Akhtar
2023-10-01 17:02 ` [PATCH 2/4] PCI: kirin: " Uwe Kleine-König
2023-10-02 22:12   ` Bjorn Helgaas
2023-10-03 10:15     ` Uwe Kleine-König
2023-10-03 20:23       ` Uwe Kleine-König
2023-10-03 20:40         ` Bjorn Helgaas
2023-10-04  8:16           ` Uwe Kleine-König
2023-10-01 17:02 ` [PATCH 3/4] PCI: keystone: " Uwe Kleine-König
2023-10-01 17:02 ` [PATCH 4/4] PCI: keystone: Don't put .probe callback in .init.text section Uwe Kleine-König
2023-10-10 17:23 ` [PATCH 0/4] pci: Fix some section mismatches Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).