PCI: keystone: fix incorrect annotations on probe and remove

PATCH In review Dmitry Torokhov 2 participants 5 messages Jan 8, 2015 – Jan 23, 2015 PCI

Share & tools

Dmitry Torokhov

author

2015-01-08 22:17 UTC #0 lore raw

PATCH 1/1 PCI: keystone: fix incorrect annotations on probe and remove

Even though platform bus is not hot-pluggable, devices on it can be unbound from the driver and bound back to it via sysfs, so we should not be using __init annotations on probe() and __exit annotations on remove() methods.

Signed-off-by Dmitry Torokhov <[email protected]>

---

Not tested, found by casual code inspection.

 drivers/pci/host/pci-keystone.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/pci/host/pci-keystone.c +4 -4

index 1b893bc..7b84e1d 100644
@@ -332,7 +332,7 @@ static const struct of_device_id ks_pcie_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ks_pcie_of_match);
 
-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);

@@ -341,7 +341,7 @@ static int __exit ks_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int __init ks_pcie_probe(struct platform_device *pdev)
+static int ks_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct keystone_pcie *ks_pcie;

@@ -398,9 +398,9 @@ fail_clk:
 	return ret;
 }
 
-static struct platform_driver ks_pcie_driver __refdata = {
+static struct platform_driver ks_pcie_driver = {
 	.probe  = ks_pcie_probe,
-	.remove = __exit_p(ks_pcie_remove),
+	.remove = ks_pcie_remove,
 	.driver = {
 		.name	= "keystone-pcie",
 		.owner	= THIS_MODULE,

Signature · 5 lines

2.2.0.rc0.207.ga3a616c


-- 
Dmitry

Bjorn Helgaas

maintainer

2015-01-23 21:34 UTC #1 lore raw

[+cc Fabio]

Quoted 3 lines · Dmitry Torokhov

Even though platform bus is not hot-pluggable, devices on it can be unbound from the driver and bound back to it via sysfs, so we should not be using __init annotations on probe() and __exit annotations on remove() methods.

I don't completely understand this problem. Does this mean there should be no __init/__exit annotations in these drivers at all? If other PCI host bridge drivers have the same problem, I'd like to fix them all at once.

For example, after applying this patch, ks_pcie_probe() is non-__init, but it calls ks_add_pcie_port(), which is still __init. I thought that was illegal.

I'm not sure about module_platform_driver_probe() either; it generates __init and __exit functions. Should those annotations be removed, too?

Quoted 48 lines

Signed-off-by Dmitry Torokhov <[email protected]>

---

Not tested, found by casual code inspection.

 drivers/pci/host/pci-keystone.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/pci/host/pci-keystone.c +4 -4

index 1b893bc..7b84e1d 100644
@@ -332,7 +332,7 @@ static const struct of_device_id ks_pcie_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ks_pcie_of_match);
 
-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);

@@ -341,7 +341,7 @@ static int __exit ks_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int __init ks_pcie_probe(struct platform_device *pdev)
+static int ks_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct keystone_pcie *ks_pcie;

@@ -398,9 +398,9 @@ fail_clk:
 	return ret;
 }
 
