| Message ID | 20231001170254.2506508-3-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pci: Fix some section mismatches | expand |
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 >
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
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
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
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
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,
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(-)