| Message ID | 1441714066-5599-2-git-send-email-emilio.lopez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)