diff mbox

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

Message ID 49D08A0A.2070102@jp.fujitsu.com
State Superseded, archived
Headers show

Commit Message

Taku Izumi March 30, 2009, 8:59 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.

<changelog>
 - change the symlink name from driver name to "driver" according to Kenji-san's
   comment.

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

Alexander Chiang March 30, 2009, 7:10 p.m. UTC | #1
* Taku Izumi <izumi.taku@jp.fujitsu.com>:
> 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.
> 
> <changelog>
>  - change the symlink name from driver name to "driver" according to Kenji-san's
>    comment.
> 
> 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))

Stylistically, you don't need those extra parens.

> +		return -ENODEV;
> +	if (slot->ops->owner)
> +		return 0;

The return value from this function seems backwards from the name
of the function.

Usually, a has_foo() or is_foo() convenience function returns a
non-zero value if the "foo" property is true.

In other words, when I read code like:

	if (has_owner_file(slot) == 0)

Then that makes me think "slot" does _not_ have an owner file.

Oh, I just read the earlier part of this thread. Eike Beer
complained about the same thing...

Would you mind creating a cleanup patch to fix these horrible
names? I know it's not your fault, and you're just trying to add
some useful functionality, but let's take the opportunity to get
it right.

Thanks.

/ac

> +	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,
> +					   "driver");
> +		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, "driver");
>  }
> 
>  static struct hotplug_slot *get_slot_from_name (const char *name)
--
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 June 1, 2009, 7:47 a.m. UTC | #2
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.
> 
> <changelog>
>  - change the symlink name from driver name to "driver" according to Kenji-san's
>    comment.
> 
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> 

I tested your patch and I found the following problems.

- Symbolic link is not created when the hotplug controller driver is
  configured as a built-in module. The reason is the "owner" field in
  struct hotplug_slot_ops (initialized by THIS_MODULE) is NULL in this
  case.

- Build error when CONFIG_MODULES is not set. The reason is struct
  module is not defined.

I'll send the updated patch soon.

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,
+					   "driver");
+		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, "driver");
 }

 static struct hotplug_slot *get_slot_from_name (const char *name)