On Fri, Jan 16, 2015 at 07:22:25PM -0500, Vivien Didelot wrote: > Hi Greg, > > > On Fri, Jan 16, 2015 at 04:29:10PM -0500, Vivien Didelot wrote: > > > Using the optional is_visible function, it is actually possible to > > > either hide an attribute, or add a new permission, but not remove > > > one. > > > > What code wants to remove attributes? > > Sorry, I meant removing a permission. Actually, drivers hide attributes > (if a feature isn't supported by a device) by returning 0 in is_visible. > > E.g, if we consider a read-only attribute, a driver can add the write > permission by returning S_IWUSR, but removing S_IRUGO isn't possible.
Sorry, I meant to say, "what code wants to remove permissions". > > > This commit uses all the UGO bits returned by is_visible instead of > > > OR'ing them with the default attribute mode. > > > > > > Concretely, this allows a driver to use macros like DEVICE_ATTR_RW > > > to > > > set the attribute show and store functions and remove the S_IWUSR > > > permission in is_visible if the implementation doesn't provide a > > > setter. > > > > What bus wants to do this? > > Some high level frameworks such as DSA. My motivation behind this was to > clarify how modes are set for temperature attributes in DSA. Optional > functions can be provided by switch drivers to get temperatures or set > limits. I hope the following patch helps clarifying this point: > https://gist.github.com/vivien/72734ba0673ad0b79a6b > > (I Cc: Guenter as he is the author of NET_DSA_HWMON, see 51579c3). > > > > Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com> > > > --- > > > fs/sysfs/group.c | 12 +++++++----- > > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > > > index 7d2a860..a8cfe03 100644 > > > --- a/fs/sysfs/group.c > > > +++ b/fs/sysfs/group.c > > > @@ -41,7 +41,7 @@ static int create_files(struct kernfs_node > > > *parent, struct kobject *kobj, > > > > > > if (grp->attrs) { > > > for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) { > > > - umode_t mode = 0; > > > + umode_t mode = (*attr)->mode; > > > > > > /* > > > * In update mode, we're changing the permissions or > > > @@ -51,13 +51,15 @@ static int create_files(struct kernfs_node > > > *parent, struct kobject *kobj, > > > if (update) > > > kernfs_remove_by_name(parent, (*attr)->name); > > > if (grp->is_visible) { > > > - mode = grp->is_visible(kobj, *attr, i); > > > - if (!mode) > > > + umode_t ugo = grp->is_visible(kobj, *attr, i); > > > + > > > + if (!(ugo & S_IRWXUGO)) > > > continue; > > > + > > > + mode = (mode & ~S_IRWXUGO) | (ugo & S_IRWXUGO); > > > > Please document what you are doing here in the code, it's not obvious > > at first glance. You somehow ignored this comment :( -- 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/