| Message ID | 49CC80C1.2090101@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Taku Izumi wrote: > This patch creates the symlink to the hotplug driver at the hotplug slot > directory (/sys/bus/pci/slots/<slot_name>). As a result, we can know easily > what driver manages the hotplug slot. > > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> > > --- > drivers/pci/hotplug/pci_hotplug_core.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > Index: linux-next.29rc6/drivers/pci/hotplug/pci_hotplug_core.c > =================================================================== > --- linux-next.29rc6.orig/drivers/pci/hotplug/pci_hotplug_core.c > +++ linux-next.29rc6/drivers/pci/hotplug/pci_hotplug_core.c > @@ -420,9 +420,20 @@ static int has_test_file(struct pci_slot > return -ENOENT; > } > > +static int has_owner_file(struct pci_slot *pci_slot) > +{ > + struct hotplug_slot *slot = pci_slot->hotplug; > + if ((!slot) || (!slot->ops)) > + return -ENODEV; > + if (slot->ops->owner) > + return 0; > + return -ENOENT; > +} Given the name of the function I would expect to return it either 0 (no slot there) or 1 (yes, there is one). On things like slot being NULL I would also expect an error code. Now with the given name you would do something like: if (!has_owner_file(foo)) which is very unintuitive. From just reading over that code it would mean exactly the opposite of what is expected. I see that the other functions around have the same interface so you are not to blame for this, but ... well, understanding this expression needs a look into the implementation of the has_foo() function. It also looks as the error code is never checked for, so this function might really return either 0 or 1 or bool. Eike
Hi Eike >> +static int has_owner_file(struct pci_slot *pci_slot) >> +{ >> + struct hotplug_slot *slot = pci_slot->hotplug; >> + if ((!slot) || (!slot->ops)) >> + return -ENODEV; >> + if (slot->ops->owner) >> + return 0; >> + return -ENOENT; >> +} > > Given the name of the function I would expect to return it either 0 (no slot > there) or 1 (yes, there is one). On things like slot being NULL I would also > expect an error code. Now with the given name you would do something like: > > if (!has_owner_file(foo)) > > which is very unintuitive. From just reading over that code it would mean > exactly the opposite of what is expected. > > I see that the other functions around have the same interface so you are not > to blame for this, but ... well, understanding this expression needs a look > into the implementation of the has_foo() function. It also looks as the error > code is never checked for, so this function might really return either 0 or 1 > or bool. You are exactly right. I think the function whose name is "can_xxx" or "is_xxx" or "has_xxx"　should return boolean value and non zero meas True. However in order to maintain consistency with other functions, please overlook it. Best regards, Taku Izumi -- 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
Taku Izumi wrote: > Hi Eike > > >> +static int has_owner_file(struct pci_slot *pci_slot) > >> +{ > >> + struct hotplug_slot *slot = pci_slot->hotplug; > >> + if ((!slot) || (!slot->ops)) > >> + return -ENODEV; > >> + if (slot->ops->owner) > >> + return 0; > >> + return -ENOENT; > >> +} > > > > Given the name of the function I would expect to return it either 0 (no > > slot there) or 1 (yes, there is one). On things like slot being NULL I > > would also expect an error code. Now with the given name you would do > > something like: > > > > if (!has_owner_file(foo)) > > > > which is very unintuitive. From just reading over that code it would mean > > exactly the opposite of what is expected. > > > > I see that the other functions around have the same interface so you are > > not to blame for this, but ... well, understanding this expression needs > > a look into the implementation of the has_foo() function. It also looks > > as the error code is never checked for, so this function might really > > return either 0 or 1 or bool. > > You are exactly right. I think the function whose name is "can_xxx" or > "is_xxx" or "has_xxx"?should return boolean value and non zero meas True. > However in order to maintain consistency with other functions, please > overlook it. As I said, you are not to blame for this ;) Maybe someone should cook up against -next to clean up that once and forever? Eike
Taku Izumi wrote: > This patch creates the symlink to the hotplug driver at the hotplug slot directory > (/sys/bus/pci/slots/<slot_name>). As a result, we can know easily what driver manages > the hotplug slot. > > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> > > --- > drivers/pci/hotplug/pci_hotplug_core.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > Index: linux-next.29rc6/drivers/pci/hotplug/pci_hotplug_core.c > =================================================================== > --- linux-next.29rc6.orig/drivers/pci/hotplug/pci_hotplug_core.c > +++ linux-next.29rc6/drivers/pci/hotplug/pci_hotplug_core.c > @@ -420,9 +420,20 @@ static int has_test_file(struct pci_slot > return -ENOENT; > } > > +static int has_owner_file(struct pci_slot *pci_slot) > +{ > + struct hotplug_slot *slot = pci_slot->hotplug; > + if ((!slot) || (!slot->ops)) > + return -ENODEV; > + if (slot->ops->owner) > + return 0; > + return -ENOENT; > +} > + > static int fs_add_slot(struct pci_slot *slot) > { > int retval = 0; > + struct hotplug_slot *hotplug = slot->hotplug; > > if (has_power_file(slot) == 0) { > retval = sysfs_create_file(&slot->kobj, &hotplug_slot_attr_power.attr); > @@ -472,8 +483,20 @@ static int fs_add_slot(struct pci_slot * > goto exit_test; > } > > + if (has_owner_file(slot) == 0) { > + retval = sysfs_create_link(&slot->kobj, > + &hotplug->ops->owner->mkobj.kobj, > + hotplug->ops->owner->name); I feel a little bit strange about using module name as a symbolic link name, because I cannot imagine what the symbolic link is for from its name. For example, I cannot imagine what the symbolic link named "pciehp" is for. What about using "hotplug_driver" as a symbolic link name? 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: linux-next.29rc6/drivers/pci/hotplug/pci_hotplug_core.c =================================================================== --- linux-next.29rc6.orig/drivers/pci/hotplug/pci_hotplug_core.c +++ linux-next.29rc6/drivers/pci/hotplug/pci_hotplug_core.c @@ -420,9 +420,20 @@ static int has_test_file(struct pci_slot return -ENOENT; } +static int has_owner_file(struct pci_slot *pci_slot) +{ + struct hotplug_slot *slot = pci_slot->hotplug; + if ((!slot) || (!slot->ops)) + return -ENODEV; + if (slot->ops->owner) + return 0; + return -ENOENT; +} + static int fs_add_slot(struct pci_slot *slot) { int retval = 0; + struct hotplug_slot *hotplug = slot->hotplug; if (has_power_file(slot) == 0) { retval = sysfs_create_file(&slot->kobj, &hotplug_slot_attr_power.attr); @@ -472,8 +483,20 @@ static int fs_add_slot(struct pci_slot * goto exit_test; } + if (has_owner_file(slot) == 0) { + retval = sysfs_create_link(&slot->kobj, + &hotplug->ops->owner->mkobj.kobj, + hotplug->ops->owner->name); + if (retval) + goto exit_owner; + } + goto exit; +exit_owner: + if (has_test_file(slot) == 0) + sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_test.attr); + exit_test: if (has_cur_bus_speed_file(slot) == 0) sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_cur_bus_speed.attr); @@ -524,6 +547,9 @@ static void fs_remove_slot(struct pci_sl if (has_test_file(slot) == 0) sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_test.attr); + + if (has_owner_file(slot) == 0) + sysfs_remove_link(&slot->kobj, slot->hotplug->ops->owner->name); } static struct hotplug_slot *get_slot_from_name (const char *name)
This patch creates the symlink to the hotplug driver at the hotplug slot directory (/sys/bus/pci/slots/<slot_name>). As a result, we can know easily what driver manages the hotplug slot. Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- drivers/pci/hotplug/pci_hotplug_core.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) -- 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