[PATCH 0/3] gpiolib: gpio naming in sysfs

[PATCH 0/3] gpiolib: gpio naming in sysfs

[Jani Nikula]

Hi David and Greg -

There's been debate in the past about naming gpios exported to the
sysfs. Long story short, there are users for that, and there are now
two ways of naming gpios in the sysfs: char **names in struct
gpio_chip [1], and gpio_export_link() [2].

This patchset combines these two by allowing gpio_export_link() to
have dev == NULL to make the link under gpiolib sysfs (instead of
arbitrary device), and to use gpio_chip names to create links (instead
of naming the actual devices with those). This gpio_export_link() with
dev == NULL would also be useful for gpios not associated with a
driver.

Greg, patch 1/3 introduces class_{create,remove}_link(), is that
acceptable?

CC Daniel, you introduced names in struct gpio_chip in the first
place, does this (especially patch 3/3) look acceptable to you?


BR,
Jani.


[1] commit 926b663ce8215ba448960e1ff6e58b67a2c3b99b
[2] commit a4177ee7f1a83eecb1d75e85d32664b023ef65e9


Artem Bityutskiy (1):
  device class: add symlink creation helpers

Jani Nikula (2):
  gpiolib: add support for having symlinks under gpio class directory
  gpiolib: use chip->names for symlinks, always use gpioN for device
    names

 drivers/base/class.c   |   21 ++++++++++
 drivers/gpio/gpiolib.c |  101 +++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/device.h |    4 ++
 3 files changed, 120 insertions(+), 6 deletions(-)

[PATCH 1/3] device class: add symlink creation helpers

[Jani Nikula]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/base/class.c   |   21 +++++++++++++++++++++
 include/linux/device.h |    4 ++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index 161746d..818cbe2 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -92,6 +92,25 @@ void class_remove_file(struct class *cls, const struct class_attribute *attr)
 		sysfs_remove_file(&cls->p->class_subsys.kobj, &attr->attr);
 }
 
+int class_create_link(struct class *cls, struct kobject *target,
+		      const char *name)
+{
+	int error;
+
+	if (cls)
+		error = sysfs_create_link(&cls->p->class_subsys.kobj,
+					  target, name);
+	else
+		error = -EINVAL;
+	return error;
+}
+
+void class_remove_link(struct class *cls, const char *name)
+{
+	if (cls)
+		sysfs_remove_link(&cls->p->class_subsys.kobj, name);
+}
+
 static struct class *class_get(struct class *cls)
 {
 	if (cls)
@@ -585,6 +604,8 @@ int __init classes_init(void)
 
 EXPORT_SYMBOL_GPL(class_create_file);
 EXPORT_SYMBOL_GPL(class_remove_file);
+EXPORT_SYMBOL_GPL(class_create_link);
+EXPORT_SYMBOL_GPL(class_remove_link);
 EXPORT_SYMBOL_GPL(class_unregister);
 EXPORT_SYMBOL_GPL(class_destroy);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 2ea3e49..e877be6 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -262,6 +262,10 @@ extern int __must_check class_create_file(struct class *class,
 					  const struct class_attribute *attr);
 extern void class_remove_file(struct class *class,
 			      const struct class_attribute *attr);
+extern int __must_check class_create_link(struct class *class,
+					  struct kobject *target,
+					  const char *name);
+extern void class_remove_link(struct class *class, const char *name);
 
 struct class_interface {
 	struct list_head	node;
-- 
1.6.5.2

[PATCH 2/3] gpiolib: add support for having symlinks under gpio class directory

[Jani Nikula]

Extend the functionality of gpio_export_link() to allow exported GPIOs
to have names using sysfs links under /sys/class/gpio.

Also automatically remove links on gpio_unexport().

Signed-off-by: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
 drivers/gpio/gpiolib.c |   95 +++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 90 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 50de0f5..f254195 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -9,6 +9,8 @@
 #include <linux/seq_file.h>
 #include <linux/gpio.h>
 #include <linux/idr.h>
+#include <linux/list.h>
+#include "../base/base.h"
 
 
 /* Optional implementation infrastructure for GPIO interfaces.
@@ -608,6 +610,84 @@ static struct class gpio_class = {
 	.class_attrs =	gpio_class_attrs,
 };
 
+/* sysfs link */
+struct sysfs_link {
+	unsigned gpio;
+	struct device *dev;
+	const char *name;
+	struct list_head node;
+};
+
+/* list of links to gpio sysfs nodes, protected by sysfs_mutex */
+static LIST_HEAD(sysfs_links);
+
+/* create link, store info */
+static int create_link(unsigned gpio, struct device *dev,
+		struct kobject *target, const char *name)
+{
+	struct sysfs_link	*link;
+	int			status = -ENOMEM;
+
+	link = kzalloc(sizeof(*link), GFP_KERNEL);
+	if (link == NULL)
+		goto err0;
+
+	link->name = kstrdup(name, GFP_KERNEL);
+	if (link->name == NULL)
+		goto err1;
+
+	link->gpio = gpio;
+	link->dev = dev;
+
+	if (dev != NULL) {
+		dev = get_device(dev);
+		if (dev == NULL) {
+			status = -ENODEV;
+			goto err2;
+		}
+		status = sysfs_create_link(&dev->kobj, target, name);
+	} else {
+		status = class_create_link(&gpio_class, target, name);
+	}
+
+	if (status)
+		goto err3;
+
+	list_add(&link->node, &sysfs_links);
+
+	return 0;
+
+err3:
+	if (dev != NULL)
+		put_device(dev);
+err2:
+	kfree(link->name);
+err1:
+	kfree(link);
+err0:
+
+	return status;
+}
+
+/* remove symlinks pointing to gpio */
+static void remove_links(unsigned gpio)
+{
+	struct sysfs_link	*link, *tmp;
+
+	list_for_each_entry_safe(link, tmp, &sysfs_links, node) {
+		if (link->gpio == gpio) {
+			if (link->dev != NULL) {
+				sysfs_remove_link(&link->dev->kobj, link->name);
+				put_device(link->dev);
+			} else {
+				class_remove_link(&gpio_class, link->name);
+			}
+			list_del(&link->node);
+			kfree(link->name);
+			kfree(link);
+		}
+	}
+}
 
 /**
  * gpio_export - export a GPIO through sysfs
@@ -701,12 +781,17 @@ static int match_export(struct device *dev, void *data)
 
 /**
  * gpio_export_link - create a sysfs link to an exported GPIO node
- * @dev: device under which to create symlink
+ * @dev: device under which to create symlink, or NULL for gpio class
  * @name: name of the symlink
  * @gpio: gpio to create symlink to, already exported
  *
- * Set up a symlink from /sys/.../dev/name to /sys/class/gpio/gpioN
- * node. Caller is responsible for unlinking.
+ * If @dev is non-NULL, set up a symlink from /sys/.../dev/name to
+ * /sys/class/gpio/gpioN node.
+ *
+ * If @dev is NULL, set up the symlink from /sys/class/gpio/name to
+ * /sys/class/gpio/gpioN node.
+ *
+ * Symlinks are removed on gpio_unexport() on the @gpio.
  *
  * Returns zero on success, else an error.
  */
@@ -727,8 +812,7 @@ int gpio_export_link(struct device *dev, const char *name, unsigned gpio)
 
 		tdev = class_find_device(&gpio_class, NULL, desc, match_export);
 		if (tdev != NULL) {
-			status = sysfs_create_link(&dev->kobj, &tdev->kobj,
-						name);
+			status = create_link(gpio, dev, &tdev->kobj, name);
 		} else {
 			status = -ENODEV;
 		}
@@ -769,6 +853,7 @@ void gpio_unexport(unsigned gpio)
 		if (dev) {
 			gpio_setup_irq(desc, dev, 0);
 			clear_bit(FLAG_EXPORT, &desc->flags);
+			remove_links(gpio);
 			put_device(dev);
 			device_unregister(dev);
 			status = 0;
-- 
1.6.5.2

[PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Jani Nikula]

Restore gpiolib sysfs device naming back to /sys/class/gpio/gpioN, where
N is the GPIO number. Use the chip->names mechanism for creating
symlinks to those devices instead of creating the devices using
chip->names. All GPIO naming (other than gpioN) in gpiolib sysfs is now
done using symlinks.

While the device names are now unique again, collisions in chip->names
will still prevent duplicates from being exported to sysfs.

Signed-off-by: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
 drivers/gpio/gpiolib.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f254195..fe7ffad 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -740,7 +740,7 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 		struct device	*dev;
 
 		dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
-				desc, ioname ? ioname : "gpio%d", gpio);
+				desc, "gpio%d", gpio);
 		if (!IS_ERR(dev)) {
 			if (direction_may_change)
 				status = sysfs_create_group(&dev->kobj,
@@ -756,6 +756,10 @@ int gpio_export(unsigned gpio, bool direction_may_change)
 				status = device_create_file(dev,
 						&dev_attr_edge);
 
+			if (!status && ioname != NULL)
+				status = create_link(gpio, NULL, &dev->kobj,
+						ioname);
+
 			if (status != 0)
 				device_unregister(dev);
 		} else
-- 
1.6.5.2

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[David Brownell]

On Wednesday 09 December 2009, Jani Nikula wrote:
> While the device names are now unique again, collisions in chip->names
> will still prevent duplicates from being exported to sysfs.

That's why I was never a real fan of chip->names...

IMO a "good" solution in this space needs to accept that
those names are not going to be globally unique ... but
that they'll be unique within some context, of necessity.

If Greg doesn't want to see those names under classes,
so be it ... but where should they then appear?

- Dave

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Greg KH]

On Thu, Dec 10, 2009 at 07:39:30PM -0800, David Brownell wrote:
> On Wednesday 09 December 2009, Jani Nikula wrote:
> > While the device names are now unique again, collisions in chip->names
> > will still prevent duplicates from being exported to sysfs.
> 
> That's why I was never a real fan of chip->names...
> 
> IMO a "good" solution in this space needs to accept that
> those names are not going to be globally unique ... but
> that they'll be unique within some context, of necessity.
> 
> If Greg doesn't want to see those names under classes,
> so be it ... but where should they then appear?

As a sysfs file within the device directory called 'name'?  Then just
grep through the tree to find the right device, that also handles
duplicates just fine, right?

thanks,

greg k-h

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[David Brownell]

On Thursday 10 December 2009, Greg KH wrote:
> 
> > IMO a "good" solution in this space needs to accept that
> > those names are not going to be globally unique ... but
> > that they'll be unique within some context, of necessity.
> > 
> > If Greg doesn't want to see those names under classes,
> > so be it ... but where should they then appear?
> 
> As a sysfs file within the device directory called 'name'?  Then just
> grep through the tree to find the right device, that also handles
> duplicates just fine, right?

I want a concrete example.  Those chip->names things don't
seem helpful to me though...

If for example I were building a JTAG adapter on Linux, it
might consist of a spidev node (chardev) plus a handful of
GPIOs.  So "the device directory" would be the sysfs home
of that spidev node (or some variant)?  And inside that
directory would be files named after various signals that
are used as GPIOs ... maybe SRST, TRST, and DETECT to start
with?  Holding some cookie that gets mapped to those GPIO's
sysfs entries?

I confess I'd still think a symlink from that directory
to the real GPIO would be easier to work with...

- Dave

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Greg KH]

