* Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>:
> Matthew Wilcox wrote:
> > On Mon, Jun 01, 2009 at 04:53:29PM +0900, Kenji Kaneshige wrote:
> >> @@ -571,14 +578,16 @@ int pci_hp_register(struct hotplug_slot
> >> return -EINVAL;
> >> }
> >>
> >> - mutex_lock(&pci_hp_mutex);
> >> + slot->ops->owner = owner;
> >> + slot->ops->mod_name = mod_name;
> >>
> >> + mutex_lock(&pci_hp_mutex);
> >
> > Why not have the hotplug drivers fill in the ops->owner and ops->mod_name?
> > That would be the more common pattern within the kernel -- eg for
> > file_operations. There's only 9 places to change.
> >
>
> Yes, that's an another choice. The reason why I didn't change
> the hotplug driver side was I didn't like drivers to have the
> copied and pasted lines like below.
>
> struct hotplug_slot_ops foo {
> .owner = THIS_MODULE,
> .mod_name = KBUILD_MODNAME,
> ...
>
> Those are not for hotplug driver, but for PCI hotplug core.
> So I think that should not be visible to hotplug drivers as
> far as possible.
>
> If its more common pattern within the kernel, I'll follow it.
> But pci_register_driver() and usb_register(), for example, are
> implemented by the same pattern as my patch.
I think I prefer Kenji's approach, as it avoids copy n' paste
boilerplate. The hotplug drivers have enough of this already as
it is.
> >> Index: 20090529/drivers/pci/slot.c
> >> ===================================================================
> >> --- 20090529.orig/drivers/pci/slot.c
> >> +++ 20090529/drivers/pci/slot.c
> >> @@ -307,6 +307,50 @@ 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)
> >> +{
> >
> > I don't understand why you want to put these functions in the slot
> > driver rather than in the PCI hotplug core. Then they'd be private to
> > the hotplug core.
>
> I wanted to put those functions into PCI hotplug core. But
> to do this, we need to export symbols of 'module_kset' and
> 'kset_find_obj' since PCI hotplug core can be configured as
> loadable module. I couldn't imagine the other users of those
> symbols, and I felt exporting those symbols was excessive.
> So I didn't do that.
>
> IMHO, making PCI hotplug core always be configured built-in
> is one of choices.
I think the real issue here is that a struct pci_slot contains a
raw kobject, leading us to do very low-level kobject operations
on it.
The real fix would be to convert a struct pci_slot into a real
device, and then we wouldn't have to go poking our fingers into
places we don't belong.
That said, I don't think it's fair to ask Kenji to do the
conversion. We've already bike-shedded this patch a bit by asking
him (and Taku) to fix those horrible has_foo names, which he did
(and thank you for that).
I think the feature being proposed here is useful, and I don't
see a reason to delay it for these sorts of style issues. Let's
make some progress here, and we can have the goal of converting
struct pci_slot to a real device in the future to fix this
implementation issue.
I do wonder if this new symlink should be documented in
Documentation/ABI/ though.
> >> + struct hotplug_slot *slot = pci_slot->hotplug;
> >> + struct kobject *kobj = NULL;
> >> + int no_warn;
> >> +
> >> + if (!slot || !slot->ops)
> >> + return;
> >> + if (!slot->ops->owner)
> >> + kobj = kset_find_obj(module_kset, slot->ops->mod_name);
> >> +#ifdef CONFIG_MODULES
> >> + else
> >> + kobj = kobject_get(&slot->ops->owner->mkobj.kobj);
> >> +#endif /* CONFIG_MODULES */
> >
> > Ew. Surely there's a better way ... even if it's putting the kobject
> > in the slot ops.
>
> I cannot get the point. Could you give me details?
I'll let Matthew explain what he's asking for; if I try to guess,
I'm sure I'll get it wrong. ;)
Thanks.
/ac
--
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
