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.
>> 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.
>
>> + 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?
Thanks,
Kenji Kaneshige
--
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