On Thu, Dec 10, 2009 at 08:13:58PM -0800, David Brownell wrote:
> On Thursday 10 December 2009, Greg KH wrote:
> > 
> > > IMO a "good" solution in this space needs to accept that
> > > those names are not going to be globally unique ... but
> > > that they'll be unique within some context, of necessity.
> > > 
> > > If Greg doesn't want to see those names under classes,
> > > so be it ... but where should they then appear?
> > 
> > As a sysfs file within the device directory called 'name'? ?Then just
> > grep through the tree to find the right device, that also handles
> > duplicates just fine, right?
> 
> I want a concrete example.  Those chip->names things don't
> seem helpful to me though...
> 
> If for example I were building a JTAG adapter on Linux, it
> might consist of a spidev node (chardev) plus a handful of
> GPIOs.  So "the device directory" would be the sysfs home
> of that spidev node (or some variant)?  And inside that
> directory would be files named after various signals that
> are used as GPIOs ... maybe SRST, TRST, and DETECT to start
> with?  Holding some cookie that gets mapped to those GPIO's
> sysfs entries?

Um, I really don't know, as I don't know the GPIO subsystem, nor why you
all have this problem in the first place :)

I also find it funny that you think changing the kernel is easier than
userspace, that's a strange situation.

Anyway, I assumed that you already have a struct device for the GPIO
devices, right?  Put it in there was what I was thinking, but as I don't
understand your current layout, I really don't know.

> I confess I'd still think a symlink from that directory
> to the real GPIO would be easier to work with...

No, don't do that, no symlinks from a class please.

thanks,

greg k-h

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[David Brownell]

On Thursday 10 December 2009, Greg KH wrote:
> On Thu, Dec 10, 2009 at 08:13:58PM -0800, David Brownell wrote:
> > On Thursday 10 December 2009, Greg KH wrote:
> > > 
> > > > IMO a "good" solution in this space needs to accept that
> > > > those names are not going to be globally unique ... but
> > > > that they'll be unique within some context, of necessity.
> > > > 
> > > > If Greg doesn't want to see those names under classes,
> > > > so be it ... but where should they then appear?
> > > 
> > > As a sysfs file within the device directory called 'name'? ?Then just
> > > grep through the tree to find the right device, that also handles
> > > duplicates just fine, right?
> > 
> > I want a concrete example.  Those chip->names things don't
> > seem helpful to me though...
> > 
> > ...
> 
> Um, I really don't know, as I don't know the GPIO subsystem, nor why you
> all have this problem in the first place :)

Maybe Jani can provide a more concrete example.


> I also find it funny that you think changing the kernel is easier than
> userspace, that's a strange situation.

I don't recall saying that.  :)

It's a case of kernel having access to system data that's not
otherwise exported to userspace.  It knows how the various bits
of hardware fit together ... and in this case wants to export
associations between some GPIOs and some other hardware.  Given
that, userspace can pick things up.

 
> Anyway, I assumed that you already have a struct device for the GPIO
> devices, right?  Put it in there was what I was thinking, but as I don't
> understand your current layout, I really don't know.

There are gpio_chip devices, for a set of GPIOs.  But not for
the individual GPIOs.


> > I confess I'd still think a symlink from that directory
> > to the real GPIO would be easier to work with...
> 
> No, don't do that, no symlinks from a class please.

I didn't catch a reason for that request... could you
explain that?

- Dave

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Greg KH]

On Thu, Dec 10, 2009 at 09:13:06PM -0800, David Brownell wrote:
> On Thursday 10 December 2009, Greg KH wrote:
> > On Thu, Dec 10, 2009 at 08:13:58PM -0800, David Brownell wrote:
> > > On Thursday 10 December 2009, Greg KH wrote:
> > > > 
> > > > > IMO a "good" solution in this space needs to accept that
> > > > > those names are not going to be globally unique ... but
> > > > > that they'll be unique within some context, of necessity.
> > > > > 
> > > > > If Greg doesn't want to see those names under classes,
> > > > > so be it ... but where should they then appear?
> > > > 
> > > > As a sysfs file within the device directory called 'name'? ?Then just
> > > > grep through the tree to find the right device, that also handles
> > > > duplicates just fine, right?
> > > 
> > > I want a concrete example.  Those chip->names things don't
> > > seem helpful to me though...
> > > 
> > > ...
> > 
> > Um, I really don't know, as I don't know the GPIO subsystem, nor why you
> > all have this problem in the first place :)
> 
> Maybe Jani can provide a more concrete example.
> 
> 
> > I also find it funny that you think changing the kernel is easier than
> > userspace, that's a strange situation.
> 
> I don't recall saying that.  :)
> 
> It's a case of kernel having access to system data that's not
> otherwise exported to userspace.  It knows how the various bits
> of hardware fit together ... and in this case wants to export
> associations between some GPIOs and some other hardware.  Given
> that, userspace can pick things up.
> 
>  
> > Anyway, I assumed that you already have a struct device for the GPIO
> > devices, right?  Put it in there was what I was thinking, but as I don't
> > understand your current layout, I really don't know.
> 
> There are gpio_chip devices, for a set of GPIOs.  But not for
> the individual GPIOs.
> 
> 
> > > I confess I'd still think a symlink from that directory
> > > to the real GPIO would be easier to work with...
> > 
> > No, don't do that, no symlinks from a class please.
> 
> I didn't catch a reason for that request... could you
> explain that?

It only causes confusion and you would be the only subsystem needing
such a strange thing, causing me to believe something is wrong with the
request.

Also see Kay's response, it was better :)

