[PATCH 0/3] platform/chrome: vboot context support

[PATCH 0/3] platform/chrome: vboot context support

[Emilio López]

Hi everyone,

This series adds support for reading and writing the verified boot context
nvram space on the EC using the cros_ec sysfs interface.

The first patch fixes is_visible() functionality, making it work for binary
attributes as well as normal ones. This is needed so the sysfs group
can be hidden when the EC doesn't offer any space for the context.

The second patch is the actual code implementing the interface to read
and write the context data.

The third patch adds the DT properties on peach boards which, judging by
the vendor tree, use the EC to store the verified boot context.

The series was tested on a peach pi and was found to work OK there. As
always, all comments and further tests are welcome :)

Cheers!
Emilio

Emilio López (3):
  sysfs: Fix is_visible() support for binary attributes
  platform/chrome: Support reading/writing the vboot context
  ARM: dts: Enable EC vboot context support on Peach boards

 Documentation/devicetree/bindings/mfd/cros-ec.txt |   4 +
 arch/arm/boot/dts/exynos5420-peach-pit.dts        |   1 +
 arch/arm/boot/dts/exynos5800-peach-pi.dts         |   1 +
 drivers/platform/chrome/Makefile                  |   5 +-
 drivers/platform/chrome/cros_ec_dev.c             |   1 +
 drivers/platform/chrome/cros_ec_vbc.c             | 137 ++++++++++++++++++++++
 fs/sysfs/group.c                                  |  22 +++-
 include/linux/mfd/cros_ec.h                       |   1 +
 8 files changed, 167 insertions(+), 5 deletions(-)
 create mode 100644 drivers/platform/chrome/cros_ec_vbc.c

-- 
2.1.4

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

[Emilio López]

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

[PATCH 2/3] platform/chrome: Support reading/writing the vboot context

[Emilio López]

Some EC implementations include a small nvram space used to store
verified boot context data. This patch offers a way to expose this
data to userspace.

Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---
 Documentation/devicetree/bindings/mfd/cros-ec.txt |   4 +
 drivers/platform/chrome/Makefile                  |   5 +-
 drivers/platform/chrome/cros_ec_dev.c             |   1 +
 drivers/platform/chrome/cros_ec_vbc.c             | 137 ++++++++++++++++++++++
 include/linux/mfd/cros_ec.h                       |   1 +
 5 files changed, 147 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/chrome/cros_ec_vbc.c

diff --git a/Documentation/devicetree/bindings/mfd/cros-ec.txt b/Documentation/devicetree/bindings/mfd/cros-ec.txt
index 1777916..136e0c2 100644
--- a/Documentation/devicetree/bindings/mfd/cros-ec.txt
+++ b/Documentation/devicetree/bindings/mfd/cros-ec.txt
@@ -34,6 +34,10 @@ Required properties (LPC):
 - compatible: "google,cros-ec-lpc"
 - reg: List of (IO address, size) pairs defining the interface uses
 
+Optional properties (all):
+- google,has-vbc-nvram: Some implementations of the EC include a small
+  nvram space used to store verified boot context data. This boolean flag
+  is used to specify whether this nvram is present or not.
 
 Example for I2C:
 
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 4a11b01..787be61 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -1,7 +1,10 @@
 
 obj-$(CONFIG_CHROMEOS_LAPTOP)	+= chromeos_laptop.o
 obj-$(CONFIG_CHROMEOS_PSTORE)	+= chromeos_pstore.o
-cros_ec_devs-objs               := cros_ec_dev.o cros_ec_sysfs.o cros_ec_lightbar.o
+cros_ec_devs-objs               := cros_ec_dev.o
+cros_ec_devs-objs               += cros_ec_lightbar.o
+cros_ec_devs-objs               += cros_ec_sysfs.o
+cros_ec_devs-objs               += cros_ec_vbc.o
 obj-$(CONFIG_CROS_EC_CHARDEV)   += cros_ec_devs.o
 obj-$(CONFIG_CROS_EC_LPC)       += cros_ec_lpc.o
 obj-$(CONFIG_CROS_EC_PROTO)	+= cros_ec_proto.o
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index e8fcdc2..d19263f 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -32,6 +32,7 @@ static int ec_major;
 static const struct attribute_group *cros_ec_groups[] = {
 	&cros_ec_attr_group,
 	&cros_ec_lightbar_attr_group,
+	&cros_ec_vbc_attr_group,
 	NULL,
 };
 
diff --git a/drivers/platform/chrome/cros_ec_vbc.c b/drivers/platform/chrome/cros_ec_vbc.c
new file mode 100644
index 0000000..f6ca8b6
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_vbc.c
@@ -0,0 +1,137 @@
+/*
+ * cros_ec_vbc - Expose the vboot context nvram to userspace
+ *
+ * Copyright (C) 2015 Collabora Ltd.
+ *
+ * based on vendor driver,
+ *
+ * Copyright (C) 2012 The Chromium OS Authors
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/slab.h>
+
+static ssize_t vboot_context_read(struct file *filp, struct kobject *kobj,
+				  struct bin_attribute *att, char *buf,
+				  loff_t pos, size_t count)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
+					      class_dev);
+	struct cros_ec_device *ecdev = ec->ec_dev;
+	struct ec_params_vbnvcontext *params;
+	struct cros_ec_command *msg;
+	int err;
+	const size_t para_sz = sizeof(struct ec_params_vbnvcontext);
+	const size_t resp_sz = sizeof(struct ec_response_vbnvcontext);
+	const size_t payload = max(para_sz, resp_sz);
+
+	msg = kmalloc(sizeof(*msg) + payload, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	params = (struct ec_params_vbnvcontext *)msg->data;
+	params->op = EC_VBNV_CONTEXT_OP_READ;
+
+	msg->version = EC_VER_VBNV_CONTEXT;
+	msg->command = EC_CMD_VBNV_CONTEXT;
+	msg->outsize = sizeof(params->op);
+	msg->insize = resp_sz;
+
+	err = cros_ec_cmd_xfer(ecdev, msg);
+	if (err < 0) {
+		dev_err(dev, "Error sending read request: %d\n", err);
+		kfree(msg);
+		return err;
+	}
+
+	BUILD_BUG_ON(resp_sz > PAGE_SIZE);
+	memcpy(buf, msg->data, resp_sz);
+
+	kfree(msg);
+	return resp_sz;
+}
+
+static ssize_t vboot_context_write(struct file *filp, struct kobject *kobj,
+				   struct bin_attribute *attr, char *buf,
+				   loff_t pos, size_t count)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
+					      class_dev);
+	struct cros_ec_device *ecdev = ec->ec_dev;
+	struct ec_params_vbnvcontext *params;
+	struct cros_ec_command *msg;
+	int err;
+	const size_t para_sz = sizeof(*params);
+	const size_t data_sz = sizeof(params->block);
+
+	/* Only write full values */
+	if (count != data_sz)
+		return -EINVAL;
+
+	msg = kmalloc(sizeof(*msg) + para_sz, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	params = (struct ec_params_vbnvcontext *)msg->data;
+	params->op = EC_VBNV_CONTEXT_OP_WRITE;
+	memcpy(params->block, buf, data_sz);
+
+	msg->version = EC_VER_VBNV_CONTEXT;
+	msg->command = EC_CMD_VBNV_CONTEXT;
+	msg->outsize = para_sz;
+	msg->insize = 0;
+
+	err = cros_ec_cmd_xfer(ecdev, msg);
+	if (err < 0) {
+		dev_err(dev, "Error sending write request: %d\n", err);
+		kfree(msg);
+		return err;
+	}
+
+	kfree(msg);
+	return data_sz;
+}
+
+static umode_t cros_ec_vbc_is_visible(struct kobject *kobj,
+				      struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
+					      class_dev);
+	struct device_node *np = ec->ec_dev->dev->of_node;
+
+	if (IS_ENABLED(CONFIG_OF) && np) {
+		if (of_property_read_bool(np, "google,has-vbc-nvram"))
+			return a->mode;
+	}
+
+	return 0;
+}
+
+static BIN_ATTR_RW(vboot_context, 16);
+
+static struct bin_attribute *cros_ec_vbc_bin_attrs[] = {
+	&bin_attr_vboot_context,
+	NULL
+};
+
+struct attribute_group cros_ec_vbc_attr_group = {
+	.name = "vbc",
+	.bin_attrs = cros_ec_vbc_bin_attrs,
+	.is_visible = cros_ec_vbc_is_visible,
+};
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index da72671..494682c 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -255,5 +255,6 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev);
 /* sysfs stuff */
 extern struct attribute_group cros_ec_attr_group;
 extern struct attribute_group cros_ec_lightbar_attr_group;
+extern struct attribute_group cros_ec_vbc_attr_group;
 
 #endif /* __LINUX_MFD_CROS_EC_H */
-- 
2.1.4

[PATCH 3/3] ARM: dts: Enable EC vboot context support on Peach boards

[Emilio López]

The Peach boards use the EC to store the vboot context information,
so add the corresponding properties on the EC node to indicate so.

Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---
 arch/arm/boot/dts/exynos5420-peach-pit.dts | 1 +
 arch/arm/boot/dts/exynos5800-peach-pi.dts  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
index 8f4d76c..ac02fb4 100644
--- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
@@ -935,6 +935,7 @@
 		pinctrl-0 = <&ec_spi_cs &ec_irq>;
 		reg = <0>;
 		spi-max-frequency = <3125000>;
+		google,has-vbc-nvram;
 
 		controller-data {
 			samsung,spi-feedback-delay = <1>;
diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index 7d5b386..b60dec0 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -898,6 +898,7 @@
 		pinctrl-0 = <&ec_spi_cs &ec_irq>;
 		reg = <0>;
 		spi-max-frequency = <3125000>;
+		google,has-vbc-nvram;
 
 		controller-data {
 			samsung,spi-feedback-delay = <1>;
-- 
2.1.4

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

[Guenter Roeck]

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

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

[Greg KH]

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

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

[Guenter Roeck]

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

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

[Emilio López]

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)

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

[Guenter Roeck]

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

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

[Greg KH]

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

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

[Guenter Roeck]

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

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

[Emilio López]

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

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

[Greg KH]

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

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

[Guenter Roeck]

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