Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1220726 > unrolled thread

[PATCH 1/3] sysfs: Fix is_visible() support for binary attributes

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Emilio López <emilio.lopez@collabora.co.uk> - 2015-09-08 14:10 +0200
    Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Guenter Roeck <linux@roeck-us.net> - 2015-09-08 17:40 +0200
      Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Greg KH <gregkh@linuxfoundation.org> - 2015-09-08 21:20 +0200
        Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Guenter Roeck <linux@roeck-us.net> - 2015-09-08 21:40 +0200
          Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Emilio López <emilio.lopez@collabora.co.uk> - 2015-09-09 03:00 +0200
            Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Guenter Roeck <linux@roeck-us.net> - 2015-09-09 03:20 +0200
              Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Greg KH <gregkh@linuxfoundation.org> - 2015-09-09 06:00 +0200
                Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Guenter Roeck <linux@roeck-us.net> - 2015-09-09 06:20 +0200
                  Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Emilio López <emilio.lopez@collabora.co.uk> - 2015-09-09 15:20 +0200
                    Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Guenter Roeck <linux@roeck-us.net> - 2015-09-09 15:40 +0200
                    Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Greg KH <gregkh@linuxfoundation.org> - 2015-09-09 20:30 +0200

#1220726 — [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes

According to the sysfs header file:

    "The returned value will replace static permissions defined in
     struct attribute or struct bin_attribute."