thanks,

greg k-h

> 
> - Dave

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Artem Bityutskiy]

On Thu, 2009-12-10 at 20:38 -0800, Greg KH wrote:
> On Thu, Dec 10, 2009 at 08:13:58PM -0800, David Brownell wrote:
> > On Thursday 10 December 2009, Greg KH wrote:
> > > 
> > > > IMO a "good" solution in this space needs to accept that
> > > > those names are not going to be globally unique ... but
> > > > that they'll be unique within some context, of necessity.
> > > > 
> > > > If Greg doesn't want to see those names under classes,
> > > > so be it ... but where should they then appear?
> > > 
> > > As a sysfs file within the device directory called 'name'? ?Then just
> > > grep through the tree to find the right device, that also handles
> > > duplicates just fine, right?
> > 
> > I want a concrete example.  Those chip->names things don't
> > seem helpful to me though...
> > 
> > If for example I were building a JTAG adapter on Linux, it
> > might consist of a spidev node (chardev) plus a handful of
> > GPIOs.  So "the device directory" would be the sysfs home
> > of that spidev node (or some variant)?  And inside that
> > directory would be files named after various signals that
> > are used as GPIOs ... maybe SRST, TRST, and DETECT to start
> > with?  Holding some cookie that gets mapped to those GPIO's
> > sysfs entries?
> 
> Um, I really don't know, as I don't know the GPIO subsystem, nor why you
> all have this problem in the first place :)
> 
> I also find it funny that you think changing the kernel is easier than
> userspace, that's a strange situation.

User-space does not know which GPIO number is what. E.g., is the
"power_button" GPIO number 6 or 99? It just does not have this
information.

Kernel knows this, either because it was compiled for a specific board
revision, or it got this information from the bootloader.

And the whole idea is to make kernel export this name. Currently, the
kernel exports only the numbers in sysfs, which is not enough.

And because gpiolib is designed as it is designed, we found that having
additional link in /sys/class/gpio is the nicest solution from gpiolib's
POW. It just fits naturally to the existing design.

And no, we do not have per-gpio struct device, so we cannot add a new
"name" attribute there. We need to either persuade you to accept our
solution or make you take closer look at gpoiolib subsystem and suggest
us the right way to go :-)

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Greg KH]

On Fri, Dec 11, 2009 at 07:36:11AM +0200, Artem Bityutskiy wrote:
> On Thu, 2009-12-10 at 20:38 -0800, Greg KH wrote:
> > On Thu, Dec 10, 2009 at 08:13:58PM -0800, David Brownell wrote:
> > > On Thursday 10 December 2009, Greg KH wrote:
> > > > 
> > > > > IMO a "good" solution in this space needs to accept that
> > > > > those names are not going to be globally unique ... but
> > > > > that they'll be unique within some context, of necessity.
> > > > > 
> > > > > If Greg doesn't want to see those names under classes,
> > > > > so be it ... but where should they then appear?
> > > > 
> > > > As a sysfs file within the device directory called 'name'? ?Then just
> > > > grep through the tree to find the right device, that also handles
> > > > duplicates just fine, right?
> > > 
> > > I want a concrete example.  Those chip->names things don't
> > > seem helpful to me though...
> > > 
> > > If for example I were building a JTAG adapter on Linux, it
> > > might consist of a spidev node (chardev) plus a handful of
> > > GPIOs.  So "the device directory" would be the sysfs home
> > > of that spidev node (or some variant)?  And inside that
> > > directory would be files named after various signals that
> > > are used as GPIOs ... maybe SRST, TRST, and DETECT to start
> > > with?  Holding some cookie that gets mapped to those GPIO's
> > > sysfs entries?
> > 
> > Um, I really don't know, as I don't know the GPIO subsystem, nor why you
> > all have this problem in the first place :)
> > 
> > I also find it funny that you think changing the kernel is easier than
> > userspace, that's a strange situation.
> 
> User-space does not know which GPIO number is what. E.g., is the
> "power_button" GPIO number 6 or 99? It just does not have this
> information.
> 
> Kernel knows this, either because it was compiled for a specific board
> revision, or it got this information from the bootloader.
> 
> And the whole idea is to make kernel export this name. Currently, the
> kernel exports only the numbers in sysfs, which is not enough.

Then export a file with the name somewhere as well.  Why not put that in
the directory you were going to point the symlink at?

> And because gpiolib is designed as it is designed, we found that having
> additional link in /sys/class/gpio is the nicest solution from gpiolib's
> POW. It just fits naturally to the existing design.
> 
> And no, we do not have per-gpio struct device, so we cannot add a new
> "name" attribute there. We need to either persuade you to accept our
> solution or make you take closer look at gpoiolib subsystem and suggest
> us the right way to go :-)

If you don't have a struct device, then what are you going to generate a
symlink to?

And sorry, I'm swamped with other work right now, I honestly have no
time to dig into the gpio subsystem.

thanks,

greg k-h

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Artem Bityutskiy]

On Fri, 2009-12-11 at 06:46 +0100, ext Greg KH wrote:
> > And no, we do not have per-gpio struct device, so we cannot add a new
> > "name" attribute there. We need to either persuade you to accept our
> > solution or make you take closer look at gpoiolib subsystem and suggest
> > us the right way to go :-)
> 
> If you don't have a struct device, then what are you going to generate a
> symlink to?

Sorry, this piece of information was wrong.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Greg KH]

On Fri, Dec 11, 2009 at 09:51:34AM +0200, Artem Bityutskiy wrote:
> On Fri, 2009-12-11 at 06:46 +0100, ext Greg KH wrote:
> > > And no, we do not have per-gpio struct device, so we cannot add a new
> > > "name" attribute there. We need to either persuade you to accept our
> > > solution or make you take closer look at gpoiolib subsystem and suggest
> > > us the right way to go :-)
> > 
> > If you don't have a struct device, then what are you going to generate a
> > symlink to?
> 
> Sorry, this piece of information was wrong.

Great, then you can use the 'let's make a sysfs file and grep for it
from userspace' option.

Or, you can use the 'create a directory for your class, and create
symlinks in there like other subsystems do' option.

Both of which require no changes in the driver core, and have precedent
in that other subsystems already do this today.

Good luck,

greg k-h

[PATCH]crypto: Fix complain about lack test for internal used algorithm

[Youquan,Song]

When load aesni-intel and ghash_clmulni-intel driver,kernel will complain no
 test for some internal used algorithm.
The strange information as following:

alg: No test for __aes-aesni (__driver-aes-aesni)
alg: No test for __ecb-aes-aesni (__driver-ecb-aes-aesni)
alg: No test for __cbc-aes-aesni (__driver-cbc-aes-aesni)
alg: No test for __ecb-aes-aesni (cryptd(__driver-ecb-aes-aesni)
alg: No test for __ghash (__ghash-pclmulqdqni)
alg: No test for __ghash (cryptd(__ghash-pclmulqdqni))

After summarize, the internal used algorithm driver with priority equal 0 or
equal 50 when cryptd is used.

This patch add function to check driver is internal used and will shut up when
internal driver is not tested.  

Signed-off-by: Youquan, Song <youquan.song@intel.com>
Signed-off-by: Ying, Huang <ying.huang@intel.com>
---

diff --git a/crypto/algapi.c b/crypto/algapi.c
index a03ebcb..a602223 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/rtnetlink.h>
 #include <linux/string.h>
+#include <crypto/cryptd.h>
 
 #include "internal.h"
 
@@ -243,6 +244,32 @@ err:
 	goto out;
 }
 
