[PATCH 08/23] PNPACPI: Fix device ref leaking in acpi_pnp_match

[PATCH 08/23] PNPACPI: Fix device ref leaking in acpi_pnp_match

[Yinghai Lu]

During testing pci root bus removal, found some root bus bridge is not freed.

If booting with pnpacpi=off, those hostbridge could be freed without problem.

It turns out that some devices reference are not released during acpi_pnp_match.

that match should not hold one device ref during every calling.

Add put_device calling before returning.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: stable@kernel.org
Cc: Len Brown <lenb@kernel.org>
Cc: Adam Belay <abelay@mit.edu>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-acpi@vger.kernel.org
---
 drivers/pnp/pnpacpi/core.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
index b00c176..d21e8f5 100644
--- a/drivers/pnp/pnpacpi/core.c
+++ b/drivers/pnp/pnpacpi/core.c
@@ -321,9 +321,14 @@ static int __init acpi_pnp_match(struct device *dev, void *_pnp)
 {
 	struct acpi_device *acpi = to_acpi_device(dev);
 	struct pnp_dev *pnp = _pnp;
+	struct device *physical_device;
+
+	physical_device = acpi_get_physical_device(acpi->handle);
+	if (physical_device)
+		put_device(physical_device);
 
 	/* true means it matched */
-	return !acpi_get_physical_device(acpi->handle)
+	return !physical_device
 	    && compare_pnp_id(pnp->id, acpi_device_hid(acpi));
 }
 
-- 
1.7.7

Re: [PATCH 08/23] PNPACPI: Fix device ref leaking in acpi_pnp_match

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> During testing pci root bus removal, found some root bus bridge is not freed.
>
> If booting with pnpacpi=off, those hostbridge could be freed without problem.
>
> It turns out that some devices reference are not released during acpi_pnp_match.
>
> that match should not hold one device ref during every calling.
>
> Add put_device calling before returning.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: stable@kernel.org
> Cc: Len Brown <lenb@kernel.org>
> Cc: Adam Belay <abelay@mit.edu>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-acpi@vger.kernel.org
> ---
>  drivers/pnp/pnpacpi/core.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
> index b00c176..d21e8f5 100644
> --- a/drivers/pnp/pnpacpi/core.c
> +++ b/drivers/pnp/pnpacpi/core.c
> @@ -321,9 +321,14 @@ static int __init acpi_pnp_match(struct device *dev, void *_pnp)
>  {
>        struct acpi_device *acpi = to_acpi_device(dev);
>        struct pnp_dev *pnp = _pnp;
> +       struct device *physical_device;
> +
> +       physical_device = acpi_get_physical_device(acpi->handle);
> +       if (physical_device)
> +               put_device(physical_device);
>
>        /* true means it matched */
> -       return !acpi_get_physical_device(acpi->handle)
> +       return !physical_device
>            && compare_pnp_id(pnp->id, acpi_device_hid(acpi));
>  }
>

I spent about an hour convincing myself that this patch does the right
thing.  I *think* it does, but it certainly is not obvious.  It's
always nicer if the get/put are in the same function or in functions
that obviously correspond to each other.  But you didn't create this
mess, so I don't hold you responsible for fixing it :)

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 17/23] PCI, ACPI: make acpi_pci_root_remove remove pci root bus too

[Yinghai Lu]

it will call new added pci_stop_and_remove_bus() to stop/remove pci root bus.

also checking if that pci_root_bus get removed already in bus remove in /sys

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/pci_root.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 7aff631..b38e347 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -643,10 +643,24 @@ static int acpi_pci_root_remove(struct acpi_device *device, int type)
 {
 	struct acpi_pci_root *root = acpi_driver_data(device);
 
+	/* that root bus could be removed already */
+	if (!pci_find_bus(root->segment, root->secondary.start)) {
+		dev_printk(KERN_DEBUG, &device->dev,
+		  "freeing acpi_pci_root, but pci root bus was removed before");
+		goto out;
+	}
+
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);
 
+	dev_printk(KERN_DEBUG, &device->dev,
+		"freeing acpi_pci_root, will remove pci root bus at first");
+	pci_stop_and_remove_bus(root->bus);
+
+out:
+	list_del(&root->node);
 	kfree(root);
+
 	return 0;
 }
 
-- 
1.7.7

[PATCH 18/23] PCI, ACPI: add acpi_pci_root_rescan()

[Yinghai Lu]

it will rescan all acpi pci root if related pci root bus get removed before.

Signed-off-by: Yinghai <yinghai@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-acpi@vger.kernel.org
---
 drivers/acpi/pci_root.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index b38e347..e1814e0 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -679,3 +679,55 @@ static int __init acpi_pci_root_init(void)
 }
 
 subsys_initcall(acpi_pci_root_init);
