From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754994AbbIHPaW (ORCPT ); Tue, 8 Sep 2015 11:30:22 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:33760 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754535AbbIHPaT (ORCPT ); Tue, 8 Sep 2015 11:30:19 -0400 Date: Tue, 8 Sep 2015 08:30:13 -0700 From: Guenter Roeck To: Emilio =?iso-8859-1?Q?L=F3pez?= Cc: gregkh@linuxfoundation.org, olof@lixom.net, kgene@kernel.org, k.kozlowski@samsung.com, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org Subject: Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes Message-ID: <20150908153013.GA6758@roeck-us.net> References: <1441714066-5599-1-git-send-email-emilio.lopez@collabora.co.uk> <1441714066-5599-2-git-send-email-emilio.lopez@collabora.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1441714066-5599-2-git-send-email-emilio.lopez@collabora.co.uk> User-Agent: Mutt/1.5.23 (2014-03-12) X-Authenticated_sender: guenter@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: guenter@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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