+int crypto_is_internal(const char *driver)
+{
+	struct crypto_alg *q;
+	int found = 0;
+
+	list_for_each_entry(q, &crypto_alg_list, cra_list) {
+		if (!strcmp(q->cra_driver_name, driver)) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (found) {
+
+		if (q->cra_priority == 0)
+			return 1;
+		if (q->cra_priority == CRYPTD_PRIORITY_ADDITION &&
+				strstr(q->cra_driver_name, "cryptd"))
+			return 1;
+	}
+
+	return 0;
+
+}
+EXPORT_SYMBOL_GPL(crypto_is_internal);
+
 void crypto_alg_tested(const char *name, int err)
 {
 	struct crypto_larval *test;
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index f8ae0d9..79785b7 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -275,7 +275,7 @@ static void *cryptd_alloc_instance(struct crypto_alg *alg, unsigned int head,
 
 	memcpy(inst->alg.cra_name, alg->cra_name, CRYPTO_MAX_ALG_NAME);
 
-	inst->alg.cra_priority = alg->cra_priority + 50;
+	inst->alg.cra_priority = alg->cra_priority + CRYPTD_PRIORITY_ADDITION;
 	inst->alg.cra_blocksize = alg->cra_blocksize;
 	inst->alg.cra_alignmask = alg->cra_alignmask;
 
diff --git a/crypto/internal.h b/crypto/internal.h
index 2d22636..23b6498 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -101,6 +101,8 @@ int crypto_register_notifier(struct notifier_block *nb);
 int crypto_unregister_notifier(struct notifier_block *nb);
 int crypto_probing_notify(unsigned long val, void *v);
 
+int crypto_is_internal(const char *driver);
+
 static inline void crypto_alg_put(struct crypto_alg *alg)
 {
 	if (atomic_dec_and_test(&alg->cra_refcnt) && alg->cra_destroy)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index fe68115..542941c 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -2413,7 +2413,8 @@ test_done:
 	return rc;
 
 notest:
-	printk(KERN_INFO "alg: No test for %s (%s)\n", alg, driver);
+	if (!crypto_is_internal(driver))
+		printk(KERN_INFO "alg: No test for %s (%s)\n", alg, driver);
 	return 0;
 non_fips_alg:
 	return -EINVAL;
diff --git a/include/crypto/cryptd.h b/include/crypto/cryptd.h
index 1c96b25..50cd473 100644
--- a/include/crypto/cryptd.h
+++ b/include/crypto/cryptd.h
@@ -9,6 +9,8 @@
 #include <linux/kernel.h>
 #include <crypto/hash.h>
 
+#define CRYPTD_PRIORITY_ADDITION 50
+
 struct cryptd_ablkcipher {
 	struct crypto_ablkcipher base;
 };

Re: [PATCH]crypto: Fix complain about lack test for internal used algorithm

[Herbert Xu]

On Fri, Dec 11, 2009 at 08:23:21AM -0500, Youquan,Song wrote:
> When load aesni-intel and ghash_clmulni-intel driver,kernel will complain no
>  test for some internal used algorithm.
> The strange information as following:
> 
> alg: No test for __aes-aesni (__driver-aes-aesni)
> alg: No test for __ecb-aes-aesni (__driver-ecb-aes-aesni)
> alg: No test for __cbc-aes-aesni (__driver-cbc-aes-aesni)
> alg: No test for __ecb-aes-aesni (cryptd(__driver-ecb-aes-aesni)
> alg: No test for __ghash (__ghash-pclmulqdqni)
> alg: No test for __ghash (cryptd(__ghash-pclmulqdqni))

I'd prefer to just add nul testmgr entries for these algorithms.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH]crypto: Fix complain about lack test for internal used algorithm

[Youquan,Song]

> On Fri, Dec 11, 2009 at 08:23:21AM -0500, Youquan,Song wrote:
> > When load aesni-intel and ghash_clmulni-intel driver,kernel will complain no
> >  test for some internal used algorithm.
> > The strange information as following:
> > 
> > alg: No test for __aes-aesni (__driver-aes-aesni)
> > alg: No test for __ecb-aes-aesni (__driver-ecb-aes-aesni)
> > alg: No test for __cbc-aes-aesni (__driver-cbc-aes-aesni)
> > alg: No test for __ecb-aes-aesni (cryptd(__driver-ecb-aes-aesni)
> > alg: No test for __ghash (__ghash-pclmulqdqni)
> > alg: No test for __ghash (cryptd(__ghash-pclmulqdqni))
> 
> I'd prefer to just add nul testmgr entries for these algorithms.

Hi Herbert,

Do you like the following modification to the patch?
If yes, I will resend the patch.

Thanks

-Youquan

---

diff --git a/crypto/algapi.c b/crypto/algapi.c
index a03ebcb..a602223 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/rtnetlink.h>
 #include <linux/string.h>
+#include <crypto/cryptd.h>
 
 #include "internal.h"
 
@@ -243,6 +244,32 @@ err:
 	goto out;
 }
 
+int crypto_is_internal(const char *driver)
+{
+	struct crypto_alg *q;
+	int found = 0;
+
+	list_for_each_entry(q, &crypto_alg_list, cra_list) {
+		if (!strcmp(q->cra_driver_name, driver)) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (found) {
+
+		if (q->cra_priority == 0)
+			return 1;
+		if (q->cra_priority == CRYPTD_PRIORITY_ADDITION &&
+				strstr(q->cra_driver_name, "cryptd"))
+			return 1;
+	}
+
+	return 0;
+
+}
+EXPORT_SYMBOL_GPL(crypto_is_internal);
+
 void crypto_alg_tested(const char *name, int err)
 {
 	struct crypto_larval *test;
diff --git a/crypto/cryptd.c b/crypto/cryptd.c
index f8ae0d9..79785b7 100644
--- a/crypto/cryptd.c
+++ b/crypto/cryptd.c
@@ -275,7 +275,7 @@ static void *cryptd_alloc_instance(struct crypto_alg *alg, unsigned int head,
 
 	memcpy(inst->alg.cra_name, alg->cra_name, CRYPTO_MAX_ALG_NAME);
 
-	inst->alg.cra_priority = alg->cra_priority + 50;
+	inst->alg.cra_priority = alg->cra_priority + CRYPTD_PRIORITY_ADDITION;
 	inst->alg.cra_blocksize = alg->cra_blocksize;
 	inst->alg.cra_alignmask = alg->cra_alignmask;
 
diff --git a/crypto/internal.h b/crypto/internal.h
index 2d22636..23b6498 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -101,6 +101,8 @@ int crypto_register_notifier(struct notifier_block *nb);
 int crypto_unregister_notifier(struct notifier_block *nb);
 int crypto_probing_notify(unsigned long val, void *v);
 
+int crypto_is_internal(const char *driver);
+
 static inline void crypto_alg_put(struct crypto_alg *alg)
 {
 	if (atomic_dec_and_test(&alg->cra_refcnt) && alg->cra_destroy)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index f70ce52..0de1ef4 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1316,6 +1316,12 @@ out:
 	return err;
 }
 
+static int alg_test_null(const struct alg_test_desc *desc,
+			     const char *driver, u32 type, u32 mask)
+{
+	return 0;
+}
+
 static int alg_test_skcipher(const struct alg_test_desc *desc,
 			     const char *driver, u32 type, u32 mask)
 {
@@ -2086,6 +2092,15 @@ static const struct alg_test_desc alg_test_descs[] = {
 			}
 		}
 	}, {
+		.alg = "null",
+		.test = alg_test_null,
+		.suite = {
+			.hash = {
+				.vecs = NULL,
+				.count = 0
+			}
+		}
+	}, {
 		.alg = "pcbc(fcrypt)",
 		.test = alg_test_skcipher,
 		.suite = {
@@ -2365,6 +2380,15 @@ int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
 	int j;
 	int rc;
 
+	if (crypto_is_internal(driver)) {
+		i = alg_find_test("null");
+		if (i < 0)
+			goto notest;
+		rc = alg_test_descs[i].test(alg_test_descs + i, "null",
+					     type, mask);
+		goto test_done;
+
+	}
 	if ((type & CRYPTO_ALG_TYPE_MASK) == CRYPTO_ALG_TYPE_CIPHER) {
 		char nalg[CRYPTO_MAX_ALG_NAME];
 
diff --git a/include/crypto/cryptd.h b/include/crypto/cryptd.h
index 1c96b25..50cd473 100644
--- a/include/crypto/cryptd.h
+++ b/include/crypto/cryptd.h
@@ -9,6 +9,8 @@
 #include <linux/kernel.h>
 #include <crypto/hash.h>
 
+#define CRYPTD_PRIORITY_ADDITION 50
+
 struct cryptd_ablkcipher {
 	struct crypto_ablkcipher base;
 };

Re: [PATCH]crypto: Fix complain about lack test for internal used algorithm

[Herbert Xu]

On Sat, Dec 19, 2009 at 04:40:43AM -0500, Youquan,Song wrote:
>
> Do you like the following modification to the patch?
> If yes, I will resend the patch.

No what I meant is that each algorithm should have its own nul
entry.  IOW we don't need this crypto_is_internal stuff at all.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH]crypto: Fix complain about lack test for internal used algorithm

[Youquan,Song]

> No what I meant is that each algorithm should have its own nul
> entry.  IOW we don't need this crypto_is_internal stuff at all.

Hi Herbert,

Well, Are you satisfied with the following modification?

Thanks.

-Youquan


diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index f70ce52..60c9857 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1477,9 +1477,69 @@ static int alg_test_cprng(const struct alg_test_desc *desc, const char *driver,
 	return err;
 }
 
+static int alg_test_null(const struct alg_test_desc *desc,
+			     const char *driver, u32 type, u32 mask)
+{
+	return 0;
+}
+
 /* Please keep this list sorted by algorithm name. */
 static const struct alg_test_desc alg_test_descs[] = {
 	{
+		.alg = "ecb(__aes-aesni)",
+		.test = alg_test_null,
+		.suite = {
+			.cipher = {
+				.enc = {
+					.vecs = NULL,
+					.count = 0
+				},
+				.dec = {
+					.vecs = NULL,
+					.count = 0
+				}
+			}
+		}
+	}, {
+		.alg = "__driver-cbc-aes-aesni",
+		.test = alg_test_null,
+		.suite = {
+			.cipher = {
+				.enc = {
+					.vecs = NULL,
+					.count = 0
+				},
+				.dec = {
+					.vecs = NULL,
+					.count = 0
+				}
+			}
+		}
+	}, {
+		.alg = "__driver-ecb-aes-aesni",
+		.test = alg_test_null,
+		.suite = {
+			.cipher = {
+				.enc = {
+					.vecs = NULL,
+					.count = 0
+				},
+				.dec = {
+					.vecs = NULL,
+					.count = 0
+				}
+			}
+		}
+	}, {
+		.alg = "__ghash-pclmulqdqni",
+		.test = alg_test_null,
+		.suite = {
+			.hash = {
+				.vecs = NULL,
+				.count = 0
+			}
+		}
+	}, {
 		.alg = "ansi_cprng",
 		.test = alg_test_cprng,
 		.fips_allowed = 1,
@@ -1623,6 +1683,30 @@ static const struct alg_test_desc alg_test_descs[] = {
 			}
 		}
 	}, {
+		.alg = "cryptd(__driver-ecb-aes-aesni)",
+		.test = alg_test_null,
+		.suite = {
+			.cipher = {
+				.enc = {
+					.vecs = NULL,
+					.count = 0
+				},
+				.dec = {
+					.vecs = NULL,
+					.count = 0
+				}
+			}
+		}
+	}, {
+		.alg = "cryptd(__ghash-pclmulqdqni)",
+		.test = alg_test_null,
+		.suite = {
+			.hash = {
+				.vecs = NULL,
+				.count = 0
+			}
+		}
+	}, {
 		.alg = "ctr(aes)",
 		.test = alg_test_skcipher,
 		.fips_allowed = 1,

Re: [PATCH]crypto: Fix complain about lack test for internal used algorithm

[Herbert Xu]

On Sat, Dec 19, 2009 at 10:07:36AM -0500, Youquan,Song wrote:
> > No what I meant is that each algorithm should have its own nul
> > entry.  IOW we don't need this crypto_is_internal stuff at all.
> 
> Hi Herbert,
> 
> Well, Are you satisfied with the following modification?

Yes looks good to me.  Please resubmit with a proper changelog
and sign-off.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[Resend PATCH]crypto: Fix complain about lack test for internal used algorithm

[Youquan,Song]

When load aesni-intel and ghash_clmulni-intel driver,kernel will complain no
 test for some internal used algorithm.
The strange information as following:

alg: No test for __aes-aesni (__driver-aes-aesni)
alg: No test for __ecb-aes-aesni (__driver-ecb-aes-aesni)
alg: No test for __cbc-aes-aesni (__driver-cbc-aes-aesni)
alg: No test for __ecb-aes-aesni (cryptd(__driver-ecb-aes-aesni)
alg: No test for __ghash (__ghash-pclmulqdqni)
alg: No test for __ghash (cryptd(__ghash-pclmulqdqni))

This patch add NULL test entries for these algorithm and driver.  

Signed-off-by: Youquan, Song <youquan.song@intel.com>
Signed-off-by: Ying, Huang <ying.huang@intel.com>
---


diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index f70ce52..e934eca 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1477,9 +1477,54 @@ static int alg_test_cprng(const struct alg_test_desc *desc, const char *driver,
 	return err;
 }
 
+static int alg_test_null(const struct alg_test_desc *desc,
+			     const char *driver, u32 type, u32 mask)
+{
+	return 0;
+}
+
 /* Please keep this list sorted by algorithm name. */
 static const struct alg_test_desc alg_test_descs[] = {
 	{
+		.alg = "__driver-cbc-aes-aesni",
+		.test = alg_test_null,
+		.suite = {
+			.cipher = {
+				.enc = {
+					.vecs = NULL,
+					.count = 0
+				},
+				.dec = {
+					.vecs = NULL,
+					.count = 0
+				}
+			}
+		}
+	}, {
+		.alg = "__driver-ecb-aes-aesni",
+		.test = alg_test_null,
+		.suite = {
+			.cipher = {
+				.enc = {
+					.vecs = NULL,
+					.count = 0
+				},
+				.dec = {
+					.vecs = NULL,
+					.count = 0
+				}
+			}
+		}
+	}, {
+		.alg = "__ghash-pclmulqdqni",
+		.test = alg_test_null,
+		.suite = {
+			.hash = {
+				.vecs = NULL,
+				.count = 0
+			}
+		}
+	}, {
 		.alg = "ansi_cprng",
 		.test = alg_test_cprng,
 		.fips_allowed = 1,
@@ -1623,6 +1668,30 @@ static const struct alg_test_desc alg_test_descs[] = {
 			}
 		}
 	}, {
+		.alg = "cryptd(__driver-ecb-aes-aesni)",
+		.test = alg_test_null,
+		.suite = {
+			.cipher = {
+				.enc = {
+					.vecs = NULL,
+					.count = 0
+				},
+				.dec = {
+					.vecs = NULL,
+					.count = 0
+				}
+			}
+		}
+	}, {
+		.alg = "cryptd(__ghash-pclmulqdqni)",
+		.test = alg_test_null,
+		.suite = {
+			.hash = {
+				.vecs = NULL,
+				.count = 0
+			}
+		}
+	}, {
 		.alg = "ctr(aes)",
 		.test = alg_test_skcipher,
 		.fips_allowed = 1,
@@ -1669,6 +1738,21 @@ static const struct alg_test_desc alg_test_descs[] = {
 			}
 		}
 	}, {
+		.alg = "ecb(__aes-aesni)",
+		.test = alg_test_null,
+		.suite = {
+			.cipher = {
+				.enc = {
+					.vecs = NULL,
+					.count = 0
+				},
+				.dec = {
+					.vecs = NULL,
+					.count = 0
+				}
+			}
+		}
+	}, {
 		.alg = "ecb(aes)",
 		.test = alg_test_skcipher,
 		.fips_allowed = 1,

Re: [Resend PATCH]crypto: Fix complain about lack test for internal used algorithm

[Herbert Xu]

On Mon, Dec 21, 2009 at 05:38:29AM -0500, Youquan,Song wrote:
> When load aesni-intel and ghash_clmulni-intel driver,kernel will complain no
>  test for some internal used algorithm.
> The strange information as following:
> 
> alg: No test for __aes-aesni (__driver-aes-aesni)
> alg: No test for __ecb-aes-aesni (__driver-ecb-aes-aesni)
> alg: No test for __cbc-aes-aesni (__driver-cbc-aes-aesni)
> alg: No test for __ecb-aes-aesni (cryptd(__driver-ecb-aes-aesni)
> alg: No test for __ghash (__ghash-pclmulqdqni)
> alg: No test for __ghash (cryptd(__ghash-pclmulqdqni))
> 
> This patch add NULL test entries for these algorithm and driver.  
> 
> Signed-off-by: Youquan, Song <youquan.song@intel.com>
> Signed-off-by: Ying, Huang <ying.huang@intel.com>

Patch applied.  Thanks a lot!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Ben Nizette]

On Thu, 2009-12-10 at 20:13 -0800, David Brownell wrote:
> On Thursday 10 December 2009, Greg KH wrote:
> > 
> > > IMO a "good" solution in this space needs to accept that
> > > those names are not going to be globally unique ... but
> > > that they'll be unique within some context, of necessity.
> > > 
> > > If Greg doesn't want to see those names under classes,
> > > so be it ... but where should they then appear?
> > 
> > As a sysfs file within the device directory called 'name'?  Then just
> > grep through the tree to find the right device, that also handles
> > duplicates just fine, right?
> 
> I want a concrete example.  Those chip->names things don't
> seem helpful to me though...
> 
> If for example I were building a JTAG adapter on Linux, it
> might consist of a spidev node (chardev) plus a handful of
> GPIOs.  So "the device directory" would be the sysfs home
> of that spidev node (or some variant)?  And inside that
> directory would be files named after various signals that
> are used as GPIOs ... maybe SRST, TRST, and DETECT to start
> with?  Holding some cookie that gets mapped to those GPIO's
> sysfs entries?

If you've got a spidev node then you've got a dev to pass to
gpio_export_link, all's right with the world.  I think the best thing
(which I think is what Greg was thinking) would be to
have /sys/class/gpio/gpioN/name which reads null unless the kernel has
registered a name against that gpio.

Now I don't like the chip->name thing as a way to hold these labels, I'd
prefer an IDR indexed by gpio number, but that's an implementation
detail for later.

A concrete use case: I've got a board here, the support patch for which
will hopefully appear in a few days.  On it I've got an 8-bit uC which
does a bunch of I/O and on certain conditions, toggles gpio lines so a
userspace daemon knows what's going on.  At the moment, I
have /sys/class/<board name>/ which exports things like revision
information as well as attributes telling me which gpio numbers are used
for which purpose.  My scripts therefore look something like

echo 1 > /sys/class/gpio/gpio`cat /sys/class/<board-name>/ack-gpio`/value

I'm actually fairly happy with that but I don't pretend it's everyone's
cup of tea.  The point is though, the microprocessor has no in-kernel
incarnation except these few gpio so there's no convenient dev node to
link the names through; gpio_export_link() isn't useful.

This problem could be solved as I have, but it could equally-well be
solved by exporting gpioN/name attributes which I could search for.

	--Ben.

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Ben Nizette]

On Thu, 2009-12-10 at 19:47 -0800, Greg KH wrote:
> On Thu, Dec 10, 2009 at 07:39:30PM -0800, David Brownell wrote:
> > On Wednesday 09 December 2009, Jani Nikula wrote:
> > > While the device names are now unique again, collisions in chip->names
> > > will still prevent duplicates from being exported to sysfs.
> > 
> > That's why I was never a real fan of chip->names...

Yea, poor idea indeed.

> > 
> > IMO a "good" solution in this space needs to accept that
> > those names are not going to be globally unique ... but
> > that they'll be unique within some context, of necessity.
> > 
> > If Greg doesn't want to see those names under classes,
> > so be it ... but where should they then appear?
> 
> As a sysfs file within the device directory called 'name'?  Then just
> grep through the tree to find the right device, that also handles
> duplicates just fine, right?

Well it bunts the handling of duplicates to who ever is grepping but
yea, sounds good.  The user script can sanity-check it's results against
the controlling gpio-chip if need be.  In fact, maybe symlink from
gpioN/chip back to gpio-chipY could be useful?  A bit redundant though,
as you can check using the number ranges..

In fact I thought I had a patch to create /sys/class/gpio/gpioN/name at
some stage..  Can't find it though, oh well.

	--Ben.

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Jani Nikula]

Hi David and Ben -

On Fri, 2009-12-11 at 06:12 +0100, ext Ben Nizette wrote:
> On Thu, 2009-12-10 at 19:47 -0800, Greg KH wrote:
> > As a sysfs file within the device directory called 'name'?  Then just
> > grep through the tree to find the right device, that also handles
> > duplicates just fine, right?
> 
> Well it bunts the handling of duplicates to who ever is grepping but
> yea, sounds good.  The user script can sanity-check it's results against
> the controlling gpio-chip if need be.  In fact, maybe symlink from
> gpioN/chip back to gpio-chipY could be useful?  A bit redundant though,
> as you can check using the number ranges..
> 
> In fact I thought I had a patch to create /sys/class/gpio/gpioN/name at
> some stage..  Can't find it though, oh well.

Ben, could you please look harder? ;)

If we were to add /sys/class/gpio/gpioN/name attribute, what would be
the optimal source for the names?

I'd prefer a scheme where a) the name could be set in both board files
and drivers, the latter overriding the former as necessary, and b) the
name could be set without actually requesting the gpio, so you could set
all known names in board files without interfering with the drivers.

AFAICS this would pretty much lead to adding a pair of new functions
gpio_set_name() and gpio_get_name(), which would work also for gpios
that haven't been requested. (IDR lookup Ben mentioned in another mail
sounds good, though there's the problem you can't specify the id - this
is why gpio_setup_irq() uses the flags for storing the id.)

Here are some other alternatives I could think of, but none of them
sound good to me:

1) Add new function gpio_export_name() to export with a certain name
attribute. Leads to two ways of exporting.

2) Add 'name' parameter to gpio_export() to export with a certain name
attribute. Changes an existing interface.

3) Use 'label' in gpio_request() for name attribute. Stores names also
for gpios that are never exported, wastes a pointer per gpio in
gpio_desc.

4) Use chip->names. Wastes a pointer per gpio even if one name is used,
almost the same as adding char *name to struct gpio_desc. Not convenient
to use, at least in OMAP.

Opinions?


BR,
Jani.

Re: [PATCH 3/3] gpiolib: use chip->names for symlinks, always use gpioN for device names

[Ben Nizette]

On Mon, 2009-12-14 at 13:16 +0200, Jani Nikula wrote:
> Hi David and Ben -
> 
> On Fri, 2009-12-11 at 06:12 +0100, ext Ben Nizette wrote:
> > On Thu, 2009-12-10 at 19:47 -0800, Greg KH wrote:
> > > As a sysfs file within the device directory called 'name'?  Then just
> > > grep through the tree to find the right device, that also handles
> > > duplicates just fine, right?
> > 
> > Well it bunts the handling of duplicates to who ever is grepping but
> > yea, sounds good.  The user script can sanity-check it's results against
> > the controlling gpio-chip if need be.  In fact, maybe symlink from
> > gpioN/chip back to gpio-chipY could be useful?  A bit redundant though,
> > as you can check using the number ranges..
> > 
> > In fact I thought I had a patch to create /sys/class/gpio/gpioN/name at
> > some stage..  Can't find it though, oh well.
> 
> Ben, could you please look harder? ;)

hehe, likely destroyed in the Great Hard-Disk Fire of '08.  Shouldn't be
hard to recreate, though I don't have the time at the moment.

> 
> If we were to add /sys/class/gpio/gpioN/name attribute, what would be
> the optimal source for the names?

You and I seem to be on the same page here; my $0.02 is for an IDR with
gpio_{get,set,lookup}_name() functions.  Actually, I can't remember, do
the new flex arrays efficiently store sparse array data, or are they
just useful 'coz they can grow?

Still open to debate though is whether gpio_set_name should enforce
uniqueness; is isn't required at a FS-level any more but I can't see
anything good coming from namespace collisions here.

> 
> I'd prefer a scheme where a) the name could be set in both board files
> and drivers, the latter overriding the former as necessary, and b) the
> name could be set without actually requesting the gpio, so you could set
> all known names in board files without interfering with the drivers.
> 
> AFAICS this would pretty much lead to adding a pair of new functions
> gpio_set_name() and gpio_get_name(), which would work also for gpios
> that haven't been requested. (IDR lookup Ben mentioned in another mail
> sounds good, though there's the problem you can't specify the id - this
> is why gpio_setup_irq() uses the flags for storing the id.)

Agreed.

> 
> Here are some other alternatives I could think of, but none of them
> sound good to me:
> 
> 1) Add new function gpio_export_name() to export with a certain name
> attribute. Leads to two ways of exporting.

Well 3 including _link().  I don't think that's the problem so much as
restricting naming to userspace users only.  While I can't think of
kernel users who /need/ this functionality, it'd clean a few code paths
and while we're at it, why not?

> 
> 2) Add 'name' parameter to gpio_export() to export with a certain name
> attribute. Changes an existing interface.

As above

> 
> 3) Use 'label' in gpio_request() for name attribute. Stores names also
> for gpios that are never exported, wastes a pointer per gpio in
> gpio_desc.

And while debugfs is emphatically /not/ an ABI, it's still a semantic
change to the exported information

> 
> 4) Use chip->names. Wastes a pointer per gpio even if one name is used,
> almost the same as adding char *name to struct gpio_desc. Not convenient
> to use, at least in OMAP.

Not convenient on any platform I know of, I've never liked chip->names
and would hope it goes away when (something like) the above interface is
implemented.

	--Ben.

Re: [PATCH 2/3] gpiolib: add support for having symlinks under gpio class directory

[Greg KH]

On Wed, Dec 09, 2009 at 03:49:03PM +0200, Jani Nikula wrote:
> Extend the functionality of gpio_export_link() to allow exported GPIOs
> to have names using sysfs links under /sys/class/gpio.

No, please don't create symlinks under a class, that is not something
that any userspace tool is expecting.

I don't understand what you are trying to do here, why do you need a
symlink?  What is wrong with the original device names?

confused,

greg k-h

Re: [PATCH 2/3] gpiolib: add support for having symlinks under gpio class directory

[Jani Nikula]

On Thu, 2009-12-10 at 03:48 +0100, ext Greg KH wrote:
> On Wed, Dec 09, 2009 at 03:49:03PM +0200, Jani Nikula wrote:
> > Extend the functionality of gpio_export_link() to allow exported GPIOs
> > to have names using sysfs links under /sys/class/gpio.
> 
> No, please don't create symlinks under a class, that is not something
> that any userspace tool is expecting.
> 
> I don't understand what you are trying to do here, why do you need a
> symlink?  What is wrong with the original device names?

The problem
~~~~~~~~~~~

GPIOs can be exported to gpiolib sysfs at /sys/class/gpio/ like this:

# ls -l /sys/class/gpio/
--w-------    1 root     0            4096 Jan  1 00:00 export
lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio25 -> ../../devices/virtual/gpio/gpio25
lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio38 -> ../../devices/virtual/gpio/gpio38
...

The GPIO lines may and do change from board revision to another. For
example a power button's GPIO number might be 25 in board rev 1.0 but 66
in board rev 1.1.

