pci: Add support for unbinding the generic PCI host controller
PATCH 1/1 pci: Add support for unbinding the generic PCI host controller
Particularly useful when working in virtual environments where the controller may come and go, but possibly not only there.
--- drivers/pci/ecam.h | 1 + drivers/pci/host/pci-host-common.c | 13 +++++++++++++ drivers/pci/host/pci-host-generic.c | 1 + 3 files changed, 15 insertions(+)
drivers/pci/ecam.h +1 -0
@@ -63,5 +63,6 @@ extern struct pci_ecam_ops pci_generic_ecam_ops;/* for DT-based PCI controllers that support ECAM */ int pci_host_common_probe (struct platform_device *pdev,struct pci_ecam_ops *ops); +int pci_host_common_remove (struct platform_device *pdev);#endif #endif
drivers/pci/host/pci-host-common.c +13 -0
@@ -164,6 +164,19 @@ int pci_host_common_probe(struct platform_device *pdev, }pci_bus_add_devices (bus); +platform_set_drvdata (pdev, bus); +return 0 ; +} + +int pci_host_common_remove (struct platform_device *pdev) +{ +struct pci_bus *bus =platform_get_drvdata (pdev); + +pci_lock_rescan_remove (); +pci_stop_root_bus (bus); +pci_remove_root_bus (bus); +pci_unlock_rescan_remove (); +return 0 ; }
drivers/pci/host/pci-host-generic.c +1 -0
@@ -65,6 +65,7 @@ static struct platform_driver gen_pci_driver = { .of_match_table = gen_pci_of_match, }, .probe = gen_pci_probe, + .remove = pci_host_common_remove, };module_platform_driver (gen_pci_driver);
Signature · 1 line
2.1.4
Hi Jan,
Quoted 31 lines · Jan Kiszka
Particularly useful when working in virtual environments where the controller may come and go, but possibly not only there.
Signed-off-by Jan Kiszka <[email protected]>--- drivers/pci/ecam.h | 1 + drivers/pci/host/pci-host-common.c | 13 +++++++++++++ drivers/pci/host/pci-host-generic.c | 1 + 3 files changed, 15 insertions(+)drivers/pci/ecam.h +1
-0@@ -63,5 +63,6 @@ extern struct pci_ecam_ops pci_generic_ecam_ops;/* for DT-based PCI controllers that support ECAM */ int pci_host_common_probe (struct platform_device *pdev,struct pci_ecam_ops *ops); +int pci_host_common_remove (struct platform_device *pdev);#endif #endif drivers/pci/host/pci-host-common.c +2
-0@@ -164,6 +164,19 @@ int pci_host_common_probe(struct platform_device *pdev, }pci_bus_add_devices (bus); +platform_set_drvdata (pdev, bus); +return 0 ;
drivers/pci/host/pci-host-common.c int pci_host_common_probe(struct platform_device *pdev, · line 178
Quoted 10 lines
drivers/pci/host/pci-host-common.c +10
-0+} + +int pci_host_common_remove (struct platform_device *pdev) +{ +struct pci_bus *bus =platform_get_drvdata (pdev); + +pci_lock_rescan_remove (); +pci_stop_root_bus (bus); +pci_remove_root_bus (bus); +pci_unlock_rescan_remove ();
A couple of comments/questions about this:
- The probe path seems to have some stateful operations outside of PCI resources. For example, kzalloc'ing the bus_range resource in of_pci_get_host_bridge_resources. Do we need to clean these up explicitly?
- Similarly, we don't seem to tear-down the config space mappings and data structures for that, so we leak VA space afaict.
Will
Quoted 47 lines · Will Deacon
Hi Jan,
Quoted 31 lines · Jan Kiszka
Particularly useful when working in virtual environments where the controller may come and go, but possibly not only there.
Signed-off-by Jan Kiszka <[email protected]>--- drivers/pci/ecam.h | 1 + drivers/pci/host/pci-host-common.c | 13 +++++++++++++ drivers/pci/host/pci-host-generic.c | 1 + 3 files changed, 15 insertions(+)drivers/pci/ecam.h +1
-0@@ -63,5 +63,6 @@ extern struct pci_ecam_ops pci_generic_ecam_ops;/* for DT-based PCI controllers that support ECAM */ int pci_host_common_probe (struct platform_device *pdev,struct pci_ecam_ops *ops); +int pci_host_common_remove (struct platform_device *pdev);#endif #endif drivers/pci/host/pci-host-common.c +2
-0@@ -164,6 +164,19 @@ int pci_host_common_probe(struct platform_device *pdev, }pci_bus_add_devices (bus); +platform_set_drvdata (pdev, bus); +return 0 ;drivers/pci/host/pci-host-common.c int pci_host_common_probe(struct platform_device *pdev, · line 178
Quoted 10 lines
drivers/pci/host/pci-host-common.c +10
-0+} + +int pci_host_common_remove (struct platform_device *pdev) +{ +struct pci_bus *bus =platform_get_drvdata (pdev); + +pci_lock_rescan_remove (); +pci_stop_root_bus (bus); +pci_remove_root_bus (bus); +pci_unlock_rescan_remove ();A couple of comments/questions about this:
Quoted 8 lines
- The probe path seems to have some stateful operations outside of PCI resources. For example, kzalloc'ing the
bus_rangeresource inof_pci_get_host_bridge_resources. Do we need to clean these up explicitly?- Similarly, we don't seem to tear-down the config space mappings and data structures for that, so we leak VA space afaict.
Good points. But to my understanding, everything is released automatically on pci_remove_root_bus because all the resources are registered with the bus which takes care of them during destruction. And if I trace the release, I find this e.g.
...devres_release_all() {
_raw_spin_lock_irqsave();
release_nodes() {
_raw_spin_unlock_irqrestore();
devm_action_release() {
gen_pci_unmap_cfg() {Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux
Quoted 39 lines · Jan Kiszka
Quoted 24 lines · Will Deacon
Quoted 10 lines · Jan Kiszka
drivers/pci/host/pci-host-common.c +2
-0@@ -164,6 +164,19 @@ int pci_host_common_probe(struct platform_device *pdev, }pci_bus_add_devices (bus); +platform_set_drvdata (pdev, bus); +return 0 ;drivers/pci/host/pci-host-common.c int pci_host_common_probe(struct platform_device *pdev, · line 178
Quoted 10 lines
drivers/pci/host/pci-host-common.c +10
-0+} + +int pci_host_common_remove (struct platform_device *pdev) +{ +struct pci_bus *bus =platform_get_drvdata (pdev); + +pci_lock_rescan_remove (); +pci_stop_root_bus (bus); +pci_remove_root_bus (bus); +pci_unlock_rescan_remove ();A couple of comments/questions about this:
Quoted 8 lines
- The probe path seems to have some stateful operations outside of PCI resources. For example, kzalloc'ing the
bus_rangeresource inof_pci_get_host_bridge_resources. Do we need to clean these up explicitly?- Similarly, we don't seem to tear-down the config space mappings and data structures for that, so we leak VA space afaict.
Good points. But to my understanding, everything is released automatically on
pci_remove_root_busbecause all the resources are registered with the bus which takes care of them during destruction. And if I trace the release, I find this e.g.
Quoted 7 lines
...
devres_release_all(){
_raw_spin_lock_irqsave();release_nodes() { _raw_spin_unlock_irqrestore(); devm_action_release() { gen_pci_unmap_cfg() {
Ah, thanks for investigating that -- I completely missed the explicit devm_ callback. That resolves my concern about the config space mappings, but I still don't understand what happens to the resources allocated in of_pci_get_host_bridge_resources. It looks like they should actually be freed within that function, since pci_add_resource{_offset} copy the resources into their own resource_entries anyway.
Am I missing something?
Will
Quoted 48 lines · Will Deacon
Quoted 39 lines · Jan Kiszka
Quoted 24 lines · Will Deacon
Quoted 10 lines · Jan Kiszka
drivers/pci/host/pci-host-common.c +2
-0@@ -164,6 +164,19 @@ int pci_host_common_probe(struct platform_device *pdev, }pci_bus_add_devices (bus); +platform_set_drvdata (pdev, bus); +return 0 ;drivers/pci/host/pci-host-common.c int pci_host_common_probe(struct platform_device *pdev, · line 178
Quoted 10 lines
drivers/pci/host/pci-host-common.c +10
-0+} + +int pci_host_common_remove (struct platform_device *pdev) +{ +struct pci_bus *bus =platform_get_drvdata (pdev); + +pci_lock_rescan_remove (); +pci_stop_root_bus (bus); +pci_remove_root_bus (bus); +pci_unlock_rescan_remove ();A couple of comments/questions about this:
Quoted 8 lines
- The probe path seems to have some stateful operations outside of PCI resources. For example, kzalloc'ing the
bus_rangeresource inof_pci_get_host_bridge_resources. Do we need to clean these up explicitly?- Similarly, we don't seem to tear-down the config space mappings and data structures for that, so we leak VA space afaict.
Good points. But to my understanding, everything is released automatically on
pci_remove_root_busbecause all the resources are registered with the bus which takes care of them during destruction. And if I trace the release, I find this e.g.Quoted 7 lines
...
devres_release_all(){
_raw_spin_lock_irqsave();release_nodes() { _raw_spin_unlock_irqrestore(); devm_action_release() { gen_pci_unmap_cfg() {
Quoted 8 lines
Ah, thanks for investigating that -- I completely missed the explicit
devm_callback. That resolves my concern about the config space mappings, but I still don't understand what happens to the resources allocated inof_pci_get_host_bridge_resources. It looks like they should actually be freed within that function, since pci_add_resource{_offset} copy the resources into their ownresource_entriesanyway.Am I missing something?
Tracing kmalloc and kfree in addition, it seems you are right. But then we already have leaks in the code in case the setup fails somewhere in the middle, no? gen_pci_init only releases the resource list on errors, but not bus_range. And pci_host_common_probe does non of both if pci_scan_root_bus fails. Anything else?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux
Quoted 32 lines · Jan Kiszka
Quoted 31 lines · Will Deacon
Quoted 15 lines · Jan Kiszka
Quoted 8 lines · Will Deacon
- The probe path seems to have some stateful operations outside of PCI resources. For example, kzalloc'ing the
bus_rangeresource inof_pci_get_host_bridge_resources. Do we need to clean these up explicitly?- Similarly, we don't seem to tear-down the config space mappings and data structures for that, so we leak VA space afaict.
Good points. But to my understanding, everything is released automatically on
pci_remove_root_busbecause all the resources are registered with the bus which takes care of them during destruction. And if I trace the release, I find this e.g.Quoted 7 lines
...
devres_release_all(){
_raw_spin_lock_irqsave();release_nodes() { _raw_spin_unlock_irqrestore(); devm_action_release() { gen_pci_unmap_cfg() {Ah, thanks for investigating that -- I completely missed the explicit
devm_callback. That resolves my concern about the config space mappings, but I still don't understand what happens to the resources allocated inof_pci_get_host_bridge_resources. It looks like they should actually be freed within that function, since pci_add_resource{_offset} copy the resources into their ownresource_entriesanyway.
Quoted 7 lines
Quoted 1 line
Am I missing something?
Tracing kmalloc and kfree in addition, it seems you are right. But then we already have leaks in the code in case the setup fails somewhere in the middle, no?
gen_pci_initonly releases the resource list on errors, but notbus_range. Andpci_host_common_probedoes non of both ifpci_scan_root_busfails. Anything else?
Yeah, it's a right old mess. of_pci_get_host_bridge_resources even cleans up after itself, but gen_pci_init can fail in other ways and then doesn't clean up properly.
I wonder why of_pci_get_host_bridge_resources can't just use devm_kzalloc.
Will
Hi Jan,
Quoted 2 lines · Jan Kiszka
Particularly useful when working in virtual environments where the controller may come and go, but possibly not only there.
I'm not sure where we're at with this. Will had a few questions about the tear-down paths, and I couldn't tell what the resolution was.
Quoted 56 lines
Signed-off-by Jan Kiszka <[email protected]>--- drivers/pci/ecam.h | 1 + drivers/pci/host/pci-host-common.c | 13 +++++++++++++ drivers/pci/host/pci-host-generic.c | 1 + 3 files changed, 15 insertions(+)drivers/pci/ecam.h +1
-0@@ -63,5 +63,6 @@ extern struct pci_ecam_ops pci_generic_ecam_ops;/* for DT-based PCI controllers that support ECAM */ int pci_host_common_probe (struct platform_device *pdev,struct pci_ecam_ops *ops); +int pci_host_common_remove (struct platform_device *pdev);#endif #endif drivers/pci/host/pci-host-common.c +13
-0@@ -164,6 +164,19 @@ int pci_host_common_probe(struct platform_device *pdev, }pci_bus_add_devices (bus); +platform_set_drvdata (pdev, bus); +return 0 ; +} + +int pci_host_common_remove (struct platform_device *pdev) +{ +struct pci_bus *bus =platform_get_drvdata (pdev); + +pci_lock_rescan_remove (); +pci_stop_root_bus (bus); +pci_remove_root_bus (bus); +pci_unlock_rescan_remove (); +return 0 ; }drivers/pci/host/pci-host-generic.c +1
-0@@ -65,6 +65,7 @@ static struct platform_driver gen_pci_driver = { .of_match_table = gen_pci_of_match, }, .probe = gen_pci_probe, + .remove = pci_host_common_remove, };module_platform_driver (gen_pci_driver);Signature · 1 line
2.1.4
Quoted 8 lines · Bjorn Helgaas
Hi Jan,
Quoted 2 lines · Jan Kiszka
Particularly useful when working in virtual environments where the controller may come and go, but possibly not only there.
I'm not sure where we're at with this. Will had a few questions about the tear-down paths, and I couldn't tell what the resolution was.
Likely already the existing code leaks memory when rolling back on errors. That needs to be analysed and resolved. Then it will be simple to make unbinding leak-free as well.
I'm not familiar enough with the functions involved and still need to find the required time to dig into this.
Jan
Quoted 57 lines
Quoted 57 lines
Signed-off-by Jan Kiszka <[email protected]>--- drivers/pci/ecam.h | 1 + drivers/pci/host/pci-host-common.c | 13 +++++++++++++ drivers/pci/host/pci-host-generic.c | 1 + 3 files changed, 15 insertions(+)drivers/pci/ecam.h +1
-0@@ -63,5 +63,6 @@ extern struct pci_ecam_ops pci_generic_ecam_ops;/* for DT-based PCI controllers that support ECAM */ int pci_host_common_probe (struct platform_device *pdev,struct pci_ecam_ops *ops); +int pci_host_common_remove (struct platform_device *pdev);#endif #endif drivers/pci/host/pci-host-common.c +13
-0@@ -164,6 +164,19 @@ int pci_host_common_probe(struct platform_device *pdev, }pci_bus_add_devices (bus); +platform_set_drvdata (pdev, bus); +return 0 ; +} + +int pci_host_common_remove (struct platform_device *pdev) +{ +struct pci_bus *bus =platform_get_drvdata (pdev); + +pci_lock_rescan_remove (); +pci_stop_root_bus (bus); +pci_remove_root_bus (bus); +pci_unlock_rescan_remove (); +return 0 ; }drivers/pci/host/pci-host-generic.c +1
-0@@ -65,6 +65,7 @@ static struct platform_driver gen_pci_driver = { .of_match_table = gen_pci_of_match, }, .probe = gen_pci_probe, + .remove = pci_host_common_remove, };module_platform_driver (gen_pci_driver);Signature · 1 line
2.1.4
Signature · 2 lines
Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux