On Mon, Jun 15, 2009 at 01:20:36PM -0600, Alex Chiang wrote:
> * Greg KH <greg@xxxxxxxxx>:
> > On Mon, Jun 15, 2009 at 11:52:31AM -0600, Alex Chiang wrote:
> > > * Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>:
> > > > Create symbolic link to hotplug driver module in the PCI slot
> > > > directory (/sys/bus/pci/slots/<SLOT#>). In the past, we need to load
> > > > hotplug drivers one by one to identify the hotplug driver that handles
> > > > the slot, and it was very inconvenient especially for trouble shooting.
> > > > With this change, we can easily identify the hotplug driver.
> > >
> > > I think this implementation is much nicer than the previous one.
> > >
> > > One comment below regarding the symlink name.
> > >
> > > > Index: 20090612/drivers/pci/slot.c
> > > > ===================================================================
> > > > --- 20090612.orig/drivers/pci/slot.c
> > > > +++ 20090612/drivers/pci/slot.c
> > > > @@ -307,6 +307,45 @@ void pci_destroy_slot(struct pci_slot *s
> > > > }
> > > > EXPORT_SYMBOL_GPL(pci_destroy_slot);
> > > >
> > > > +#if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
> > > > +#include <linux/pci_hotplug.h>
> > > > +/**
> > > > + * pci_hp_create_link - create symbolic link to the hotplug driver module.
> > > > + * @slot: struct pci_slot
> > > > + *
> > > > + * Helper function for pci_hotplug_core.c to create symbolic link to
> > > > + * the hotplug driver module.
> > > > + */
> > > > +void pci_hp_create_module_link(struct pci_slot *pci_slot)
> > > > +{
> > > > + struct hotplug_slot *slot = pci_slot->hotplug;
> > > > + struct kobject *kobj = NULL;
> > > > + int no_warn;
> > > > +
> > > > + if (!slot || !slot->ops)
> > > > + return;
> > > > + kobj = kset_find_obj(module_kset, slot->ops->mod_name);
> > > > + if (!kobj)
> > > > + return;
> > > > + no_warn = sysfs_create_link(&pci_slot->kobj, kobj, "module");
> > >
> > > I would prefer to call this link "hotplug_module" or "hp_module".
> > > The reason is, if we ever decide to create another link for that
> > > slot's low level device driver, we won't be limited by something
> > > that already called "module".
> >
> > Note that "module" is already used in sysfs, so as long as you are
> > pointing to the same thing that those other symlinks are, you should be
> > fine. You are pointing to /sys/module/NAME, here, right?
>
> Looks like the common use of "module" is:
>
> /sys/bus/$bus/drivers/$driver/module -> /sys/module/NAME
>
> What I was reacting to is that this patch proposes:
>
> /sys/bus/pci/slots/$slot/module -> /sys/module/NAME
Which would be correct.
> This scheme is fine as long as only PCI hotplug modules create
> those $slot entries.
What else would ever create a pci slot?
> But it is limiting, if in the future, we teach the PCI core to
> create $slot entries when a low level device driver, say tg3 or
> whatever, is loaded.
Then the pci core would be the module there, right? Not the pci driver
that controls the device in the slot.
> In that scenario, it would be more logical if module pointed at
> the LLDD's module, not whatever PCI hotplug module happens to
> claim that slot
Huh? I'm confused now, why not just leave the code alone in such a
situation, having the module symlink point to the module that controls
the slot?
> Since we're talking about an ABI agreement with userspace, I
> think "hotplug_module" is better for future flexibility.
I disagree, "module" is already part of the ABI, let's not create
something that does the exact same thing, yet is named different :)
thanks,
greg k-h
--
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