We want to assign symbolic names to GPIO lines and hide the numbering
changes from userspace, because it is very painful to amend userspace
for every board revision.

Existing solution
~~~~~~~~~~~~~~~~~

gpio_export_link() can be used to create symlinks from drivers' sysfs
to gpiolib, but this obviously requires a driver. For example:

# ls -l /sys/devices/platform/foo/
lrwxrwxrwx    1 root     0               0 Jan  2 00:31 power_button -> ../../virtual/gpio/gpio25
...

This doesn't really work for GPIO lines not associated with any driver.

How we addressed the problem
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We simply decided to extend gpio_export_link(). When it is called
with 'dev == NULL' (no driver), it creates an additional symlink
in /sys/class/gpio/

# ls -l /sys/class/gpio/
--w-------    1 root     0            4096 Jan  1 00:00 export
lrwxrwxrwx    1 root     0               0 Jan  1 00:00 power_button -> ../../devices/virtual/gpio/gpio25
lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio25 -> ../../devices/virtual/gpio/gpio25
lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio38 -> ../../devices/virtual/gpio/gpio38
...

This is exactly what our patchset allows to do.

An alternative would be a dummy driver just to create a home in sysfs
for the standalone GPIOs and use the gpio_export_link() to make the
links there.

Any other suggestions?


Thanks,
Jani,
Artem.

Re: [PATCH 2/3] gpiolib: add support for having symlinks under gpio class directory

[Greg KH]

On Thu, Dec 10, 2009 at 04:32:17PM +0200, Jani Nikula wrote:
> On Thu, 2009-12-10 at 03:48 +0100, ext Greg KH wrote:
> > On Wed, Dec 09, 2009 at 03:49:03PM +0200, Jani Nikula wrote:
> > > Extend the functionality of gpio_export_link() to allow exported GPIOs
> > > to have names using sysfs links under /sys/class/gpio.
> > 
> > No, please don't create symlinks under a class, that is not something
> > that any userspace tool is expecting.
> > 
> > I don't understand what you are trying to do here, why do you need a
> > symlink?  What is wrong with the original device names?
> 
> The problem
> ~~~~~~~~~~~
> 
> GPIOs can be exported to gpiolib sysfs at /sys/class/gpio/ like this:
> 
> # ls -l /sys/class/gpio/
> --w-------    1 root     0            4096 Jan  1 00:00 export
> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio25 -> ../../devices/virtual/gpio/gpio25
> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio38 -> ../../devices/virtual/gpio/gpio38
> ...
> 
> The GPIO lines may and do change from board revision to another. For
> example a power button's GPIO number might be 25 in board rev 1.0 but 66
> in board rev 1.1.
> 
> We want to assign symbolic names to GPIO lines and hide the numbering
> changes from userspace, because it is very painful to amend userspace
> for every board revision.

True, just like it is hard to change the kernel for every type of
configuration as well :)

This is the problem that udev solves, from usersapce, but you don't have
device nodes for it to manage, right?

> Existing solution
> ~~~~~~~~~~~~~~~~~
> 
> gpio_export_link() can be used to create symlinks from drivers' sysfs
> to gpiolib, but this obviously requires a driver. For example:
> 
> # ls -l /sys/devices/platform/foo/
> lrwxrwxrwx    1 root     0               0 Jan  2 00:31 power_button -> ../../virtual/gpio/gpio25
> ...
> 
> This doesn't really work for GPIO lines not associated with any driver.

Understood

> How we addressed the problem
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> We simply decided to extend gpio_export_link(). When it is called
> with 'dev == NULL' (no driver), it creates an additional symlink
> in /sys/class/gpio/
> 
> # ls -l /sys/class/gpio/
> --w-------    1 root     0            4096 Jan  1 00:00 export
> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 power_button -> ../../devices/virtual/gpio/gpio25
> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio25 -> ../../devices/virtual/gpio/gpio25
> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio38 -> ../../devices/virtual/gpio/gpio38
> ...
> 
> This is exactly what our patchset allows to do.
> 
> An alternative would be a dummy driver just to create a home in sysfs
> for the standalone GPIOs and use the gpio_export_link() to make the
> links there.
> 
> Any other suggestions?

This type of "policy" should be better off done in userspace whereever
possible.  Can't you just have a udev rule to create symlinks from
somewhere else, into /sys/class/gpio/ that show this type of information
that you are wanting to have?

thanks,

greg k-h

Re: [PATCH 2/3] gpiolib: add support for having symlinks under gpio class directory

[Kay Sievers]

On Thu, Dec 10, 2009 at 15:49, Greg KH <gregkh@suse.de> wrote:
> On Thu, Dec 10, 2009 at 04:32:17PM +0200, Jani Nikula wrote:
>> We simply decided to extend gpio_export_link(). When it is called
>> with 'dev == NULL' (no driver), it creates an additional symlink
>> in /sys/class/gpio/
>>
>> # ls -l /sys/class/gpio/
>> --w-------    1 root     0            4096 Jan  1 00:00 export
>> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 power_button -> ../../devices/virtual/gpio/gpio25
>> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio25 -> ../../devices/virtual/gpio/gpio25
>> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio38 -> ../../devices/virtual/gpio/gpio38
>> ...
>>
>> This is exactly what our patchset allows to do.
>>
>> An alternative would be a dummy driver just to create a home in sysfs
>> for the standalone GPIOs and use the gpio_export_link() to make the
>> links there.
>>
>> Any other suggestions?

We can not do anything like that. The symlink name must always match
the name in /sys/devices/, and it must be a 1:1 relation. We can not
allow anything else, it confuses tools which rightfully expect this.

You could convert the class to a "bus", the /sys/class layout is
pretty much broken by design, and should better be avoided. It's not
extensible, flat in its layout, and today just a too dumb version of
the "bus" subsystem which does not have all these limitations.

All the device links would show up in /sys/bus/gpio/devices/, here you
have the same rules as in /sys/class/: no additional symlinks, the
same names as in /sys/devices -- but you could add a custom folder to
the  /sys/bus/gpio/ directory where you could do whatever fits your
needs. :)

Thanks,
Kay

Re: [PATCH 2/3] gpiolib: add support for having symlinks under gpio class directory

[Greg KH]

