Message-ID: <2704169.6ID6mDVPAt@vostro.rjw.lan>
Date:	Thu, 10 Jan 2013 22:50:59 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Myron Stowe <mstowe@...hat.com>
Cc:	Jiang Liu <liuj97@...il.com>, Jiang Liu <jiang.liu@...wei.com>,
	Yijing Wang <wangyijing@...wei.com>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	Myron Stowe <myron.stowe@...hat.com>,
	bhelgaas <bhelgaas@...gle.com>, Yinghai Lu <yinghai@...nel.org>
Subject: Re: [PATCH v3 3/6] ACPI/pci_slot: update PCI slot information when PCI hotplug event happens

On Thursday, January 10, 2013 02:24:23 PM Myron Stowe wrote:
> [+cc Yinghai]
> 
> On Wed, 2013-01-09 at 13:44 -0700, Bjorn Helgaas wrote:
> > [+cc Myron]
> > 
> > On Wed, Jan 9, 2013 at 1:19 PM, Rafael J. Wysocki <rjw@...k.pl> wrote:
> > > 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.
> > 
> > I agree that there's no value in supporting CONFIG_ACPI_PCI_SLOT=m.  I
> > think Myron has some patches that remove that case.
> > 
> > I'm not sure what the best way to merge them is.  We have a bunch of
> > stuff this cycle that touches both ACPI and PCI.
> 
> Rafael:
> 
> The series Bjorn mentions is at https://lkml.org/lkml/2012/12/7/11  It
> converts both the "ACPI Hot Plug PCI Controller Driver ("acpiphp")" and
> "ACPI PCI Slot Detection Driver ("pci_slot")" sub-drivers to built-in
> drivers (i.e. no longer supported as modules).
> 
> Yinghai commented back - https://lkml.org/lkml/2012/12/7/29 - indicating
> a possible issue with such but his response was too terse for me to get
> anything from.  Also, I've noticed that a couple of the distros, debian
> being one, have made similar changes so I'm a little skeptical about
> Yinghai's concerns.
> 
> Since you have similar thoughts - "I really don't see a value in having
> a modular pci_slot driver" - can you foresee any issues with such?

Well, I don't see what functional problems that can bring.

In theory people may want to have them as modules to avoid loading them on
systems that don't use PCI hotplug, but honestly I think that the complexity
this causes us to deal with is not worth it.

Moreover, removing the modularity may actually allow us to solve some ordering
issues once and for good.

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