Hi,
On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> Currently the pci_slot driver doesn't update PCI slot information
> when PCI device hotplug event happens, which may cause memory leak
> and returning stale information to user.
>
> So hook the BUS_NOTIFY_ADD_DEVICE/BUS_NOTIFY_DEL_DEVICE events to
> update PCI slot information when PCI hotplug event happens.
>
> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxx>
> Signed-off-by: Yijing Wang <wangyijing@xxxxxxxxxx>
> ---
> drivers/acpi/pci_slot.c | 86 ++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 78 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> index d22585f..e04ea8e 100644
> --- a/drivers/acpi/pci_slot.c
> +++ b/drivers/acpi/pci_slot.c
> @@ -32,6 +32,7 @@
> #include <linux/acpi.h>
> #include <acpi/acpi_bus.h>
> #include <acpi/acpi_drivers.h>
> +#include <linux/pci-acpi.h>
> #include <linux/dmi.h>
>
> static bool debug;
> @@ -123,12 +124,7 @@ struct callback_args {
> /*
> * register_slot
> *
> - * Called once for each SxFy object in the namespace. Don't worry about
> - * calling pci_create_slot multiple times for the same pci_bus:device,
> - * since each subsequent call simply bumps the refcount on the pci_slot.
> - *
> - * The number of calls to pci_destroy_slot from unregister_slot is
> - * symmetrical.
> + * Called once for each SxFy object in the namespace.
> */
> static acpi_status
> register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> @@ -145,6 +141,15 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> if (device < 0)
> return AE_OK;
>
> + /* Avoid duplicated records for the same slot */
> + list_for_each_entry(slot, &slot_list, list) {
> + pci_slot = slot->pci_slot;
> + if (pci_slot && pci_slot->bus == pci_bus &&
> + pci_slot->number == device) {
> + return AE_OK;
> + }
> + }
> +
> slot = kmalloc(sizeof(*slot), GFP_KERNEL);
> if (!slot) {
> err("%s: cannot allocate memory\n", __func__);
> @@ -162,9 +167,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> slot->root_handle = parent_context->root_handle;
> slot->pci_slot = pci_slot;
> INIT_LIST_HEAD(&slot->list);
> - mutex_lock(&slot_list_lock);
> list_add(&slot->list, &slot_list);
> - mutex_unlock(&slot_list_lock);
>
> get_device(&pci_bus->dev);
>
> @@ -274,7 +277,9 @@ acpi_pci_slot_add(struct acpi_pci_root *root)
> {
> acpi_status status;
>
> + mutex_lock(&slot_list_lock);
> status = walk_root_bridge(root, register_slot);
> + mutex_unlock(&slot_list_lock);
> if (ACPI_FAILURE(status))
> err("%s: register_slot failure - %d\n", __func__, status);
>
> @@ -330,17 +335,82 @@ static struct dmi_system_id acpi_pci_slot_dmi_table[] __initdata = {
> {}
> };
>
> +static void acpi_pci_slot_notify_add(struct pci_dev *dev)
> +{
> + acpi_handle handle;
> + struct callback_args context;
> +
> + if (!dev->subordinate)
> + return;
> +
> + mutex_lock(&slot_list_lock);
> + handle = DEVICE_ACPI_HANDLE(&dev->dev);
> + context.root_handle = acpi_find_root_bridge_handle(dev);
There's a patch under discussion that removes this function.
Isn't there any other way to do this?
> + if (handle && context.root_handle) {
> + context.pci_bus = dev->subordinate;
> + context.user_function = register_slot;
> + acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
You can just pass 1 here I think. Does the compiler complain?
> + register_slot, NULL, &context, NULL);
> + }
> + mutex_unlock(&slot_list_lock);
> +}
> +
> +static void acpi_pci_slot_notify_del(struct pci_dev *dev)
> +{
> + struct acpi_pci_slot *slot, *tmp;
> + struct pci_bus *bus = dev->subordinate;
> +
> + if (!bus)
> + return;
> +
> + mutex_lock(&slot_list_lock);
> + list_for_each_entry_safe(slot, tmp, &slot_list, list)
> + if (slot->pci_slot && slot->pci_slot->bus == bus) {
> + list_del(&slot->list);
> + pci_destroy_slot(slot->pci_slot);
> + put_device(&bus->dev);
> + kfree(slot);
> + }
> + mutex_unlock(&slot_list_lock);
> +}
> +
> +static int acpi_pci_slot_notify_fn(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct device *dev = data;
> +
> + switch (event) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + acpi_pci_slot_notify_add(to_pci_dev(dev));
> + break;
Do I think correctly that this is going to be called for every PCI device
added to the system, even if it's not a bridge?
> + case BUS_NOTIFY_DEL_DEVICE:
> + acpi_pci_slot_notify_del(to_pci_dev(dev));
> + break;
> + default:
> + return NOTIFY_DONE;
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block acpi_pci_slot_notifier = {
> + .notifier_call = &acpi_pci_slot_notify_fn,
> +};
> +
> static int __init
> acpi_pci_slot_init(void)
> {
> dmi_check_system(acpi_pci_slot_dmi_table);
> acpi_pci_register_driver(&acpi_pci_slot_driver);
> + bus_register_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
I wonder if/why this has to be so convoluted?
We have found a PCI bridge in the ACPI namespace, so we've created a struct
acpi_device for it and we've walked the namespace below it already.
Now we're creating a struct pci_dev for it and while registering it we're
going to walk the namespace below the bridge again to find and register its
slots and that is done indirectly from a bus type notifier.
Why can't we enumerate the slots directly upfront?
> +
> return 0;
> }
>
> static void __exit
> acpi_pci_slot_exit(void)
> {
> + bus_unregister_notifier(&pci_bus_type, &acpi_pci_slot_notifier);
> acpi_pci_unregister_driver(&acpi_pci_slot_driver);
> }
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