+
+static acpi_status
+rescan_root_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+	struct acpi_device *device, *pdevice;
+	struct acpi_pci_root *root;
+	acpi_handle phandle;
+	int ret_val;
+
+	if (!acpi_is_root_bridge(handle))
+		return AE_OK;
+
+	acpi_get_parent(handle, &phandle);
+	if (acpi_bus_get_device(phandle, &pdevice)) {
+		printk(KERN_DEBUG "no parent device, assuming NULL\n");
+		pdevice = NULL;
+	}
+
+	if (!acpi_bus_get_device(handle, &device)) {
+		/* check if  pci root_bus is removed */
+		root = acpi_driver_data(device);
+		if (pci_find_bus(root->segment, root->secondary.start))
+			return AE_OK;
+
+		printk(KERN_DEBUG "bus exists... trim\n");
+		/* this shouldn't be in here, so remove
+		 * the bus then re-add it...
+		 */
+		ret_val = acpi_bus_trim(device, 1);
+		printk(KERN_DEBUG "acpi_bus_trim return %x\n", ret_val);
+	}
+
+	ret_val = acpi_bus_add(&device, pdevice, handle, ACPI_BUS_TYPE_DEVICE);
+	if (ret_val) {
+		printk(KERN_ERR "error adding bus, %x\n", -ret_val);
+		goto out;
+	}
+
+	root = acpi_driver_data(device);
+	pci_assign_unassigned_bus_resources(root->bus);
+
+	ret_val = acpi_bus_start(device);
+
+out:
+	return ret_val;
+}
+
+void acpi_pci_root_rescan(void)
+{
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+		ACPI_UINT32_MAX, rescan_root_bridge, NULL, NULL, NULL);
+}
-- 
1.7.7

Re: [PATCH 18/23] PCI, ACPI: add acpi_pci_root_rescan()

[Bjorn Helgaas]

On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> it will rescan all acpi pci root if related pci root bus get removed before.
>
> Signed-off-by: Yinghai <yinghai@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-acpi@vger.kernel.org
> ---
>  drivers/acpi/pci_root.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index b38e347..e1814e0 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -679,3 +679,55 @@ static int __init acpi_pci_root_init(void)
>  }
>
>  subsys_initcall(acpi_pci_root_init);
> +
> +static acpi_status
> +rescan_root_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
> +{
> +       struct acpi_device *device, *pdevice;
> +       struct acpi_pci_root *root;
> +       acpi_handle phandle;
> +       int ret_val;
> +
> +       if (!acpi_is_root_bridge(handle))
> +               return AE_OK;
> +
> +       acpi_get_parent(handle, &phandle);
> +       if (acpi_bus_get_device(phandle, &pdevice)) {
> +               printk(KERN_DEBUG "no parent device, assuming NULL\n");

This printk (and the others below) does not have enough context.  A
user or developer will have no clue what it relates to.  Ideally they
would at least be dev_printk().

> +               pdevice = NULL;

Is a host bridge without a parent possible?  Seems dubious to me.

> +       }
> +
> +       if (!acpi_bus_get_device(handle, &device)) {
> +               /* check if  pci root_bus is removed */
> +               root = acpi_driver_data(device);
> +               if (pci_find_bus(root->segment, root->secondary.start))
> +                       return AE_OK;
> +
> +               printk(KERN_DEBUG "bus exists... trim\n");
> +               /* this shouldn't be in here, so remove
> +                * the bus then re-add it...
> +                */
> +               ret_val = acpi_bus_trim(device, 1);
> +               printk(KERN_DEBUG "acpi_bus_trim return %x\n", ret_val);
> +       }
> +
> +       ret_val = acpi_bus_add(&device, pdevice, handle, ACPI_BUS_TYPE_DEVICE);
> +       if (ret_val) {
> +               printk(KERN_ERR "error adding bus, %x\n", -ret_val);
> +               goto out;
> +       }
> +
> +       root = acpi_driver_data(device);
> +       pci_assign_unassigned_bus_resources(root->bus);
> +
> +       ret_val = acpi_bus_start(device);
> +
> +out:
> +       return ret_val;
> +}
> +
> +void acpi_pci_root_rescan(void)
> +{
> +       acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> +               ACPI_UINT32_MAX, rescan_root_bridge, NULL, NULL, NULL);
> +}

Walking the ACPI namespace is rarely the right thing to do.  Why do we
need it here?

Is this because we don't handle hotplug of ACPI devices in general?
The ACPI core *should* be noticing device removal and addition and
calling the driver .remove() and .add() methods directly.  I don't
think that's completely implemented, but I wish somebody would put
some effort into that rather than adding more hacky workarounds.

+cc Matthew Garrett, because I think he cares about this area too.

Re: [PATCH 16/23] PCI: add pci bus removal through /sys/.../pci_bus/.../remove

[Bjorn Helgaas]

[+cc Matthew Garrett, linux-acpi for ACPI hotplug]

