lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <1493550.As37othyQz@vostro.rjw.lan> Date: Sun, 13 Jan 2013 00:18:12 +0100 From: "Rafael J. Wysocki" <rjw@...k.pl> To: Yinghai Lu <yinghai@...nel.org> Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Len Brown <lenb@...nel.org>, Taku Izumi <izumi.taku@...fujitsu.com>, Jiang Liu <jiang.liu@...wei.com>, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org Subject: Re: [PATCH v8 08/22] PCI, acpiphp: Separate out hot-add support of pci host bridge On Friday, January 11, 2013 02:40:35 PM Yinghai Lu wrote: > It causes confusion. > > We may only need acpi hp for pci host bridge. What does this mean? > Split host bridge hot-add support to pci_root_hp, and keep acpiphp simple. s/Split/Move/ I suppose? In any case that's not telling the whole story, because the patch doesn't just move code from one file to another. > -v2: put back pci_root_hp change in one patch > -v3: add pcibios_resource_survey_bus() calling > -v4: remove not needed code with remove_bridge > -v5: put back support for acpiphp support for slots just on root bus. > -v6: change some functions to *_p2p_* to make it more clean. > -v7: split hot_added change out. > -v8: Move to pci_root.c instead of adding another file requested by Bjorn. > > Signed-off-by: Yinghai Lu <yinghai@...nel.org> > --- > drivers/acpi/pci_root.c | 220 ++++++++++++++++++++++++++++++++++++ > drivers/pci/hotplug/acpiphp_glue.c | 59 +++------- > 2 files changed, 235 insertions(+), 44 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 471b2dc..5c1f462c 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -673,3 +673,223 @@ int __init acpi_pci_root_init(void) > > return 0; > } > + > +/* > + * Separated from drivers/pci/hotplug/acpiphp_glue.c > + * only support root bridge > + */ This comment will be useless after applying the patch. > + > +static LIST_HEAD(acpi_root_bridge_list); > +struct acpi_root_bridge { > + struct list_head list; > + acpi_handle handle; > + u32 flags; > +}; We have struct acpi_pci_root already. Why do we need this in addition? Also, we have acpi_pci_roots, so why do we need another list of root bridges? > + > +/* bridge flags */ > +#define ROOT_BRIDGE_HAS_EJ0 (0x00000002) > +#define ROOT_BRIDGE_HAS_PS3 (0x00000080) What is that needed for? > + > +#define ACPI_STA_FUNCTIONING (0x00000008) > + > +static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle) > +{ > + struct acpi_root_bridge *bridge; > + > + list_for_each_entry(bridge, &acpi_root_bridge_list, list) > + if (bridge->handle == handle) > + return bridge; > + > + return NULL; > +} > + > +/* allocate and initialize host bridge data structure */ > +static void add_acpi_root_bridge(acpi_handle handle) > +{ > + struct acpi_root_bridge *bridge; > + acpi_handle dummy_handle; > + acpi_status status; > + Why do we need to evaluate all of the methods directly here? Don't we have a struct acpi_device for handle already? > + /* if the bridge doesn't have _STA, we assume it is always there */ > + status = acpi_get_handle(handle, "_STA", &dummy_handle); > + if (ACPI_SUCCESS(status)) { > + unsigned long long tmp; > + > + status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp); > + if (ACPI_FAILURE(status)) { > + printk(KERN_DEBUG "%s: _STA evaluation failure\n", > + __func__); > + return; > + } > + if ((tmp & ACPI_STA_FUNCTIONING) == 0) > + /* don't register this object */ > + return; > + } > + > + bridge = kzalloc(sizeof(struct acpi_root_bridge), GFP_KERNEL); > + if (!bridge) > + return; > + > + bridge->handle = handle; > + > + if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle))) > + bridge->flags |= ROOT_BRIDGE_HAS_EJ0; > + if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle))) > + bridge->flags |= ROOT_BRIDGE_HAS_PS3; All of this attempts to duplicate the scanning code from scan.c in a very incomplete and questionable way. For example, what if the root bridge has _PR0? > + > + list_add(&bridge->list, &acpi_root_bridge_list); > +} > + > +struct acpi_root_hp_work { > + struct work_struct work; > + acpi_handle handle; > + u32 type; > + void *context; > +}; > + > +static void alloc_acpi_root_hp_work(acpi_handle handle, u32 type, > + void *context, > + void (*func)(struct work_struct *work)) > +{ > + struct acpi_root_hp_work *hp_work; > + int ret; > + > + hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL); > + if (!hp_work) > + return; > + > + hp_work->handle = handle; > + hp_work->type = type; > + hp_work->context = context; > + > + INIT_WORK(&hp_work->work, func); > + ret = queue_work(kacpi_hotplug_wq, &hp_work->work); > + if (!ret) > + kfree(hp_work); > +} The function above is called only once and used by __init stuff only. Why don't we move it to the caller and mark that caller as __init too? > + > +static void handle_root_bridge_insertion(acpi_handle handle) > +{ > + struct acpi_device *device, *pdevice; > + acpi_handle phandle; > + int ret_val; > + > + 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 */ > + struct acpi_pci_root *root = acpi_driver_data(device); > + if (pci_find_bus(root->segment, root->secondary.start)) > + return; > + > + printk(KERN_DEBUG "bus exists... trim\n"); > + /* this shouldn't be in here, so remove > + * the bus then re-add it... > + */ Why? Shouldn't we just bail out here? > + /* remove pci devices at first */ > + ret_val = acpi_bus_trim(device, 0); > + printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val); > + /* remove acpi devices */ > + ret_val = acpi_bus_trim(device, 1); Oh, I see why you need the second argument of acpi_bus_trim() now. Do I think correctly that you want acpi_pci_root_remove() to be executed before all of the struct acpi_device objects are removed? In which case why don't we call acpi_pci_root_remove() directly before doing the acpi_bus_trim(device, 1) instead of making the code next to impossible to understand? > + printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val); > + } > + if (acpi_bus_add(handle)) > + printk(KERN_ERR "cannot add bridge to acpi list\n"); > +} > + > +static void _handle_hotplug_event_root(struct work_struct *work) > +{ > + struct acpi_root_bridge *bridge; > + char objname[64]; > + struct acpi_buffer buffer = { .length = sizeof(objname), > + .pointer = objname }; > + struct acpi_root_hp_work *hp_work; > + acpi_handle handle; > + u32 type; > + > + hp_work = container_of(work, struct acpi_root_hp_work, work); > + handle = hp_work->handle; > + type = hp_work->type; > + > + bridge = acpi_root_handle_to_bridge(handle); > + > + acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > + > + switch (type) { > + case ACPI_NOTIFY_BUS_CHECK: > + /* bus enumerate */ > + printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__, > + objname); > + if (!bridge) { > + handle_root_bridge_insertion(handle); I don't think we should call add_acpi_root_bridge() for handle if the above fails. So probably handle_root_bridge_insertion() should return error codes? > + add_acpi_root_bridge(handle); > + } > + > + break; > + > + case ACPI_NOTIFY_DEVICE_CHECK: > + /* device check */ > + printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__, > + objname); > + if (!bridge) { > + handle_root_bridge_insertion(handle); > + add_acpi_root_bridge(handle); > + } > + break; > + > + default: > + printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n", > + type, objname); > + break; > + } > + > + kfree(hp_work); /* allocated in handle_hotplug_event_bridge */ > +} > + > +static void handle_hotplug_event_root(acpi_handle handle, u32 type, > + void *context) > +{ > + alloc_acpi_root_hp_work(handle, type, context, > + _handle_hotplug_event_root); > +} > + > +static acpi_status __init > +find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv) > +{ > + char objname[64]; > + struct acpi_buffer buffer = { .length = sizeof(objname), > + .pointer = objname }; > + int *count = (int *)context; > + > + if (!acpi_is_root_bridge(handle)) > + return AE_OK; > + > + (*count)++; > + > + acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > + > + acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY, > + handle_hotplug_event_root, NULL); > + printk(KERN_DEBUG "acpi root: %s notify handler installed\n", objname); > + > + add_acpi_root_bridge(handle); > + > + return AE_OK; > +} > + > +static int __init acpi_pci_root_hp_init(void) > +{ > + int num = 0; > + > + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, > + ACPI_UINT32_MAX, find_root_bridges, NULL, &num, NULL); > + > + printk(KERN_DEBUG "Found %d acpi root devices\n", num); > + > + return 0; > +} Why do we need to do that from an initcall? Couldn't we simply hook up that code to acpi_pci_root_add() somewhere? And even if not, why don't we call acpi_pci_root_hp_init() from acpi_pci_root_init()? All of the changes in acpiphp_glue.c look reasonable to me. 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-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists