diff mbox

pci_hotplug: create the symlink to the hotplug driver at the hotplug slot directory

Message ID 49CC80C1.2090101@jp.fujitsu.com
State Superseded, archived
Headers show

Commit Message

Taku Izumi March 27, 2009, 7:31 a.m. UTC
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

Comments

Rolf Eike Beer March 27, 2009, 7:42 a.m. UTC | #1
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
Taku Izumi March 27, 2009, 8:25 a.m. UTC | #2
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
Rolf Eike Beer March 27, 2009, 9:11 a.m. UTC | #3
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
Kenji Kaneshige March 27, 2009, 3:46 p.m. UTC | #4
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
diff mbox

Patch

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)