linux-kernel - Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <16794957.TmlpVGsGIb@vostro.rjw.lan>
Date:	Wed, 09 Jan 2013 21:19:17 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Jiang Liu <liuj97@...il.com>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>,
	Jiang Liu <jiang.liu@...wei.com>,
	Yijing Wang <wangyijing@...wei.com>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

On Thursday, January 10, 2013 12:58:25 AM Jiang Liu wrote:
> Hi Rafael,
> 	Thanks for your great efforts to review the patch.	
> 
> On 01/09/2013 08:01 AM, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Wednesday, January 09, 2013 12:52:22 AM Jiang Liu wrote:
> snip
> >>  
> >> +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?
> 	I will try to find a way to get rid of calling acpi_find_root_bridge_handle,
> and it seems doable.
> 
> > 
> >> +	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?
> Thanks for reminder, the (u32) is unnecessary.
> 
> > 
> >> +				    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?
> You are right. Function acpi_pci_slot_notify_add() and acpi_pci_slot_notify_del()
> will check whether it's a bridge. If preferred, I will move the check up into
> acpi_pci_slot_notify_fn().
> 
> > 
> >> +	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?
> Do you mean to create the PCI slot devices when creating the ACPI devices?
> I think there are two factors prevent us from doing that.
> The first is that the ACPI pci_slot driver could be built as a module, so
> we can't call into it from the ACPI core.

I didn't say about calling the pci_slot driver from the ACPI core, but about
enumerating slots in a way suitable for consumption by the pci_slot driver
when it's ready.

That said I really don't see a value in having a modular pci_slot driver.  It
is part of the hotplug infrastructure and should always be presend for this
reason, so we don't need to worry about the "pci_slot driver not present" case.

> The second reason is that the PCI slot is associated with a PCI bus, and the
> bus is only available until the PCI device has been created.

I suppose you mean "after"?  [I'm not sure if I agree with that, but whatever.]

We know which devices have slots before that happens, though.

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/