-static struct platform_driver ks_pcie_driver __refdata = {
+static struct platform_driver ks_pcie_driver = {
 	.probe  = ks_pcie_probe,
-	.remove = __exit_p(ks_pcie_remove),
+	.remove = ks_pcie_remove,
 	.driver = {
 		.name	= "keystone-pcie",
 		.owner	= THIS_MODULE,

Signature · 5 lines

2.2.0.rc0.207.ga3a616c


-- 
Dmitry

Dmitry Torokhov

author

2015-01-23 21:47 UTC #2 lore raw

Hi Bjorn,

Quoted 10 lines · Bjorn Helgaas

[+cc Fabio]
Quoted 3 lines · Dmitry Torokhov
Even though platform bus is not hot-pluggable, devices on it can be unbound from the driver and bound back to it via sysfs, so we should not be using __init annotations on probe() and __exit annotations on remove() methods.
I don't completely understand this problem. Does this mean there should be no __init/__exit annotations in these drivers at all? If other PCI host bridge drivers have the same problem, I'd like to fix them all at once.

Basically they should not use __init/__exit, unless you take explicit steps to make sure that devices can't be unbound and bound again later via sysfs by setting driver->suppress_bind_attrs and/or using platform_driver_probe(). Note that if you use platform_driver_probe() then your driver can't cope with -EPROBE_DEFER reported by any of the subsystems it might be using.

Quoted 4 lines

For example, after applying this patch, ks_pcie_probe() is non-__init, but it calls ks_add_pcie_port(), which is still __init. I thought that was illegal.

My bad, I missed that. The __init marking on ks_add_pcie_port() shoudl be removed as well.

Quoted 3 lines

I'm not sure about module_platform_driver_probe() either; it generates __init and __exit functions. Should those annotations be removed, too?

module_platform_driver_probe() does internally platform_driver_probe() so it is OK for that probe function be __init (because we'd suppress sysfs bind/unbind and deferred probing so discaring init section is safe).

Note that above applied to driver's ->probe() and ->remove() code paths only. Marking module init/exit code as __init and __exit is still right thing to do.

Thanks.

Quoted 49 lines

Signed-off-by Dmitry Torokhov <[email protected]>

---

Not tested, found by casual code inspection.

 drivers/pci/host/pci-keystone.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/pci/host/pci-keystone.c +4 -4

index 1b893bc..7b84e1d 100644
@@ -332,7 +332,7 @@ static const struct of_device_id ks_pcie_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ks_pcie_of_match);
 
-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);

@@ -341,7 +341,7 @@ static int __exit ks_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int __init ks_pcie_probe(struct platform_device *pdev)
+static int ks_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct keystone_pcie *ks_pcie;

@@ -398,9 +398,9 @@ fail_clk:
 	return ret;
 }
 
-static struct platform_driver ks_pcie_driver __refdata = {
+static struct platform_driver ks_pcie_driver = {
 	.probe  = ks_pcie_probe,
-	.remove = __exit_p(ks_pcie_remove),
+	.remove = ks_pcie_remove,
 	.driver = {
 		.name	= "keystone-pcie",
 		.owner	= THIS_MODULE,

Signature · 5 lines

2.2.0.rc0.207.ga3a616c


-- 
Dmitry

Signature · 1 line

Dmitry

Dmitry Torokhov

author

2015-01-23 22:03 UTC #3 lore raw

Quoted 13 lines · Dmitry Torokhov

Hi Bjorn,
Quoted 10 lines · Bjorn Helgaas
[+cc Fabio]
Quoted 3 lines · Dmitry Torokhov
Even though platform bus is not hot-pluggable, devices on it can be unbound from the driver and bound back to it via sysfs, so we should not be using __init annotations on probe() and __exit annotations on remove() methods.
I don't completely understand this problem. Does this mean there should be no __init/__exit annotations in these drivers at all? If other PCI host bridge drivers have the same problem, I'd like to fix them all at once.

By the way I think only keystone in drivers/pci has this issue. My script to find potential issues is:

#!/bin/bash
for i in grep -rl '__exit$_p\| $.*remove' $*; do grep -L platform_driver_probe $i; done

it is not 100% correct and generates both false positives and false negatives but still fains a few offenders in the tree.

Quoted 83 lines

Basically they should not use __init/__exit, unless you take explicit steps to make sure that devices can't be unbound and bound again later via sysfs by setting driver->suppress_bind_attrs and/or using platform_driver_probe(). Note that if you use platform_driver_probe() then your driver can't cope with -EPROBE_DEFER reported by any of the subsystems it might be using.
Quoted 4 lines
For example, after applying this patch, ks_pcie_probe() is non-__init, but it calls ks_add_pcie_port(), which is still __init. I thought that was illegal.
My bad, I missed that. The __init marking on ks_add_pcie_port() shoudl be removed as well.
Quoted 3 lines
I'm not sure about module_platform_driver_probe() either; it generates __init and __exit functions. Should those annotations be removed, too?
module_platform_driver_probe() does internally platform_driver_probe() so it is OK for that probe function be __init (because we'd suppress sysfs bind/unbind and deferred probing so discaring init section is safe).
Note that above applied to driver's ->probe() and ->remove() code paths only. Marking module init/exit code as __init and __exit is still right thing to do.
Thanks.
Quoted 49 lines
Quoted 49 lines
Signed-off-by Dmitry Torokhov <[email protected]>
---
Not tested, found by casual code inspection.
 drivers/pci/host/pci-keystone.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/pci/host/pci-keystone.c +4 -4
index 1b893bc..7b84e1d 100644
@@ -332,7 +332,7 @@ static const struct of_device_id ks_pcie_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ks_pcie_of_match);
 
-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);
 
@@ -341,7 +341,7 @@ static int __exit ks_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int __init ks_pcie_probe(struct platform_device *pdev)
+static int ks_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct keystone_pcie *ks_pcie;
@@ -398,9 +398,9 @@ fail_clk:
 	return ret;
 }
 
-static struct platform_driver ks_pcie_driver __refdata = {
+static struct platform_driver ks_pcie_driver = {
 	.probe  = ks_pcie_probe,
-	.remove = __exit_p(ks_pcie_remove),
+	.remove = ks_pcie_remove,
 	.driver = {
 		.name	= "keystone-pcie",
 		.owner	= THIS_MODULE,
Signature · 5 lines
2.2.0.rc0.207.ga3a616c


-- 
Dmitry
Signature · 1 line
Dmitry

Signature · 1 line

Dmitry

Bjorn Helgaas

maintainer

2015-01-23 23:04 UTC #4 lore raw

In reply to Dmitry Torokhov

Quoted 5 lines · Dmitry Torokhov

Even though platform bus is not hot-pluggable, devices on it can be unbound from the driver and bound back to it via sysfs, so we should not be using __init annotations on probe() and __exit annotations on remove() methods.
Signed-off-by Dmitry Torokhov <[email protected]>

I also removed the __init annotations from ks_pcie_host_init() and ks_add_pcie_port() and applied this to pci/host-keystone for v3.20, thanks!

Quoted 47 lines

---

Not tested, found by casual code inspection.

 drivers/pci/host/pci-keystone.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/pci/host/pci-keystone.c +4 -4

index 1b893bc..7b84e1d 100644
@@ -332,7 +332,7 @@ static const struct of_device_id ks_pcie_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, ks_pcie_of_match);
 
-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);

@@ -341,7 +341,7 @@ static int __exit ks_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int __init ks_pcie_probe(struct platform_device *pdev)
+static int ks_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct keystone_pcie *ks_pcie;

@@ -398,9 +398,9 @@ fail_clk:
 	return ret;
 }
 
-static struct platform_driver ks_pcie_driver __refdata = {
+static struct platform_driver ks_pcie_driver = {
 	.probe  = ks_pcie_probe,
-	.remove = __exit_p(ks_pcie_remove),
+	.remove = ks_pcie_remove,
 	.driver = {
 		.name	= "keystone-pcie",
 		.owner	= THIS_MODULE,

Signature · 5 lines

2.2.0.rc0.207.ga3a616c


-- 
Dmitry