On Thu, Dec 10, 2009 at 04:17:00PM +0100, Kay Sievers wrote:
> On Thu, Dec 10, 2009 at 15:49, Greg KH <gregkh@suse.de> wrote:
> > On Thu, Dec 10, 2009 at 04:32:17PM +0200, Jani Nikula wrote:
> >> We simply decided to extend gpio_export_link(). When it is called
> >> with 'dev == NULL' (no driver), it creates an additional symlink
> >> in /sys/class/gpio/
> >>
> >> # ls -l /sys/class/gpio/
> >> --w-------    1 root     0            4096 Jan  1 00:00 export
> >> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 power_button -> ../../devices/virtual/gpio/gpio25
> >> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio25 -> ../../devices/virtual/gpio/gpio25
> >> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio38 -> ../../devices/virtual/gpio/gpio38
> >> ...
> >>
> >> This is exactly what our patchset allows to do.
> >>
> >> An alternative would be a dummy driver just to create a home in sysfs
> >> for the standalone GPIOs and use the gpio_export_link() to make the
> >> links there.
> >>
> >> Any other suggestions?
> 
> We can not do anything like that. The symlink name must always match
> the name in /sys/devices/, and it must be a 1:1 relation. We can not
> allow anything else, it confuses tools which rightfully expect this.
> 
> You could convert the class to a "bus", the /sys/class layout is
> pretty much broken by design, and should better be avoided. It's not
> extensible, flat in its layout, and today just a too dumb version of
> the "bus" subsystem which does not have all these limitations.
> 
> All the device links would show up in /sys/bus/gpio/devices/, here you
> have the same rules as in /sys/class/: no additional symlinks, the
> same names as in /sys/devices -- but you could add a custom folder to
> the  /sys/bus/gpio/ directory where you could do whatever fits your
> needs. :)

Ah, yeah, good point, the pci "slots" directory does something like this
today, with symlinks pointing out of that directory to other devices.

It's messy, but it somehow works...

thanks,

greg k-h

Re: [PATCH 2/3] gpiolib: add support for having symlinks under gpio class directory

[Jani Nikula]

On Thu, 2009-12-10 at 15:49 +0100, ext Greg KH wrote:
> On Thu, Dec 10, 2009 at 04:32:17PM +0200, Jani Nikula wrote:
> > We want to assign symbolic names to GPIO lines and hide the numbering
> > changes from userspace, because it is very painful to amend userspace
> > for every board revision.
> 
> True, just like it is hard to change the kernel for every type of
> configuration as well :)

As explained by Artem, the kernel might get that information from the
bootloader, for example, so once things are in place, there'd be no need
to modify either the kernel or the userspace.

> This is the problem that udev solves, from usersapce, but you don't have
> device nodes for it to manage, right?

When exporting a GPIO to sysfs, a struct device is created with 0,0 for
dev_t, so I suppose that's correct. 

Not having a device node limits the options with udev, right?

> This type of "policy" should be better off done in userspace whereever
> possible.  Can't you just have a udev rule to create symlinks from
> somewhere else, into /sys/class/gpio/ that show this type of information
> that you are wanting to have?

Let's see. 'udevadm monitor --env' gives me this when I 'echo add >
uevent' for an exported gpio #5:

UDEV  [1230784956.241303] add      /class/gpio/gpio5 (gpio)                     
UDEV_LOG=3                                                                      
ACTION=add                                                                      
DEVPATH=/class/gpio/gpio5                                                       
SUBSYSTEM=gpio                                                                  
SEQNUM=930                                                                      
UDEVD_EVENT=1

Now *assuming* we had /sys/class/gpio/gpioN/name, I could create very
crude udev rules along the lines of this:

SUBSYSTEM=="gpio", ACTION=="add", ATTR{name}!="", RUN="/bin/ln -s /sys$devpath /tmp/gpio-$attr{name}"
SUBSYSTEM=="gpio", ACTION=="remove", ATTR{name}!="", RUN="/bin/rm /tmp/gpio-$attr{name}"

Not exactly pretty, but seems to work (using another attr for testing).
Is there a more elegant way of doing this with udev when there are no
device nodes? Or is it even sensible to use udev without the device
nodes? And what would be the right place for the symlinks?

*If* we got that working in a sensible fashion, it'd be a matter of
adding the "name" attribute to each gpio.


BR,
Jani.

Re: [PATCH 2/3] gpiolib: add support for having symlinks under gpio class directory

[Greg KH]

On Fri, Dec 11, 2009 at 10:41:40AM +0200, Jani Nikula wrote:
> Let's see. 'udevadm monitor --env' gives me this when I 'echo add >
> uevent' for an exported gpio #5:
> 
> UDEV  [1230784956.241303] add      /class/gpio/gpio5 (gpio)                     
> UDEV_LOG=3                                                                      
> ACTION=add                                                                      
> DEVPATH=/class/gpio/gpio5                                                       
> SUBSYSTEM=gpio                                                                  
> SEQNUM=930                                                                      
> UDEVD_EVENT=1
> 
> Now *assuming* we had /sys/class/gpio/gpioN/name, I could create very
> crude udev rules along the lines of this:
> 
> SUBSYSTEM=="gpio", ACTION=="add", ATTR{name}!="", RUN="/bin/ln -s /sys$devpath /tmp/gpio-$attr{name}"
> SUBSYSTEM=="gpio", ACTION=="remove", ATTR{name}!="", RUN="/bin/rm /tmp/gpio-$attr{name}"
> 
> Not exactly pretty, but seems to work (using another attr for testing).
> Is there a more elegant way of doing this with udev when there are no
> device nodes? Or is it even sensible to use udev without the device
> nodes? And what would be the right place for the symlinks?

Kay said he didn't like you creating symlinks in /dev with udev, but
this looks ok to me.

> *If* we got that working in a sensible fashion, it'd be a matter of
> adding the "name" attribute to each gpio.

Yup, just use whatever you were wanting to use as the symlink name, it
seems that logic was already done.

thanks.

greg k-h

Re: [PATCH 2/3] gpiolib: add support for having symlinks under gpio class directory

[David Brownell]

On Thursday 10 December 2009, Jani Nikula wrote:
> GPIOs can be exported to gpiolib sysfs at /sys/class/gpio/ like this:
> 
> # ls -l /sys/class/gpio/
> --w-------    1 root     0            4096 Jan  1 00:00 export
> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio25 -> ../../devices/virtual/gpio/gpio25
> lrwxrwxrwx    1 root     0               0 Jan  1 00:00 gpio38 -> ../../devices/virtual/gpio/gpio38
> ...

Hmm, ISTR they don't always appear as "virtual" ... though that's
pretty much irrelevant to the larger point you're making.  Even
when there are real device nodes for the gpio chips parenting any
given GPIOs, they end up in fairly un-useful parts of the device
tree (in terms of being related to something more significant than
the random wires that were handy to the hardware designer).


> The GPIO lines may and do change from board revision to another. For
> example a power button's GPIO number might be 25 in board rev 1.0 but 66
> in board rev 1.1.
> 
> We want to assign symbolic names to GPIO lines and hide the numbering
> changes from userspace, because it is very painful to amend userspace
> for every board revision.

Related:  these are part of what one might call "composite devices",
which are crafted using a component from this subsystem, one from that,
a few from over there, and so on.  They don't properly belong in any
single location in the device tree.  Some level of symlinking seems
inevitable.

And no, a full fledged chardev node is inappropriate so I don't
see udev as able to help out.

Re: [PATCH 1/3] device class: add symlink creation helpers

[Greg KH]

On Wed, Dec 09, 2009 at 03:49:02PM +0200, Jani Nikula wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

No, I don't like this, we do not want to be creating symlinks in a class
directory.  See my other email about this.

thanks,

greg k-h

Re: [PATCH 0/3] gpiolib: gpio naming in sysfs

[Andrew Morton]

On Wed,  9 Dec 2009 15:49:01 +0200
Jani Nikula <ext-jani.1.nikula@nokia.com> wrote:

> Hi David and Greg -
> 
> There's been debate in the past about naming gpios exported to the
> sysfs. Long story short, there are users for that, and there are now
> two ways of naming gpios in the sysfs: char **names in struct
> gpio_chip [1], and gpio_export_link() [2].
> 
> This patchset combines these two by allowing gpio_export_link() to
> have dev == NULL to make the link under gpiolib sysfs (instead of
> arbitrary device), and to use gpio_chip names to create links (instead
> of naming the actual devices with those). This gpio_export_link() with
> dev == NULL would also be useful for gpios not associated with a
> driver.
> 
> Greg, patch 1/3 introduces class_{create,remove}_link(), is that
> acceptable?
> 
> CC Daniel, you introduced names in struct gpio_chip in the first
> place, does this (especially patch 3/3) look acceptable to you?
> 

I haven't seen an email from David in perhaps two months, so we're on
our own in this.

Can you please remind us what the earlier objections were, and tell us
how this patchset addresses them?

Thanks.