but this isn't the case, as is_visible is only called on
struct attribute only. This patch adds the code paths required
to support is_visible() on binary attributes.

Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---
 fs/sysfs/group.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 39a0199..eb6996a 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 {
 	struct attribute *const *attr;
 	struct bin_attribute *const *bin_attr;
-	int error = 0, i;
+	int error = 0, i = 0;
 
 	if (grp->attrs) {
-		for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
+		for (attr = grp->attrs; *attr && !error; i++, attr++) {
 			umode_t mode = (*attr)->mode;
 
 			/*
@@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
 	}
 
 	if (grp->bin_attrs) {
-		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
+		for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
+			umode_t mode = (*bin_attr)->attr.mode;
+
 			if (update)
 				kernfs_remove_by_name(parent,
 						(*bin_attr)->attr.name);
+			if (grp->is_visible) {
+				mode = grp->is_visible(kobj,
+						       &(*bin_attr)->attr, i);
+				if (!mode)
+					continue;
+			}
+
+			WARN(mode & ~(SYSFS_PREALLOC | 0664),
+			     "Attribute %s: Invalid permissions 0%o\n",
+			     (*bin_attr)->attr.name, mode);
+
+			mode &= SYSFS_PREALLOC | 0664;
 			error = sysfs_add_file_mode_ns(parent,
 					&(*bin_attr)->attr, true,
-					(*bin_attr)->attr.mode, NULL);
+					mode, NULL);
 			if (error)
 				break;
 		}
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1220939

Emilio,

On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
> According to the sysfs header file:
> 
>     "The returned value will replace static permissions defined in
>      struct attribute or struct bin_attribute."
> 
> but this isn't the case, as is_visible is only called on
> struct attribute only. This patch adds the code paths required
> to support is_visible() on binary attributes.
> 
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> ---
>  fs/sysfs/group.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 39a0199..eb6996a 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  {
>  	struct attribute *const *attr;
>  	struct bin_attribute *const *bin_attr;
> -	int error = 0, i;
> +	int error = 0, i = 0;
>  
>  	if (grp->attrs) {
> -		for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> +		for (attr = grp->attrs; *attr && !error; i++, attr++) {
>  			umode_t mode = (*attr)->mode;
>  
>  			/*
> @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>  	}
>  
>  	if (grp->bin_attrs) {
> -		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> +		for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> +			umode_t mode = (*bin_attr)->attr.mode;
> +
>  			if (update)
>  				kernfs_remove_by_name(parent,
>  						(*bin_attr)->attr.name);
> +			if (grp->is_visible) {
> +				mode = grp->is_visible(kobj,
> +						       &(*bin_attr)->attr, i);

With this, if 'n' is the number of non-binary attributes,

for i < n:
	The index passed to is_visible points to a non-binary attribute.
for i >= n:
	The index passed to is_visible points to the (index - n)th binary
	attribute.

Unless I am missing something, this is not explained anywhere, but it is
not entirely trivial to understand. I think it should be documented.

Also, it might be a good idea to check through existing code to ensure
that this change doesn't accidentially cause trouble (existing drivers
implementing both attribute types may not expect to see an index variable
pointing to a value larger than the number of elements in the attrs array).

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221045

On Tue, Sep 08, 2015 at 08:30:13AM -0700, Guenter Roeck wrote:
> Emilio,
> 
> On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
> > According to the sysfs header file:
> > 
> >     "The returned value will replace static permissions defined in
> >      struct attribute or struct bin_attribute."
> > 
> > but this isn't the case, as is_visible is only called on
> > struct attribute only. This patch adds the code paths required
> > to support is_visible() on binary attributes.
> > 
> > Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> > ---
> >  fs/sysfs/group.c | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > index 39a0199..eb6996a 100644
> > --- a/fs/sysfs/group.c
> > +++ b/fs/sysfs/group.c
> > @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> >  {
> >  	struct attribute *const *attr;
> >  	struct bin_attribute *const *bin_attr;
> > -	int error = 0, i;
> > +	int error = 0, i = 0;
> >  
> >  	if (grp->attrs) {
> > -		for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> > +		for (attr = grp->attrs; *attr && !error; i++, attr++) {
> >  			umode_t mode = (*attr)->mode;
> >  
> >  			/*
> > @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> >  	}
> >  
> >  	if (grp->bin_attrs) {
> > -		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> > +		for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> > +			umode_t mode = (*bin_attr)->attr.mode;
> > +
> >  			if (update)
> >  				kernfs_remove_by_name(parent,
> >  						(*bin_attr)->attr.name);
> > +			if (grp->is_visible) {
> > +				mode = grp->is_visible(kobj,
> > +						       &(*bin_attr)->attr, i);
> 
> With this, if 'n' is the number of non-binary attributes,
> 
> for i < n:
> 	The index passed to is_visible points to a non-binary attribute.
> for i >= n:
> 	The index passed to is_visible points to the (index - n)th binary
> 	attribute.
> 
> Unless I am missing something, this is not explained anywhere, but it is
> not entirely trivial to understand. I think it should be documented.

I agree, make i the number of the bin attribute and that should solve
this issue.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221053

On Tue, Sep 08, 2015 at 12:10:02PM -0700, Greg KH wrote:
> On Tue, Sep 08, 2015 at 08:30:13AM -0700, Guenter Roeck wrote:
> > Emilio,
> > 
> > On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
> > > According to the sysfs header file:
> > > 
> > >     "The returned value will replace static permissions defined in
> > >      struct attribute or struct bin_attribute."
> > > 
> > > but this isn't the case, as is_visible is only called on
> > > struct attribute only. This patch adds the code paths required
> > > to support is_visible() on binary attributes.
> > > 
> > > Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> > > ---
> > >  fs/sysfs/group.c | 22 ++++++++++++++++++----
> > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > index 39a0199..eb6996a 100644
> > > --- a/fs/sysfs/group.c
> > > +++ b/fs/sysfs/group.c
> > > @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > >  {
> > >  	struct attribute *const *attr;
> > >  	struct bin_attribute *const *bin_attr;
> > > -	int error = 0, i;
> > > +	int error = 0, i = 0;
> > >  
> > >  	if (grp->attrs) {
> > > -		for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> > > +		for (attr = grp->attrs; *attr && !error; i++, attr++) {
> > >  			umode_t mode = (*attr)->mode;
> > >  
> > >  			/*
> > > @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
> > >  	}
> > >  
> > >  	if (grp->bin_attrs) {
> > > -		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> > > +		for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> > > +			umode_t mode = (*bin_attr)->attr.mode;
> > > +
> > >  			if (update)
> > >  				kernfs_remove_by_name(parent,
> > >  						(*bin_attr)->attr.name);
> > > +			if (grp->is_visible) {
> > > +				mode = grp->is_visible(kobj,
> > > +						       &(*bin_attr)->attr, i);
> > 
> > With this, if 'n' is the number of non-binary attributes,
> > 
> > for i < n:
> > 	The index passed to is_visible points to a non-binary attribute.
> > for i >= n:
> > 	The index passed to is_visible points to the (index - n)th binary
> > 	attribute.
> > 
> > Unless I am missing something, this is not explained anywhere, but it is
> > not entirely trivial to understand. I think it should be documented.
> 
> I agree, make i the number of the bin attribute and that should solve
> this issue.
> 
No, that would conflict with the "normal" use of is_visible for non-binary
attributes, and make the index all but useless, since the is_visible function
would have to search through all the attributes anyway to figure out which one
is being checked.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221166

Hi Greg & Guenter,

On 08/09/15 16:30, Guenter Roeck wrote:
> On Tue, Sep 08, 2015 at 12:10:02PM -0700, Greg KH wrote:
>> On Tue, Sep 08, 2015 at 08:30:13AM -0700, Guenter Roeck wrote:
>>> Emilio,
>>>
>>> On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
>>>> According to the sysfs header file:
>>>>
>>>>      "The returned value will replace static permissions defined in
>>>>       struct attribute or struct bin_attribute."
>>>>
>>>> but this isn't the case, as is_visible is only called on
>>>> struct attribute only. This patch adds the code paths required
>>>> to support is_visible() on binary attributes.
>>>>
>>>> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>>>> ---
>>>>   fs/sysfs/group.c | 22 ++++++++++++++++++----
>>>>   1 file changed, 18 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
>>>> index 39a0199..eb6996a 100644
>>>> --- a/fs/sysfs/group.c
>>>> +++ b/fs/sysfs/group.c
>>>> @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>>>>   {
>>>>   	struct attribute *const *attr;
>>>>   	struct bin_attribute *const *bin_attr;
>>>> -	int error = 0, i;
>>>> +	int error = 0, i = 0;
>>>>
>>>>   	if (grp->attrs) {
>>>> -		for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
>>>> +		for (attr = grp->attrs; *attr && !error; i++, attr++) {
>>>>   			umode_t mode = (*attr)->mode;
>>>>
>>>>   			/*
>>>> @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
>>>>   	}
>>>>
>>>>   	if (grp->bin_attrs) {
>>>> -		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
>>>> +		for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
>>>> +			umode_t mode = (*bin_attr)->attr.mode;
>>>> +
>>>>   			if (update)
>>>>   				kernfs_remove_by_name(parent,
>>>>   						(*bin_attr)->attr.name);
>>>> +			if (grp->is_visible) {
>>>> +				mode = grp->is_visible(kobj,
>>>> +						       &(*bin_attr)->attr, i);
>>>
>>> With this, if 'n' is the number of non-binary attributes,
>>>
>>> for i < n:
>>> 	The index passed to is_visible points to a non-binary attribute.
>>> for i >= n:
>>> 	The index passed to is_visible points to the (index - n)th binary
>>> 	attribute.
>>>
>>> Unless I am missing something, this is not explained anywhere, but it is
>>> not entirely trivial to understand. I think it should be documented.

I agree. I couldn't find any mention of what this int was supposed to be 
by looking at Documentation/ (is_visible is not even mentioned :/) or 
include/linux/sysfs.h. Once we settle on something I'll document it 
before sending a v2.

By the way, I wrote a quick coccinelle script to match is_visible() 
users which reference the index (included below), and it found 
references to drivers which do not seem to use any binary attributes, so 
I believe changing the index meaning shouldn't be an issue.

>> I agree, make i the number of the bin attribute and that should solve
>> this issue.
>>
> No, that would conflict with the "normal" use of is_visible for non-binary
> attributes, and make the index all but useless, since the is_visible function
> would have to search through all the attributes anyway to figure out which one
> is being checked.

Yeah, using the same indexes would be somewhat pointless, although not 
many seem to be using it anyway (only 14 files matched). Others seem to 
be comparing the attr* instead. An alternative would be to use negative 
indexes for binary attributes and positive indexes for normal attributes.

Cheers,
Emilio

---->8----


// Find out is_visible() users which reference the index
// somehow

virtual report

@ func @
identifier visiblefun, i, j, n;
@@

visiblefun(struct kobject *i, struct attribute *j, int n)
{
<+...n...+>
}

@ attrib depends on func @
identifier aops;
identifier func.visiblefun;
position p0;
@@

struct attribute_group aops@p0 = {
...,
.is_visible = visiblefun,
...,
};

@script:python b_report depends on report@
p0 << attrib.p0;
@@

msg = "Suspicious is_visible(), please check."
coccilib.report.print_report(p0[0], msg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221172

Hi Emilio,

On 09/08/2015 05:51 PM, Emilio López wrote:
> Hi Greg & Guenter,
>
[ ... ]
>>>>
>>>> Unless I am missing something, this is not explained anywhere, but it is
>>>> not entirely trivial to understand. I think it should be documented.
>
> I agree. I couldn't find any mention of what this int was supposed to be by looking at Documentation/ (is_visible is not even mentioned :/) or include/linux/sysfs.h. Once we settle on something I'll document it before sending a v2.
>
In the include file ? No strong preference, though.

> By the way, I wrote a quick coccinelle script to match is_visible() users which reference the index (included below), and it found references to drivers which do not seem to use any binary attributes, so I believe changing the index meaning shouldn't be an issue.
>
Good.

>>> I agree, make i the number of the bin attribute and that should solve
>>> this issue.
>>>
>> No, that would conflict with the "normal" use of is_visible for non-binary
>> attributes, and make the index all but useless, since the is_visible function
>> would have to search through all the attributes anyway to figure out which one
>> is being checked.
>
> Yeah, using the same indexes would be somewhat pointless, although not many seem to be using it anyway (only 14 files matched). Others seem to be comparing the attr* instead. An alternative would be to use negative indexes for binary attributes and positive indexes for normal attributes.
>
... and I probably wrote or reviewed a significant percentage of those ;-).

Using negative numbers for binary attributes is an interesting idea.
Kind of unusual, though. Greg, any thoughts on that ?

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221216

On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:
> Hi Emilio,
> 
> On 09/08/2015 05:51 PM, Emilio López wrote:
> >Hi Greg & Guenter,
> >
> [ ... ]
> >>>>
> >>>>Unless I am missing something, this is not explained anywhere, but it is
> >>>>not entirely trivial to understand. I think it should be documented.
> >
> >I agree. I couldn't find any mention of what this int was supposed to be by looking at Documentation/ (is_visible is not even mentioned :/) or include/linux/sysfs.h. Once we settle on something I'll document it before sending a v2.
> >
> In the include file ? No strong preference, though.
> 
> >By the way, I wrote a quick coccinelle script to match is_visible() users which reference the index (included below), and it found references to drivers which do not seem to use any binary attributes, so I believe changing the index meaning shouldn't be an issue.
> >
> Good.
> 
> >>>I agree, make i the number of the bin attribute and that should solve
> >>>this issue.
> >>>
> >>No, that would conflict with the "normal" use of is_visible for non-binary
> >>attributes, and make the index all but useless, since the is_visible function
> >>would have to search through all the attributes anyway to figure out which one
> >>is being checked.
> >
> >Yeah, using the same indexes would be somewhat pointless, although not many seem to be using it anyway (only 14 files matched). Others seem to be comparing the attr* instead. An alternative would be to use negative indexes for binary attributes and positive indexes for normal attributes.
> >
> ... and I probably wrote or reviewed a significant percentage of those ;-).
> 
> Using negative numbers for binary attributes is an interesting idea.
> Kind of unusual, though. Greg, any thoughts on that ?

Ick, no, that's a mess, maybe we just could drop the index alltogether?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221225

On 09/08/2015 08:58 PM, Greg KH wrote:
> On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:
>> Hi Emilio,
>>
>> On 09/08/2015 05:51 PM, Emilio López wrote:
>>> Hi Greg & Guenter,
>>>
>> [ ... ]
>>>>>>
>>>>>> Unless I am missing something, this is not explained anywhere, but it is
>>>>>> not entirely trivial to understand. I think it should be documented.
>>>
>>> I agree. I couldn't find any mention of what this int was supposed to be by looking at Documentation/ (is_visible is not even mentioned :/) or include/linux/sysfs.h. Once we settle on something I'll document it before sending a v2.
>>>
>> In the include file ? No strong preference, though.
>>
>>> By the way, I wrote a quick coccinelle script to match is_visible() users which reference the index (included below), and it found references to drivers which do not seem to use any binary attributes, so I believe changing the index meaning shouldn't be an issue.
>>>
>> Good.
>>
>>>>> I agree, make i the number of the bin attribute and that should solve
>>>>> this issue.
>>>>>
>>>> No, that would conflict with the "normal" use of is_visible for non-binary
>>>> attributes, and make the index all but useless, since the is_visible function
>>>> would have to search through all the attributes anyway to figure out which one
>>>> is being checked.
>>>
>>> Yeah, using the same indexes would be somewhat pointless, although not many seem to be using it anyway (only 14 files matched). Others seem to be comparing the attr* instead. An alternative would be to use negative indexes for binary attributes and positive indexes for normal attributes.
>>>
>> ... and I probably wrote or reviewed a significant percentage of those ;-).
>>
>> Using negative numbers for binary attributes is an interesting idea.
>> Kind of unusual, though. Greg, any thoughts on that ?
>
> Ick, no, that's a mess, maybe we just could drop the index alltogether?
>

No, please don't. Having to manually compare dozens of index pointers would be
even more of a mess.

Guenter


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221412

On 09/09/15 01:12, Guenter Roeck wrote:
> On 09/08/2015 08:58 PM, Greg KH wrote:
>> On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:
>>> Hi Emilio,
>>>
>>> On 09/08/2015 05:51 PM, Emilio López wrote:
>>>> Hi Greg & Guenter,
>>>>
>>> [ ... ]
>>>>>>>
>>>>>>> Unless I am missing something, this is not explained anywhere,
>>>>>>> but it is
>>>>>>> not entirely trivial to understand. I think it should be documented.
>>>>
>>>> I agree. I couldn't find any mention of what this int was supposed
>>>> to be by looking at Documentation/ (is_visible is not even mentioned
>>>> :/) or include/linux/sysfs.h. Once we settle on something I'll
>>>> document it before sending a v2.
>>>>
>>> In the include file ? No strong preference, though.
>>>
>>>> By the way, I wrote a quick coccinelle script to match is_visible()
>>>> users which reference the index (included below), and it found
>>>> references to drivers which do not seem to use any binary
>>>> attributes, so I believe changing the index meaning shouldn't be an
>>>> issue.
>>>>
>>> Good.
>>>
>>>>>> I agree, make i the number of the bin attribute and that should solve
>>>>>> this issue.
>>>>>>
>>>>> No, that would conflict with the "normal" use of is_visible for
>>>>> non-binary
>>>>> attributes, and make the index all but useless, since the
>>>>> is_visible function
>>>>> would have to search through all the attributes anyway to figure
>>>>> out which one
>>>>> is being checked.
>>>>
>>>> Yeah, using the same indexes would be somewhat pointless, although
>>>> not many seem to be using it anyway (only 14 files matched). Others
>>>> seem to be comparing the attr* instead. An alternative would be to
>>>> use negative indexes for binary attributes and positive indexes for
>>>> normal attributes.
>>>>
>>> ... and I probably wrote or reviewed a significant percentage of
>>> those ;-).
>>>
>>> Using negative numbers for binary attributes is an interesting idea.
>>> Kind of unusual, though. Greg, any thoughts on that ?
>>
>> Ick, no, that's a mess, maybe we just could drop the index alltogether?
>>
>
> No, please don't. Having to manually compare dozens of index pointers
> would be
> even more of a mess.

So, what about keeping it the way it is in the patch, and documenting it 
thoroughly? Otherwise, we could introduce another "is_bin_visible" 
function to do this same thing but just on binary attributes, but I'd 
rather not add a new function pointer if possible.

Cheers,
Emilio
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221457

On 09/09/2015 06:14 AM, Emilio López wrote:
> On 09/09/15 01:12, Guenter Roeck wrote:
>> On 09/08/2015 08:58 PM, Greg KH wrote:
>>> On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:
>>>> Hi Emilio,
>>>>
>>>> On 09/08/2015 05:51 PM, Emilio López wrote:
>>>>> Hi Greg & Guenter,
>>>>>
>>>> [ ... ]
>>>>>>>>
>>>>>>>> Unless I am missing something, this is not explained anywhere,
>>>>>>>> but it is
>>>>>>>> not entirely trivial to understand. I think it should be documented.
>>>>>
>>>>> I agree. I couldn't find any mention of what this int was supposed
>>>>> to be by looking at Documentation/ (is_visible is not even mentioned
>>>>> :/) or include/linux/sysfs.h. Once we settle on something I'll
>>>>> document it before sending a v2.
>>>>>
>>>> In the include file ? No strong preference, though.
>>>>
>>>>> By the way, I wrote a quick coccinelle script to match is_visible()
>>>>> users which reference the index (included below), and it found
>>>>> references to drivers which do not seem to use any binary
>>>>> attributes, so I believe changing the index meaning shouldn't be an
>>>>> issue.
>>>>>
>>>> Good.
>>>>
>>>>>>> I agree, make i the number of the bin attribute and that should solve
>>>>>>> this issue.
>>>>>>>
>>>>>> No, that would conflict with the "normal" use of is_visible for
>>>>>> non-binary
>>>>>> attributes, and make the index all but useless, since the
>>>>>> is_visible function
>>>>>> would have to search through all the attributes anyway to figure
>>>>>> out which one
>>>>>> is being checked.
>>>>>
>>>>> Yeah, using the same indexes would be somewhat pointless, although
>>>>> not many seem to be using it anyway (only 14 files matched). Others
>>>>> seem to be comparing the attr* instead. An alternative would be to
>>>>> use negative indexes for binary attributes and positive indexes for
>>>>> normal attributes.
>>>>>
>>>> ... and I probably wrote or reviewed a significant percentage of
>>>> those ;-).
>>>>
>>>> Using negative numbers for binary attributes is an interesting idea.
>>>> Kind of unusual, though. Greg, any thoughts on that ?
>>>
>>> Ick, no, that's a mess, maybe we just could drop the index alltogether?
>>>
>>
>> No, please don't. Having to manually compare dozens of index pointers
>> would be
>> even more of a mess.
>
> So, what about keeping it the way it is in the patch, and documenting it thoroughly? Otherwise, we could introduce another "is_bin_visible" function to do this same thing but just on binary attributes, but I'd rather not add a new function pointer if possible.
>

I would prefer to keep and document it.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221617

On Wed, Sep 09, 2015 at 10:14:46AM -0300, Emilio López wrote:
> On 09/09/15 01:12, Guenter Roeck wrote:
> >On 09/08/2015 08:58 PM, Greg KH wrote:
> >>On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:
> >>>Hi Emilio,
> >>>
> >>>On 09/08/2015 05:51 PM, Emilio López wrote:
> >>>>Hi Greg & Guenter,
> >>>>
> >>>[ ... ]
> >>>>>>>
> >>>>>>>Unless I am missing something, this is not explained anywhere,
> >>>>>>>but it is
> >>>>>>>not entirely trivial to understand. I think it should be documented.
> >>>>
> >>>>I agree. I couldn't find any mention of what this int was supposed
> >>>>to be by looking at Documentation/ (is_visible is not even mentioned
> >>>>:/) or include/linux/sysfs.h. Once we settle on something I'll
> >>>>document it before sending a v2.
> >>>>
> >>>In the include file ? No strong preference, though.
> >>>
> >>>>By the way, I wrote a quick coccinelle script to match is_visible()
> >>>>users which reference the index (included below), and it found
> >>>>references to drivers which do not seem to use any binary
> >>>>attributes, so I believe changing the index meaning shouldn't be an
> >>>>issue.
> >>>>
> >>>Good.
> >>>
> >>>>>>I agree, make i the number of the bin attribute and that should solve
> >>>>>>this issue.
> >>>>>>
> >>>>>No, that would conflict with the "normal" use of is_visible for
> >>>>>non-binary
> >>>>>attributes, and make the index all but useless, since the
> >>>>>is_visible function
> >>>>>would have to search through all the attributes anyway to figure
> >>>>>out which one
> >>>>>is being checked.
> >>>>
> >>>>Yeah, using the same indexes would be somewhat pointless, although
> >>>>not many seem to be using it anyway (only 14 files matched). Others
> >>>>seem to be comparing the attr* instead. An alternative would be to
> >>>>use negative indexes for binary attributes and positive indexes for
> >>>>normal attributes.
> >>>>
> >>>... and I probably wrote or reviewed a significant percentage of
> >>>those ;-).
> >>>
> >>>Using negative numbers for binary attributes is an interesting idea.
> >>>Kind of unusual, though. Greg, any thoughts on that ?
> >>
> >>Ick, no, that's a mess, maybe we just could drop the index alltogether?
> >>
> >
> >No, please don't. Having to manually compare dozens of index pointers
> >would be
> >even more of a mess.
> 
> So, what about keeping it the way it is in the patch, and documenting it
> thoroughly? Otherwise, we could introduce another "is_bin_visible" function
> to do this same thing but just on binary attributes, but I'd rather not add
> a new function pointer if possible.

is_bin_visiable makes sense to me instead of trying to overload the
index number in some crazy way.  There's no issue with adding another
function pointer.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web