Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On 10/22/2012 10:55 AM, Linus Walleij wrote: > On Fri, Oct 19, 2012 at 1:51 PM, Roland Stigge 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On Fri, Oct 19, 2012 at 1:51 PM, Roland Stigge 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On Fri, Oct 19, 2012 at 8:02 PM, Greg KH 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On Fri, Oct 19, 2012 at 8:02 PM, Greg KH gre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On Fri, Oct 19, 2012 at 1:51 PM, Roland Stigge sti...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On 10/22/2012 10:55 AM, Linus Walleij wrote: On Fri, Oct 19, 2012 at 1:51 PM, Roland Stigge sti...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
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(_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, _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)
> >> +{
> >> + longvalue;
> >> + int status;
> >> + struct gpio_block *block = dev_get_drvdata(dev);
> >> + int exported = gpio_block_value_is_exported(block);
> >> +
> >> + status = kstrtoul(buf, 0, );
> >> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On 10/19/2012 12:35 PM, Linus Walleij wrote:
> On Thu, Oct 18, 2012 at 12:07 PM, Roland Stigge 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On Thu, Oct 18, 2012 at 12:07 PM, Roland Stigge 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On Thu, Oct 18, 2012 at 12:07 PM, Roland Stigge sti...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
On 10/19/2012 12:35 PM, Linus Walleij wrote:
On Thu, Oct 18, 2012 at 12:07 PM, Roland Stigge sti...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
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)
+{
+ longvalue;
+ 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
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(_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, _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)
>> +{
>> +longvalue;
>> +int status;
>> +struct gpio_block *block = dev_get_drvdata(dev);
>> +int exported = gpio_block_value_is_exported(block);
>> +
>> +status = kstrtoul(buf, 0, );
>> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
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)
+{
+longvalue;
+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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
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
>
> ---
> 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(_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, _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)
> +{
> + longvalue;
> + int status;
> + struct gpio_block *block = dev_get_drvdata(dev);
> + int exported = gpio_block_value_is_exported(block);
> +
> + status = kstrtoul(buf, 0, );
> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 02/15 v5] gpio: Add sysfs support to block GPIO API
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 sti...@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)
+{
+ longvalue;
+ 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
