| Message ID | 4A2388F9.2050402@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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. > 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. > + 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.
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>: > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Alex Chiang wrote: > * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>: >> 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. > I plan to make one more patch that removes ".owner = THIS_MODULE," lines from hotplug controller drivers in the next version. >>>> 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. Ok, I'll add the documentation. > >>>> + 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. ;) > Thank you :) BTW, I thought just doing kobj = kset_find_obj(module_kset, slot->ops->mod_name); if (!kobj) return; ... would be easier to read, when I made this patch. Is that the same as your concern, Matthew-san? Thanks, Kenji Kaneshige -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: 20090529/drivers/pci/hotplug/pci_hotplug_core.c =================================================================== --- 20090529.orig/drivers/pci/hotplug/pci_hotplug_core.c +++ 20090529/drivers/pci/hotplug/pci_hotplug_core.c @@ -424,6 +424,9 @@ static int fs_add_slot(struct pci_slot * { int retval = 0; + /* Create symbolic link to the hotplug driver module */ + pci_hp_create_module_link(slot); + if (has_power_file(slot)) { retval = sysfs_create_file(&slot->kobj, &hotplug_slot_attr_power.attr); @@ -498,6 +501,7 @@ exit_attention: if (has_power_file(slot)) sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_power.attr); exit_power: + pci_hp_remove_module_link(slot); exit: return retval; } @@ -528,6 +532,8 @@ static void fs_remove_slot(struct pci_sl if (has_test_file(slot)) sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_test.attr); + + pci_hp_remove_module_link(slot); } static struct hotplug_slot *get_slot_from_name (const char *name) @@ -544,10 +550,10 @@ static struct hotplug_slot *get_slot_fro } /** - * pci_hp_register - register a hotplug_slot with the PCI hotplug subsystem + * __pci_hp_register - register a hotplug_slot with the PCI hotplug subsystem * @bus: bus this slot is on * @slot: pointer to the &struct hotplug_slot to register - * @slot_nr: slot number + * @devnr: device number * @name: name registered with kobject core * * Registers a hotplug slot with the pci hotplug subsystem, which will allow @@ -555,8 +561,9 @@ static struct hotplug_slot *get_slot_fro * * Returns 0 if successful, anything else for an error. */ -int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr, - const char *name) +int __pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, + int devnr, const char *name, + struct module *owner, const char *mod_name) { int result; struct pci_slot *pci_slot; @@ -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); /* * No problems if we call this interface from both ACPI_PCI_SLOT * driver and call it here again. If we've already created the * pci_slot, the interface will simply bump the refcount. */ - pci_slot = pci_create_slot(bus, slot_nr, name, slot); + pci_slot = pci_create_slot(bus, devnr, name, slot); if (IS_ERR(pci_slot)) { result = PTR_ERR(pci_slot); goto out; @@ -688,6 +697,6 @@ MODULE_LICENSE("GPL"); module_param(debug, bool, 0644); MODULE_PARM_DESC(debug, "Debugging mode enabled or not"); -EXPORT_SYMBOL_GPL(pci_hp_register); +EXPORT_SYMBOL_GPL(__pci_hp_register); EXPORT_SYMBOL_GPL(pci_hp_deregister); EXPORT_SYMBOL_GPL(pci_hp_change_slot_info); Index: 20090529/include/linux/pci_hotplug.h =================================================================== --- 20090529.orig/include/linux/pci_hotplug.h +++ 20090529/include/linux/pci_hotplug.h @@ -77,6 +77,7 @@ struct hotplug_slot_attribute { /** * struct hotplug_slot_ops -the callbacks that the hotplug pci core can use * @owner: The module owner of this structure + * @mod_name: The module name (KBUILD_MODNAME) of this structure * @enable_slot: Called when the user wants to enable a specific pci slot * @disable_slot: Called when the user wants to disable a specific pci slot * @set_attention_status: Called to set the specific slot's attention LED to @@ -109,6 +110,7 @@ struct hotplug_slot_attribute { */ struct hotplug_slot_ops { struct module *owner; + const char *mod_name; int (*enable_slot) (struct hotplug_slot *slot); int (*disable_slot) (struct hotplug_slot *slot); int (*set_attention_status) (struct hotplug_slot *slot, u8 value); @@ -167,12 +169,21 @@ static inline const char *hotplug_slot_n return pci_slot_name(slot->pci_slot); } -extern int pci_hp_register(struct hotplug_slot *, struct pci_bus *, int nr, - const char *name); +extern int __pci_hp_register(struct hotplug_slot *slot, struct pci_bus *pbus, + int nr, const char *name, + struct module *owner, const char *mod_name); extern int pci_hp_deregister(struct hotplug_slot *slot); extern int __must_check pci_hp_change_slot_info (struct hotplug_slot *slot, struct hotplug_slot_info *info); +static inline int pci_hp_register(struct hotplug_slot *slot, + struct pci_bus *pbus, + int devnr, const char *name) +{ + return __pci_hp_register(slot, pbus, devnr, name, + THIS_MODULE, KBUILD_MODNAME); +} + /* PCI Setting Record (Type 0) */ struct hpp_type0 { u32 revision; 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) +{ + 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 */ + if (!kobj) + return; + no_warn = sysfs_create_link(&pci_slot->kobj, kobj, "module"); + kobject_put(kobj); +} +EXPORT_SYMBOL_GPL(pci_hp_create_module_link); + +/** + * pci_hp_remove_link - remove symbolic link to the hotplug driver module. + * @slot: struct pci_slot + * + * Helper function for pci_hotplug_core.c to remove symbolic link to + * the hotplug driver module. + */ +void pci_hp_remove_module_link(struct pci_slot *pci_slot) +{ + sysfs_remove_link(&pci_slot->kobj, "module"); +} +EXPORT_SYMBOL_GPL(pci_hp_remove_module_link); +#endif + static int pci_slot_init(void) { struct kset *pci_bus_kset; Index: 20090529/include/linux/pci.h =================================================================== --- 20090529.orig/include/linux/pci.h +++ 20090529/include/linux/pci.h @@ -1249,5 +1249,10 @@ static inline irqreturn_t pci_sriov_migr } #endif +#if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE) +extern void pci_hp_create_module_link(struct pci_slot *pci_slot); +extern void pci_hp_remove_module_link(struct pci_slot *pci_slot); +#endif + #endif /* __KERNEL__ */ #endif /* LINUX_PCI_H */