On Wed, Mar 7, 2012 at 5:53 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Mar 7, 2012 at 4:03 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> it supports both pci root bus and pci bus under pci bridge.
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>> Cc: Randy Dunlap <rdunlap@xenotime.net>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: linux-doc@vger.kernel.org
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-pci |    8 ++++++++
>>>  drivers/pci/pci-sysfs.c                 |   28 ++++++++++++++++++++++++++++
>>>  2 files changed, 36 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
>>> index 95f0f37..22392de 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-pci
>>> +++ b/Documentation/ABI/testing/sysfs-bus-pci
>>> @@ -92,6 +92,14 @@ Description:
>>>                hot-remove the PCI device and any of its children.
>>>                Depends on CONFIG_HOTPLUG.
>>>
>>> +What:          /sys/bus/pci/devices/.../pci_bus/.../remove
>>> +Date:          March 2012
>>> +Contact:       Linux PCI developers <linux-pci@vger.kernel.org>
>>> +Description:
>>> +               Writing a non-zero value to this attribute will
>>> +               hot-remove the PCI bus and any of its children.
>>
>> Is this the interface we want?  It seems like the ultimate goal is to
>> remove the *host bridge*, i.e., the PNP0A08 device on x86.  If that's
>> the case, the logical thing seems like a
>> /sys/bus/acpi/devices/PNP0A08:00/remove file.
>>
>> All the current "remove" attributes are for *devices*, e.g.,
>> /sys/devices/pci0000:00/0000:00:03.0/remove, not for *buses*.
>
> could add that later.

Of course, we can always add things later.  What I'm concerned about
is that once we add an interface like this to sysfs, it's difficult to
remove.  Therefore, I don't want to add things that are clearly not
what we really want.

> I am thinking to add some non-intrusive way to make notification work
> for root-bus hot removal/add.

The Way Things Are Supposed To Work is that ACPI notifies the OS that
a device (a PNP0A08 host bridge in this case) is being removed.  The
OS driver does its cleanup and lets go of the device.  There are
several things in the ACPI part of that path that are not implemented
yet, so I understand that we might need a sysfs knob to kick things
off.  But can't we make a knob that just calls the host bridge driver
.remove() method, i.e., acpi_pci_root_remove()?

> also in some case, cpu buses get scan during booting, could use that
> to remove those not needed
> root buses.

I don't see the value in this.  I think you're talking about removing
the devices we discovered by blindly probing, i.e., the ones the BIOS
didn't tell us about.  I do think we should stop blindly probing for
them, but I don't see what we gain by adding a sysfs knob to remove
them.

>> I'm not sure it makes sense to talk about removing a "bus" and leaving
>> the upstream bridge (either a host bridge or a P2P bridge).  I think
>> it'd make more sense to remove the bridge itself, which would of
>> course have the consequence of removing the secondary bus.
>
> for root bus, that remove pci_host_bridge and pci root bus.
>
> for pci bus under pci bridge, will remove that pci bus, but will still
> keep  that pci bridge.
> that should be ok. just like some pci bridge is there, and later can
> not create child bus for it.
>
> there is one case: during test busn_alloc, i need to remove all device
> on one bus, and
> use setpci to change bridge bus number register. then use rescan
> bridge to create new bus.
>
> with this one, I just need to remove that bus, instead of remove
> children devices one by one.

I don't think making it convenient for manual testing is an argument
for this interface.  For sysfs interfaces it is more important to make
something that fits well into the grand plan of how things Should
Work.  If you need internal helper functions for convenience, I'm OK
with that, because it's easier to change those than to change sysfs
interfaces.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 16/23] PCI: add pci bus removal through /sys/.../pci_bus/.../remove

[Greg Kroah-Hartman]

On Wed, Mar 07, 2012 at 09:45:18PM -0700, Bjorn Helgaas wrote:
> >> I'm not sure it makes sense to talk about removing a "bus" and leaving
> >> the upstream bridge (either a host bridge or a P2P bridge).  I think
> >> it'd make more sense to remove the bridge itself, which would of
> >> course have the consequence of removing the secondary bus.
> >
> > for root bus, that remove pci_host_bridge and pci root bus.
> >
> > for pci bus under pci bridge, will remove that pci bus, but will still
> > keep  that pci bridge.
> > that should be ok. just like some pci bridge is there, and later can
> > not create child bus for it.
> >
> > there is one case: during test busn_alloc, i need to remove all device
> > on one bus, and
> > use setpci to change bridge bus number register. then use rescan
> > bridge to create new bus.
> >
> > with this one, I just need to remove that bus, instead of remove
> > children devices one by one.
> 
> I don't think making it convenient for manual testing is an argument
> for this interface.  For sysfs interfaces it is more important to make
> something that fits well into the grand plan of how things Should
> Work.  If you need internal helper functions for convenience, I'm OK
> with that, because it's easier to change those than to change sysfs
> interfaces.

If it's "only" for testing, then put it in debugfs, not sysfs.

thanks,

greg k-h