diff mbox

pci: Add support for unbinding the generic PCI host controller

Message ID 57698276.6050606@siemens.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Jan Kiszka June 21, 2016, 6:07 p.m. UTC
Particularly useful when working in virtual environments where the
controller may come and go, but possibly not only there.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 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(+)

Comments

Will Deacon June 22, 2016, 6:06 a.m. UTC | #1
Hi Jan,

On Tue, Jun 21, 2016 at 08:07:50PM +0200, Jan Kiszka wrote:
> Particularly useful when working in virtual environments where the
> controller may come and go, but possibly not only there.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  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(+)
> 
> diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
> index 9878beb..5a5f607 100644
> --- a/drivers/pci/ecam.h
> +++ b/drivers/pci/ecam.h
> @@ -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
> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
> index 8cba7ab..c0ff4b1 100644
> --- a/drivers/pci/host/pci-host-common.c
> +++ b/drivers/pci/host/pci-host-common.c
> @@ -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();

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kiszka June 23, 2016, 5:31 p.m. UTC | #2
On 2016-06-22 08:06, Will Deacon wrote:
> Hi Jan,
> 
> On Tue, Jun 21, 2016 at 08:07:50PM +0200, Jan Kiszka wrote:
>> Particularly useful when working in virtual environments where the
>> controller may come and go, but possibly not only there.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  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(+)
>>
>> diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
>> index 9878beb..5a5f607 100644
>> --- a/drivers/pci/ecam.h
>> +++ b/drivers/pci/ecam.h
>> @@ -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
>> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
>> index 8cba7ab..c0ff4b1 100644
>> --- a/drivers/pci/host/pci-host-common.c
>> +++ b/drivers/pci/host/pci-host-common.c
>> @@ -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();
> 
> 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.
> 

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
Will Deacon June 24, 2016, 6:12 a.m. UTC | #3
On Thu, Jun 23, 2016 at 07:31:34PM +0200, Jan Kiszka wrote:
> On 2016-06-22 08:06, Will Deacon wrote:
> > On Tue, Jun 21, 2016 at 08:07:50PM +0200, Jan Kiszka wrote:
> >> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
> >> index 8cba7ab..c0ff4b1 100644
> >> --- a/drivers/pci/host/pci-host-common.c
> >> +++ b/drivers/pci/host/pci-host-common.c
> >> @@ -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();
> > 
> > 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.
> > 
> 
> 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() {

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kiszka June 24, 2016, 6:39 a.m. UTC | #4
On 2016-06-24 08:12, Will Deacon wrote:
> On Thu, Jun 23, 2016 at 07:31:34PM +0200, Jan Kiszka wrote:
>> On 2016-06-22 08:06, Will Deacon wrote:
>>> On Tue, Jun 21, 2016 at 08:07:50PM +0200, Jan Kiszka wrote:
>>>> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
>>>> index 8cba7ab..c0ff4b1 100644
>>>> --- a/drivers/pci/host/pci-host-common.c
>>>> +++ b/drivers/pci/host/pci-host-common.c
>>>> @@ -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();
>>>
>>> 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.
>>>
>>
>> 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() {
> 
> 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?

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
Will Deacon June 24, 2016, 6:50 a.m. UTC | #5
On Fri, Jun 24, 2016 at 08:39:08AM +0200, Jan Kiszka wrote:
> On 2016-06-24 08:12, Will Deacon wrote:
> > On Thu, Jun 23, 2016 at 07:31:34PM +0200, Jan Kiszka wrote:
> >> On 2016-06-22 08:06, Will Deacon wrote:
> >>>   - 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.
> >>>
> >>
> >> 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() {
> > 
> > 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?
> 
> 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?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Aug. 22, 2016, 5:05 p.m. UTC | #6
Hi Jan,

On Tue, Jun 21, 2016 at 08:07:50PM +0200, Jan Kiszka wrote:
> 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.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  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(+)
> 
> diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
> index 9878beb..5a5f607 100644
> --- a/drivers/pci/ecam.h
> +++ b/drivers/pci/ecam.h
> @@ -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
> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
> index 8cba7ab..c0ff4b1 100644
> --- a/drivers/pci/host/pci-host-common.c
> +++ b/drivers/pci/host/pci-host-common.c
> @@ -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;
>  }
>  
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 6eaceab..0a2e75b 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -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);
>  
> -- 
> 2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kiszka Aug. 22, 2016, 5:23 p.m. UTC | #7
On 2016-08-22 13:05, Bjorn Helgaas wrote:
> Hi Jan,
> 
> On Tue, Jun 21, 2016 at 08:07:50PM +0200, Jan Kiszka wrote:
>> 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

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  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(+)
>>
>> diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
>> index 9878beb..5a5f607 100644
>> --- a/drivers/pci/ecam.h
>> +++ b/drivers/pci/ecam.h
>> @@ -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
>> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
>> index 8cba7ab..c0ff4b1 100644
>> --- a/drivers/pci/host/pci-host-common.c
>> +++ b/drivers/pci/host/pci-host-common.c
>> @@ -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;
>>  }
>>  
>> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> index 6eaceab..0a2e75b 100644
>> --- a/drivers/pci/host/pci-host-generic.c
>> +++ b/drivers/pci/host/pci-host-generic.c
>> @@ -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);
>>  
>> -- 
>> 2.1.4
diff mbox

Patch

diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
index 9878beb..5a5f607 100644
--- a/drivers/pci/ecam.h
+++ b/drivers/pci/ecam.h
@@ -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
diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
index 8cba7ab..c0ff4b1 100644
--- a/drivers/pci/host/pci-host-common.c
+++ b/drivers/pci/host/pci-host-common.c
@@ -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;
 }
 
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 6eaceab..0a2e75b 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -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);