Alex Chiang wrote:
* 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.
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
