| Message ID | 1350477107-26512-3-git-send-email-stigge@antcom.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 17, 2012 at 02:31:34PM +0200, Roland Stigge wrote: > This patch adds sysfs support to the block GPIO API. > > Signed-off-by: Roland Stigge <stigge@antcom.de> > > --- > Documentation/ABI/testing/sysfs-gpio | 6 > drivers/gpio/gpiolib.c | 214 +++++++++++++++++++++++++++++++++++ > include/asm-generic/gpio.h | 11 + > include/linux/gpio.h | 13 ++ > 4 files changed, 243 insertions(+), 1 deletion(-) > > --- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio > +++ linux-2.6/Documentation/ABI/testing/sysfs-gpio > @@ -24,4 +24,8 @@ Description: > /base ... (r/o) same as N > /label ... (r/o) descriptive, not necessarily unique > /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1) > - > + /blockN ... for each GPIO block #N > + /ngpio ... (r/o) number of GPIOs in this group > + /exported ... sysfs export state of this group (0, 1) > + /value ... current value as 32 or 64 bit integer in decimal > + (only available if /exported is 1) I think you need some more documentation here, as I just noticed something "odd": > +static int gpio_block_value_unexport(struct gpio_block *block) > +{ > + struct device *dev; > + int i; > + > + dev = class_find_device(&gpio_block_class, NULL, block, match_export); > + if (!dev) > + return -ENODEV; > + > + for (i = 0; i < block->ngpio; i++) > + gpio_free(block->gpio[i]); > + > + device_remove_file(dev, &dev_attr_block_value); > + > + return 0; > +} Wait, what? You are removing a sysfs file in this function, from within a sysfs write: > +static ssize_t gpio_block_exported_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + long value; > + int status; > + struct gpio_block *block = dev_get_drvdata(dev); > + int exported = gpio_block_value_is_exported(block); > + > + status = kstrtoul(buf, 0, &value); > + if (status < 0) > + goto err; > + > + if (value != exported) { > + if (value) > + status = gpio_block_value_export(block); > + else > + status = gpio_block_value_unexport(block); That looks like a recipie for disaster. Why do you allow userspace to do this? Anyway, the other fixups for how you create/destroy the attribute files looks great, thanks for making those changes. greg k-h
On 10/17/2012 09:05 PM, Greg KH wrote: >> +static int gpio_block_value_unexport(struct gpio_block *block) >> +{ >> + struct device *dev; >> + int i; >> + >> + dev = class_find_device(&gpio_block_class, NULL, block, match_export); >> + if (!dev) >> + return -ENODEV; >> + >> + for (i = 0; i < block->ngpio; i++) >> + gpio_free(block->gpio[i]); >> + >> + device_remove_file(dev, &dev_attr_block_value); >> + >> + return 0; >> +} > > Wait, what? You are removing a sysfs file in this function, from within > a sysfs write: Yes, exactly: >> +static ssize_t gpio_block_exported_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + long value; >> + int status; >> + struct gpio_block *block = dev_get_drvdata(dev); >> + int exported = gpio_block_value_is_exported(block); >> + >> + status = kstrtoul(buf, 0, &value); >> + if (status < 0) >> + goto err; >> + >> + if (value != exported) { >> + if (value) >> + status = gpio_block_value_export(block); >> + else >> + status = gpio_block_value_unexport(block); > > That looks like a recipie for disaster. Why do you allow userspace to > do this? Exporting for gpio blocks is done as follows: writing "1" to the "exported" _device_ attribute of the gpio block creates the "values" attribute and at the same time requests the whole block (including all of its gpios) as "sysfs". This admittedly deviates from the exporting of gpios (with the "export" and "unexport" _class_ attributes) because blocks are not numbered. In contrast, they are registered in a list (as was requested). Now, I already had the idea of also having an "export" and "unexport" _class_ attribute for blocks also, but from a userspace perspective you only see the presence and name of the gpio block if it is already being in sysfs (even if not exported yet). If it wouldn't be this way, a user couldn't guess how the required gpio block is called (or numbered), wouldn't even know about its presence. Just for understanding your strong desire for the device attribute ("value") being always present (in contrast to being created and removed dynamically) - can you please give me any hint how the dynamic approach would lead to disaster? One possibility would be to always have "value" as a default device attribute, but then it wouldn't be so obvious that it's useless until "exported" is "1". What do you think? For now, I would elaborate about my terse "only available if /exported is 1" like this: " Block GPIO devices are visible in sysfs as soon as they are registered (e.g. via devicetree definition). For actual I/O use, their "exported" boolean attribute must be set to "1". Then, the attribute "values" is created and at the same time, the GPIOs in the block are requested for exclusive use by sysfs. " Thanks in advance, Roland
On Thu, Oct 18, 2012 at 12:07 PM, Roland Stigge <stigge@antcom.de> wrote: > On 10/17/2012 09:05 PM, Greg KH wrote: >>> >>> + if (value != exported) { >>> + if (value) >>> + status = gpio_block_value_export(block); >>> + else >>> + status = gpio_block_value_unexport(block); >> >> That looks like a recipie for disaster. Why do you allow userspace to >> do this? > > Exporting for gpio blocks is done as follows: writing "1" to the > "exported" _device_ attribute of the gpio block creates the "values" > attribute and at the same time requests the whole block (including all > of its gpios) as "sysfs". To me it reads like Greg's comment is basically pinpointing a flaw in Brownell's initial design of gpio sysfs: that new sysfs files are created and destroyed by writing into sysfs */export files from userspace? See commit: d8f388d8dc8d4f36539dd37c1fff62cc404ea0fc The block GPIO stuff is just following that design pattern. Yours, Linus Walleij
On 10/19/2012 12:35 PM, Linus Walleij wrote: > On Thu, Oct 18, 2012 at 12:07 PM, Roland Stigge <stigge@antcom.de> wrote: >> On 10/17/2012 09:05 PM, Greg KH wrote: >>>> >>>> + if (value != exported) { >>>> + if (value) >>>> + status = gpio_block_value_export(block); >>>> + else >>>> + status = gpio_block_value_unexport(block); >>> >>> That looks like a recipie for disaster. Why do you allow userspace to >>> do this? >> >> Exporting for gpio blocks is done as follows: writing "1" to the >> "exported" _device_ attribute of the gpio block creates the "values" >> attribute and at the same time requests the whole block (including all >> of its gpios) as "sysfs". > > To me it reads like Greg's comment is basically pinpointing a flaw > in Brownell's initial design of gpio sysfs: that new sysfs files are > created and destroyed by writing into sysfs */export files from > userspace? > > See commit: d8f388d8dc8d4f36539dd37c1fff62cc404ea0fc > > The block GPIO stuff is just following that design pattern. So what do you think about my just proposed idea of always having the "value" argument present, but only useable when "exported" is "1"? Now only talking about the block gpios, but later maybe also for gpios? But I would only do this if you and Greg consider it reasonable. Thanks in advance, Roland
On Thu, Oct 18, 2012 at 12:07:39PM +0200, Roland Stigge wrote: > On 10/17/2012 09:05 PM, Greg KH wrote: > >> +static int gpio_block_value_unexport(struct gpio_block *block) > >> +{ > >> + struct device *dev; > >> + int i; > >> + > >> + dev = class_find_device(&gpio_block_class, NULL, block, match_export); > >> + if (!dev) > >> + return -ENODEV; > >> + > >> + for (i = 0; i < block->ngpio; i++) > >> + gpio_free(block->gpio[i]); > >> + > >> + device_remove_file(dev, &dev_attr_block_value); > >> + > >> + return 0; > >> +} > > > > Wait, what? You are removing a sysfs file in this function, from within > > a sysfs write: > > Yes, exactly: > > >> +static ssize_t gpio_block_exported_store(struct device *dev, > >> + struct device_attribute *attr, > >> + const char *buf, size_t size) > >> +{ > >> + long value; > >> + int status; > >> + struct gpio_block *block = dev_get_drvdata(dev); > >> + int exported = gpio_block_value_is_exported(block); > >> + > >> + status = kstrtoul(buf, 0, &value); > >> + if (status < 0) > >> + goto err; > >> + > >> + if (value != exported) { > >> + if (value) > >> + status = gpio_block_value_export(block); > >> + else > >> + status = gpio_block_value_unexport(block); > > > > That looks like a recipie for disaster. Why do you allow userspace to > > do this? > > Exporting for gpio blocks is done as follows: writing "1" to the > "exported" _device_ attribute of the gpio block creates the "values" > attribute and at the same time requests the whole block (including all > of its gpios) as "sysfs". > > This admittedly deviates from the exporting of gpios (with the "export" > and "unexport" _class_ attributes) because blocks are not numbered. In > contrast, they are registered in a list (as was requested). Now, I > already had the idea of also having an "export" and "unexport" _class_ > attribute for blocks also, but from a userspace perspective you only see > the presence and name of the gpio block if it is already being in sysfs > (even if not exported yet). If it wouldn't be this way, a user couldn't > guess how the required gpio block is called (or numbered), wouldn't even > know about its presence. > > Just for understanding your strong desire for the device attribute > ("value") being always present (in contrast to being created and removed > dynamically) - can you please give me any hint how the dynamic approach > would lead to disaster? > > One possibility would be to always have "value" as a default device > attribute, but then it wouldn't be so obvious that it's useless until > "exported" is "1". > > What do you think? > > For now, I would elaborate about my terse "only available if /exported > is 1" like this: > > " > Block GPIO devices are visible in sysfs as soon as they are registered > (e.g. via devicetree definition). For actual I/O use, their "exported" > boolean attribute must be set to "1". Then, the attribute "values" is > created and at the same time, the GPIOs in the block are requested for > exclusive use by sysfs. > " Yes, that explanation makes it more obvious as to what is going on, it caught me by supprise. We used to have problems with attributes removing/adding devices or attributes from their callbacks, but that has been fixed up now, and the fact that the other gpio code works this way is good enough precedence to keep me from objecting to it. thanks, greg k-h
On Fri, Oct 19, 2012 at 8:02 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > We used to have problems with attributes removing/adding devices or > attributes from their callbacks, but that has been fixed up now, and the > fact that the other gpio code works this way is good enough precedence > to keep me from objecting to it. OK thanks I get this now too... Yours, Linus Walleij
On Fri, Oct 19, 2012 at 1:51 PM, Roland Stigge <stigge@antcom.de> wrote: > [Me] >> The block GPIO stuff is just following that design pattern. > > So what do you think about my just proposed idea of always having the > "value" argument present, but only useable when "exported" is "1"? Now > only talking about the block gpios, but later maybe also for gpios? We should atleast be consistent and do it the same way for blocks as individual gpios. > But I would only do this if you and Greg consider it reasonable. I'm still afraid of this whole thing and would seek Grant's consent. Yours, Linus Walleij
On 10/22/2012 10:55 AM, Linus Walleij wrote: > On Fri, Oct 19, 2012 at 1:51 PM, Roland Stigge <stigge@antcom.de> wrote: >> So what do you think about my just proposed idea of always having the >> "value" argument present, but only useable when "exported" is "1"? Now >> only talking about the block gpios, but later maybe also for gpios? > > We should atleast be consistent and do it the same way for > blocks as individual gpios. OK, I'll leave it as creating-attribute-on-export. >> But I would only do this if you and Greg consider it reasonable. > > I'm still afraid of this whole thing and would seek Grant's consent. Yes, he acked. :-) Thanks, Roland
--- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio +++ linux-2.6/Documentation/ABI/testing/sysfs-gpio @@ -24,4 +24,8 @@ Description: /base ... (r/o) same as N /label ... (r/o) descriptive, not necessarily unique /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1) - + /blockN ... for each GPIO block #N + /ngpio ... (r/o) number of GPIOs in this group + /exported ... sysfs export state of this group (0, 1) + /value ... current value as 32 or 64 bit integer in decimal + (only available if /exported is 1) --- linux-2.6.orig/drivers/gpio/gpiolib.c +++ linux-2.6/drivers/gpio/gpiolib.c @@ -972,6 +972,214 @@ static void gpiochip_unexport(struct gpi chip->label, status); } +static ssize_t gpio_block_ngpio_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + const struct gpio_block *block = dev_get_drvdata(dev); + + return sprintf(buf, "%u\n", block->ngpio); +} + +static ssize_t gpio_block_value_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + const struct gpio_block *block = dev_get_drvdata(dev); + + return sprintf(buf, sizeof(unsigned long) == 4 ? "0x%08lx\n" : + "0x%016lx\n", gpio_block_get(block)); +} + +static bool gpio_block_is_output(struct gpio_block *block) +{ + int i; + + for (i = 0; i < block->ngpio; i++) + if (!test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags)) + return false; + return true; +} + +static ssize_t gpio_block_value_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + ssize_t status; + struct gpio_block *block = dev_get_drvdata(dev); + unsigned long value; + + status = kstrtoul(buf, 0, &value); + if (status == 0) { + mutex_lock(&sysfs_lock); + if (gpio_block_is_output(block)) { + gpio_block_set(block, value); + status = size; + } else { + status = -EPERM; + } + mutex_unlock(&sysfs_lock); + } + return status; +} + +static struct device_attribute +dev_attr_block_value = __ATTR(value, S_IWUSR | S_IRUGO, gpio_block_value_show, + gpio_block_value_store); + +static struct class gpio_block_class; + +static int gpio_block_value_is_exported(struct gpio_block *block) +{ + struct device *dev; + struct sysfs_dirent *sd = NULL; + + mutex_lock(&sysfs_lock); + dev = class_find_device(&gpio_block_class, NULL, block, match_export); + if (!dev) + goto out; + + sd = sysfs_get_dirent(dev->kobj.sd, NULL, "value"); + +out: + mutex_unlock(&sysfs_lock); + return !!sd; +} + +static ssize_t gpio_block_exported_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct gpio_block *block = dev_get_drvdata(dev); + + return sprintf(buf, "%u\n", gpio_block_value_is_exported(block)); +} + +static int gpio_block_value_export(struct gpio_block *block) +{ + struct device *dev; + int status; + int i; + + mutex_lock(&sysfs_lock); + + for (i = 0; i < block->ngpio; i++) { + status = gpio_request(block->gpio[i], "sysfs"); + if (status) + goto out; + } + + dev = class_find_device(&gpio_block_class, NULL, block, match_export); + if (!dev) { + status = -ENODEV; + goto out; + } + + status = device_create_file(dev, &dev_attr_block_value); + if (status) + goto out; + + mutex_unlock(&sysfs_lock); + return 0; + +out: + while (--i >= 0) + gpio_free(block->gpio[i]); + + mutex_unlock(&sysfs_lock); + return status; +} + +static int gpio_block_value_unexport(struct gpio_block *block) +{ + struct device *dev; + int i; + + dev = class_find_device(&gpio_block_class, NULL, block, match_export); + if (!dev) + return -ENODEV; + + for (i = 0; i < block->ngpio; i++) + gpio_free(block->gpio[i]); + + device_remove_file(dev, &dev_attr_block_value); + + return 0; +} + +static ssize_t gpio_block_exported_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + long value; + int status; + struct gpio_block *block = dev_get_drvdata(dev); + int exported = gpio_block_value_is_exported(block); + + status = kstrtoul(buf, 0, &value); + if (status < 0) + goto err; + + if (value != exported) { + if (value) + status = gpio_block_value_export(block); + else + status = gpio_block_value_unexport(block); + if (!status) + status = size; + } else { + status = size; + } +err: + return status; +} + +static struct device_attribute gpio_block_attrs[] = { + __ATTR(exported, S_IWUSR | S_IRUGO, gpio_block_exported_show, + gpio_block_exported_store), + __ATTR(ngpio, S_IRUGO, gpio_block_ngpio_show, NULL), + __ATTR_NULL, +}; + +static struct class gpio_block_class = { + .name = "gpioblock", + .owner = THIS_MODULE, + + .dev_attrs = gpio_block_attrs, +}; + +int gpio_block_export(struct gpio_block *block) +{ + int status = 0; + struct device *dev; + + /* can't export until sysfs is available ... */ + if (!gpio_block_class.p) { + pr_debug("%s: called too early!\n", __func__); + return -ENOENT; + } + + mutex_lock(&sysfs_lock); + dev = device_create(&gpio_block_class, NULL, MKDEV(0, 0), block, + block->name); + if (IS_ERR(dev)) + status = PTR_ERR(dev); + mutex_unlock(&sysfs_lock); + + return status; +} +EXPORT_SYMBOL_GPL(gpio_block_export); + +void gpio_block_unexport(struct gpio_block *block) +{ + struct device *dev; + + mutex_lock(&sysfs_lock); + dev = class_find_device(&gpio_block_class, NULL, block, match_export); + if (dev) + device_unregister(dev); + mutex_unlock(&sysfs_lock); +} +EXPORT_SYMBOL_GPL(gpio_block_unexport); + static int __init gpiolib_sysfs_init(void) { int status; @@ -982,6 +1190,10 @@ static int __init gpiolib_sysfs_init(voi if (status < 0) return status; + status = class_register(&gpio_block_class); + if (status < 0) + return status; + /* Scan and register the gpio_chips which registered very * early (e.g. before the class_register above was called). * @@ -1876,6 +2088,7 @@ int gpio_block_register(struct gpio_bloc return -EBUSY; list_add(&block->list, &gpio_block_list); + gpio_block_export(block); return 0; } @@ -1888,6 +2101,7 @@ void gpio_block_unregister(struct gpio_b list_for_each_entry(i, &gpio_block_list, list) if (i == block) { list_del(&i->list); + gpio_block_unexport(block); break; } } --- linux-2.6.orig/include/asm-generic/gpio.h +++ linux-2.6/include/asm-generic/gpio.h @@ -211,6 +211,8 @@ extern int gpio_export_link(struct devic unsigned gpio); extern int gpio_sysfs_set_active_low(unsigned gpio, int value); extern void gpio_unexport(unsigned gpio); +extern int gpio_block_export(struct gpio_block *block); +extern void gpio_block_unexport(struct gpio_block *block); #endif /* CONFIG_GPIO_SYSFS */ @@ -270,6 +272,15 @@ static inline int gpio_sysfs_set_active_ static inline void gpio_unexport(unsigned gpio) { } + +static inline int gpio_block_export(struct gpio_block *block) +{ + return -ENOSYS; +} + +static inline void gpio_block_unexport(struct gpio_block *block) +{ +} #endif /* CONFIG_GPIO_SYSFS */ #endif /* _ASM_GENERIC_GPIO_H */ --- linux-2.6.orig/include/linux/gpio.h +++ linux-2.6/include/linux/gpio.h @@ -291,6 +291,19 @@ static inline void gpio_unexport(unsigne WARN_ON(1); } +static inline int gpio_block_export(struct gpio_block *block) +{ + /* GPIO block can never have been requested or set as {in,out}put */ + WARN_ON(1); + return -EINVAL; +} + +static inline void gpio_block_unexport(struct gpio_block *block) +{ + /* GPIO block can never have been exported */ + WARN_ON(1); +} + static inline int gpio_to_irq(unsigned gpio) { /* GPIO can never have been requested or set as input */
This patch adds sysfs support to the block GPIO API. Signed-off-by: Roland Stigge <stigge@antcom.de> --- Documentation/ABI/testing/sysfs-gpio | 6 drivers/gpio/gpiolib.c | 214 +++++++++++++++++++++++++++++++++++ include/asm-generic/gpio.h | 11 + include/linux/gpio.h | 13 ++ 4 files changed, 243 insertions(+), 1 deletion(-)