* [PATCH v2 0/3] platform/chrome: vboot context support
@ 2015-09-14 12:34 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-14 12:34 UTC (permalink / raw)
To: gregkh, olof, kgene, k.kozlowski, linux
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-samsung-soc,
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 improves 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: Support is_visible() on 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 | 17 ++-
include/linux/mfd/cros_ec.h | 1 +
include/linux/sysfs.h | 18 ++-
9 files changed, 178 insertions(+), 7 deletions(-)
create mode 100644 drivers/platform/chrome/cros_ec_vbc.c
--
2.1.4
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 0/3] platform/chrome: vboot context support
@ 2015-09-14 12:34 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-14 12:34 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
olof-nZhT3qVonbNeoWH0uzbU5w, kgene-DgEjT+Ai2ygdnm+yROfE0A,
k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ, linux-0h96xk9xTtrk1uMJSBkQmQ
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA, 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 improves 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: Support is_visible() on 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 | 17 ++-
include/linux/mfd/cros_ec.h | 1 +
include/linux/sysfs.h | 18 ++-
9 files changed, 178 insertions(+), 7 deletions(-)
create mode 100644 drivers/platform/chrome/cros_ec_vbc.c
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 0/3] platform/chrome: vboot context support
@ 2015-09-14 12:34 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-14 12:34 UTC (permalink / raw)
To: linux-arm-kernel
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 improves 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: Support is_visible() on 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 | 17 ++-
include/linux/mfd/cros_ec.h | 1 +
include/linux/sysfs.h | 18 ++-
9 files changed, 178 insertions(+), 7 deletions(-)
create mode 100644 drivers/platform/chrome/cros_ec_vbc.c
--
2.1.4
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/3] sysfs: Support is_visible() on binary attributes
2015-09-14 12:34 ` Emilio López
@ 2015-09-14 12:34 ` Emilio López
-1 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-14 12:34 UTC (permalink / raw)
To: gregkh, olof, kgene, k.kozlowski, linux
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-samsung-soc,
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 introduces a new is_bin_visible() function to implement
the same functionality for binary attributes, and updates documentation
accordingly.
Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
---
Changes from v1:
- Don't overload is_visible, introduce is_bin_visible instead as
discussed on the list.
fs/sysfs/group.c | 17 +++++++++++++++--
include/linux/sysfs.h | 18 ++++++++++++++----
2 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 39a0199..51b56e6 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -73,13 +73,26 @@ 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 (i = 0, 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_bin_visible) {
+ mode = grp->is_bin_visible(kobj, *bin_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;
}
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9f65758..2f66050 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -64,10 +64,18 @@ do { \
* a new subdirectory with this name.
* @is_visible: Optional: Function to return permissions associated with an
* attribute of the group. Will be called repeatedly for each
- * attribute in the group. Only read/write permissions as well as
- * SYSFS_PREALLOC are accepted. Must return 0 if an attribute is
- * not visible. The returned value will replace static permissions
- * defined in struct attribute or struct bin_attribute.
+ * non-binary attribute in the group. Only read/write
+ * permissions as well as SYSFS_PREALLOC are accepted. Must
+ * return 0 if an attribute is not visible. The returned value
+ * will replace static permissions defined in struct attribute.
+ * @is_bin_visible:
+ * Optional: Function to return permissions associated with a
+ * binary attribute of the group. Will be called repeatedly
+ * for each binary attribute in the group. Only read/write
+ * permissions as well as SYSFS_PREALLOC are accepted. Must
+ * return 0 if a binary attribute is not visible. The returned
+ * value will replace static permissions defined in
+ * struct bin_attribute.
* @attrs: Pointer to NULL terminated list of attributes.
* @bin_attrs: Pointer to NULL terminated list of binary attributes.
* Either attrs or bin_attrs or both must be provided.
@@ -76,6 +84,8 @@ struct attribute_group {
const char *name;
umode_t (*is_visible)(struct kobject *,
struct attribute *, int);
+ umode_t (*is_bin_visible)(struct kobject *,
+ struct bin_attribute *, int);
struct attribute **attrs;
struct bin_attribute **bin_attrs;
};
--
2.1.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 1/3] sysfs: Support is_visible() on binary attributes
@ 2015-09-14 12:34 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-14 12:34 UTC (permalink / raw)
To: linux-arm-kernel
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 introduces a new is_bin_visible() function to implement
the same functionality for binary attributes, and updates documentation
accordingly.
Signed-off-by: Emilio L?pez <emilio.lopez@collabora.co.uk>
---
Changes from v1:
- Don't overload is_visible, introduce is_bin_visible instead as
discussed on the list.
fs/sysfs/group.c | 17 +++++++++++++++--
include/linux/sysfs.h | 18 ++++++++++++++----
2 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 39a0199..51b56e6 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -73,13 +73,26 @@ 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 (i = 0, 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_bin_visible) {
+ mode = grp->is_bin_visible(kobj, *bin_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;
}
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9f65758..2f66050 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -64,10 +64,18 @@ do { \
* a new subdirectory with this name.
* @is_visible: Optional: Function to return permissions associated with an
* attribute of the group. Will be called repeatedly for each
- * attribute in the group. Only read/write permissions as well as
- * SYSFS_PREALLOC are accepted. Must return 0 if an attribute is
- * not visible. The returned value will replace static permissions
- * defined in struct attribute or struct bin_attribute.
+ * non-binary attribute in the group. Only read/write
+ * permissions as well as SYSFS_PREALLOC are accepted. Must
+ * return 0 if an attribute is not visible. The returned value
+ * will replace static permissions defined in struct attribute.
+ * @is_bin_visible:
+ * Optional: Function to return permissions associated with a
+ * binary attribute of the group. Will be called repeatedly
+ * for each binary attribute in the group. Only read/write
+ * permissions as well as SYSFS_PREALLOC are accepted. Must
+ * return 0 if a binary attribute is not visible. The returned
+ * value will replace static permissions defined in
+ * struct bin_attribute.
* @attrs: Pointer to NULL terminated list of attributes.
* @bin_attrs: Pointer to NULL terminated list of binary attributes.
* Either attrs or bin_attrs or both must be provided.
@@ -76,6 +84,8 @@ struct attribute_group {
const char *name;
umode_t (*is_visible)(struct kobject *,
struct attribute *, int);
+ umode_t (*is_bin_visible)(struct kobject *,
+ struct bin_attribute *, int);
struct attribute **attrs;
struct bin_attribute **bin_attrs;
};
--
2.1.4
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
2015-09-14 12:34 ` Emilio López
@ 2015-09-14 12:34 ` Emilio López
-1 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-14 12:34 UTC (permalink / raw)
To: gregkh, olof, kgene, k.kozlowski, linux
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-samsung-soc,
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>
---
Changes from v1:
- Use is_bin_visible instead of is_visible
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..a0e8d38
--- /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 bin_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->attr.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_bin_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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
@ 2015-09-14 12:34 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-14 12:34 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
Changes from v1:
- Use is_bin_visible instead of is_visible
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..a0e8d38
--- /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 bin_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->attr.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_bin_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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 3/3] ARM: dts: Enable EC vboot context support on Peach boards
2015-09-14 12:34 ` Emilio López
@ 2015-09-14 12:34 ` Emilio López
-1 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-14 12:34 UTC (permalink / raw)
To: gregkh, olof, kgene, k.kozlowski, linux
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-samsung-soc,
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>
---
Changes from v1:
- none
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 3/3] ARM: dts: Enable EC vboot context support on Peach boards
@ 2015-09-14 12:34 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-14 12:34 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
Changes from v1:
- none
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/3] sysfs: Support is_visible() on binary attributes
2015-09-14 12:34 ` Emilio López
@ 2015-09-14 15:33 ` Guenter Roeck
-1 siblings, 0 replies; 28+ messages in thread
From: Guenter Roeck @ 2015-09-14 15:33 UTC (permalink / raw)
To: Emilio López, gregkh, olof, kgene, k.kozlowski
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-samsung-soc
On 09/14/2015 05:34 AM, 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 introduces a new is_bin_visible() function to implement
> the same functionality for binary attributes, and updates documentation
> accordingly.
>
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
Nitpick below, but otherwise looks ok to me.
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Guenter
> ---
>
> Changes from v1:
> - Don't overload is_visible, introduce is_bin_visible instead as
> discussed on the list.
>
> fs/sysfs/group.c | 17 +++++++++++++++--
> include/linux/sysfs.h | 18 ++++++++++++++----
> 2 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 39a0199..51b56e6 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -73,13 +73,26 @@ 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 (i = 0, 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_bin_visible) {
> + mode = grp->is_bin_visible(kobj, *bin_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;
Strictly speaking, the mode validation for binary attributes is new and logically
separate. Should it be mentioned in the commit log, or even be a separate patch ?
> error = sysfs_add_file_mode_ns(parent,
> &(*bin_attr)->attr, true,
> - (*bin_attr)->attr.mode, NULL);
> + mode, NULL);
> if (error)
> break;
> }
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 9f65758..2f66050 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -64,10 +64,18 @@ do { \
> * a new subdirectory with this name.
> * @is_visible: Optional: Function to return permissions associated with an
> * attribute of the group. Will be called repeatedly for each
> - * attribute in the group. Only read/write permissions as well as
> - * SYSFS_PREALLOC are accepted. Must return 0 if an attribute is
> - * not visible. The returned value will replace static permissions
> - * defined in struct attribute or struct bin_attribute.
> + * non-binary attribute in the group. Only read/write
> + * permissions as well as SYSFS_PREALLOC are accepted. Must
> + * return 0 if an attribute is not visible. The returned value
> + * will replace static permissions defined in struct attribute.
> + * @is_bin_visible:
> + * Optional: Function to return permissions associated with a
> + * binary attribute of the group. Will be called repeatedly
> + * for each binary attribute in the group. Only read/write
> + * permissions as well as SYSFS_PREALLOC are accepted. Must
> + * return 0 if a binary attribute is not visible. The returned
> + * value will replace static permissions defined in
> + * struct bin_attribute.
> * @attrs: Pointer to NULL terminated list of attributes.
> * @bin_attrs: Pointer to NULL terminated list of binary attributes.
> * Either attrs or bin_attrs or both must be provided.
> @@ -76,6 +84,8 @@ struct attribute_group {
> const char *name;
> umode_t (*is_visible)(struct kobject *,
> struct attribute *, int);
> + umode_t (*is_bin_visible)(struct kobject *,
> + struct bin_attribute *, int);
> struct attribute **attrs;
> struct bin_attribute **bin_attrs;
> };
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/3] sysfs: Support is_visible() on binary attributes
@ 2015-09-14 15:33 ` Guenter Roeck
0 siblings, 0 replies; 28+ messages in thread
From: Guenter Roeck @ 2015-09-14 15:33 UTC (permalink / raw)
To: linux-arm-kernel
On 09/14/2015 05:34 AM, 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 introduces a new is_bin_visible() function to implement
> the same functionality for binary attributes, and updates documentation
> accordingly.
>
> Signed-off-by: Emilio L?pez <emilio.lopez@collabora.co.uk>
Nitpick below, but otherwise looks ok to me.
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Guenter
> ---
>
> Changes from v1:
> - Don't overload is_visible, introduce is_bin_visible instead as
> discussed on the list.
>
> fs/sysfs/group.c | 17 +++++++++++++++--
> include/linux/sysfs.h | 18 ++++++++++++++----
> 2 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 39a0199..51b56e6 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -73,13 +73,26 @@ 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 (i = 0, 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_bin_visible) {
> + mode = grp->is_bin_visible(kobj, *bin_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;
Strictly speaking, the mode validation for binary attributes is new and logically
separate. Should it be mentioned in the commit log, or even be a separate patch ?
> error = sysfs_add_file_mode_ns(parent,
> &(*bin_attr)->attr, true,
> - (*bin_attr)->attr.mode, NULL);
> + mode, NULL);
> if (error)
> break;
> }
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 9f65758..2f66050 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -64,10 +64,18 @@ do { \
> * a new subdirectory with this name.
> * @is_visible: Optional: Function to return permissions associated with an
> * attribute of the group. Will be called repeatedly for each
> - * attribute in the group. Only read/write permissions as well as
> - * SYSFS_PREALLOC are accepted. Must return 0 if an attribute is
> - * not visible. The returned value will replace static permissions
> - * defined in struct attribute or struct bin_attribute.
> + * non-binary attribute in the group. Only read/write
> + * permissions as well as SYSFS_PREALLOC are accepted. Must
> + * return 0 if an attribute is not visible. The returned value
> + * will replace static permissions defined in struct attribute.
> + * @is_bin_visible:
> + * Optional: Function to return permissions associated with a
> + * binary attribute of the group. Will be called repeatedly
> + * for each binary attribute in the group. Only read/write
> + * permissions as well as SYSFS_PREALLOC are accepted. Must
> + * return 0 if a binary attribute is not visible. The returned
> + * value will replace static permissions defined in
> + * struct bin_attribute.
> * @attrs: Pointer to NULL terminated list of attributes.
> * @bin_attrs: Pointer to NULL terminated list of binary attributes.
> * Either attrs or bin_attrs or both must be provided.
> @@ -76,6 +84,8 @@ struct attribute_group {
> const char *name;
> umode_t (*is_visible)(struct kobject *,
> struct attribute *, int);
> + umode_t (*is_bin_visible)(struct kobject *,
> + struct bin_attribute *, int);
> struct attribute **attrs;
> struct bin_attribute **bin_attrs;
> };
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: Enable EC vboot context support on Peach boards
2015-09-14 12:34 ` Emilio López
@ 2015-09-15 0:00 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 28+ messages in thread
From: Krzysztof Kozlowski @ 2015-09-15 0:00 UTC (permalink / raw)
To: Emilio López, gregkh, olof, kgene, linux
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-samsung-soc
On 14.09.2015 21:34, Emilio López wrote:
> 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>
> ---
>
> Changes from v1:
> - none
>
> arch/arm/boot/dts/exynos5420-peach-pit.dts | 1 +
> arch/arm/boot/dts/exynos5800-peach-pi.dts | 1 +
> 2 files changed, 2 insertions(+)
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 3/3] ARM: dts: Enable EC vboot context support on Peach boards
@ 2015-09-15 0:00 ` Krzysztof Kozlowski
0 siblings, 0 replies; 28+ messages in thread
From: Krzysztof Kozlowski @ 2015-09-15 0:00 UTC (permalink / raw)
To: linux-arm-kernel
On 14.09.2015 21:34, Emilio L?pez wrote:
> 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>
> ---
>
> Changes from v1:
> - none
>
> arch/arm/boot/dts/exynos5420-peach-pit.dts | 1 +
> arch/arm/boot/dts/exynos5800-peach-pi.dts | 1 +
> 2 files changed, 2 insertions(+)
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
2015-09-14 12:34 ` Emilio López
@ 2015-09-15 13:47 ` Javier Martinez Canillas
-1 siblings, 0 replies; 28+ messages in thread
From: Javier Martinez Canillas @ 2015-09-15 13:47 UTC (permalink / raw)
To: Emilio López
Cc: Greg Kroah-Hartman, Olof Johansson, Kukjin Kim,
Krzysztof Kozłowski, Guenter Roeck, Linux Kernel,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
Hello Emilio,
Patch looks mostly good to me, I just have a few comments.
On Mon, Sep 14, 2015 at 2:34 PM, Emilio López
<emilio.lopez@collabora.co.uk> wrote:
> 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>
> ---
> Changes from v1:
> - Use is_bin_visible instead of is_visible
>
> 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:
>
I would split the DT binding part from the actual implementation, see
Documentation/devicetree/bindings/submitting-patches.txt.
> 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
Why are you changing the Makefile? AFAIK += is usually used when the
compilation is conditional based on a Kconfig symbol but since these
are build unconditionally, I'll just keep it as foo := bar baz
Which makes me think, do we need a Kconfig option for this feature
since not all machines have it?
> 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..a0e8d38
> --- /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);
Shouldn't this be para_sz ? Since you are sending to the EC the whole
struct ec_params_vbnvcontext and not only the op field.
Or if the EC only expects to get the u32 op field, then I think your
max payload calculation is not correct.
> + 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);
Why you need this? struct ec_response_vbnvcontext is really small AFAICT.
> + 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 bin_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->attr.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_bin_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
Best regards,
Javier
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
@ 2015-09-15 13:47 ` Javier Martinez Canillas
0 siblings, 0 replies; 28+ messages in thread
From: Javier Martinez Canillas @ 2015-09-15 13:47 UTC (permalink / raw)
To: linux-arm-kernel
Hello Emilio,
Patch looks mostly good to me, I just have a few comments.
On Mon, Sep 14, 2015 at 2:34 PM, Emilio L?pez
<emilio.lopez@collabora.co.uk> wrote:
> 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>
> ---
> Changes from v1:
> - Use is_bin_visible instead of is_visible
>
> 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:
>
I would split the DT binding part from the actual implementation, see
Documentation/devicetree/bindings/submitting-patches.txt.
> 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
Why are you changing the Makefile? AFAIK += is usually used when the
compilation is conditional based on a Kconfig symbol but since these
are build unconditionally, I'll just keep it as foo := bar baz
Which makes me think, do we need a Kconfig option for this feature
since not all machines have it?
> 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..a0e8d38
> --- /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);
Shouldn't this be para_sz ? Since you are sending to the EC the whole
struct ec_params_vbnvcontext and not only the op field.
Or if the EC only expects to get the u32 op field, then I think your
max payload calculation is not correct.
> + 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);
Why you need this? struct ec_response_vbnvcontext is really small AFAICT.
> + 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 bin_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->attr.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_bin_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
Best regards,
Javier
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/3] ARM: dts: Enable EC vboot context support on Peach boards
2015-09-14 12:34 ` Emilio López
@ 2015-09-15 13:49 ` Javier Martinez Canillas
-1 siblings, 0 replies; 28+ messages in thread
From: Javier Martinez Canillas @ 2015-09-15 13:49 UTC (permalink / raw)
To: Emilio López
Cc: Greg Kroah-Hartman, Olof Johansson, Kukjin Kim,
Krzysztof Kozłowski, Guenter Roeck, Linux Kernel,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
Hello Emilio,
On Mon, Sep 14, 2015 at 2:34 PM, Emilio López
<emilio.lopez@collabora.co.uk> wrote:
> 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>
> ---
Acked-by: Javier Martinez Canillas <javier@osg.samsung.com>
Best regards,
Javier
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 3/3] ARM: dts: Enable EC vboot context support on Peach boards
@ 2015-09-15 13:49 ` Javier Martinez Canillas
0 siblings, 0 replies; 28+ messages in thread
From: Javier Martinez Canillas @ 2015-09-15 13:49 UTC (permalink / raw)
To: linux-arm-kernel
Hello Emilio,
On Mon, Sep 14, 2015 at 2:34 PM, Emilio L?pez
<emilio.lopez@collabora.co.uk> wrote:
> 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>
> ---
Acked-by: Javier Martinez Canillas <javier@osg.samsung.com>
Best regards,
Javier
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
2015-09-15 13:47 ` Javier Martinez Canillas
@ 2015-09-15 19:16 ` Emilio López
-1 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-15 19:16 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Greg Kroah-Hartman, Olof Johansson, Kukjin Kim,
Krzysztof Kozłowski, Guenter Roeck, Linux Kernel,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
Hi Javier,
Thanks for the review. You'll find my answers inline
On 15/09/15 10:47, Javier Martinez Canillas wrote:
> Hello Emilio,
>
> Patch looks mostly good to me, I just have a few comments.
>
> On Mon, Sep 14, 2015 at 2:34 PM, Emilio López
> <emilio.lopez@collabora.co.uk> wrote:
>> 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>
>> ---
>> Changes from v1:
>> - Use is_bin_visible instead of is_visible
>>
>> 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:
>>
>
> I would split the DT binding part from the actual implementation, see
> Documentation/devicetree/bindings/submitting-patches.txt.
Ok, I'll do
>> 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
>
> Why are you changing the Makefile? AFAIK += is usually used when the
> compilation is conditional based on a Kconfig symbol but since these
> are build unconditionally, I'll just keep it as foo := bar baz
As far as I'm aware, += is append[0]. It's used for stuff like
obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
because the left part will resolve to "obj-y" or similar, and you want
to add to it, not replace it. I only changed the Makefile here because
the line was growing too long, and I thought it looked neater this way;
it shouldn't cause any functional change apart from the intended one.
[0] https://www.gnu.org/software/make/manual/html_node/Appending.html
> Which makes me think, do we need a Kconfig option for this feature
> since not all machines have it?
Not all machines have a lightbar either; both of the features are
runtime-conditional. IMO, it makes more sense this way when you consider
a kernel will likely support multiple platforms.
>> 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..a0e8d38
>> --- /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);
>
> Shouldn't this be para_sz ? Since you are sending to the EC the whole
> struct ec_params_vbnvcontext and not only the op field.
>
> Or if the EC only expects to get the u32 op field, then I think your
> max payload calculation is not correct.
The params struct is the same for both read and write ops, so it has the
op flag and a buffer for the write op. During the read op I believe
there's no need to send this potentially-garbage-filled buffer to the
EC, so outsize is set accordingly here.
The vendor tree does it this way, but I can change it to send the full
buffer if you prefer so.
ec code is at
https://chromium.googlesource.com/chromiumos/platform/ec/+/master/common/system.c
L1086+ if you want to take a look
>> + 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);
>
> Why you need this? struct ec_response_vbnvcontext is really small AFAICT.
This is just me being paranoid about memcpy :) Indeed, the struct should
be way smaller than a page. I can drop it if you think it's too much.
>> + memcpy(buf, msg->data, resp_sz);
>> +
>> + kfree(msg);
>> + return resp_sz;
>> +}
>> +
Thanks!
Emilio
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
@ 2015-09-15 19:16 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-15 19:16 UTC (permalink / raw)
To: linux-arm-kernel
Hi Javier,
Thanks for the review. You'll find my answers inline
On 15/09/15 10:47, Javier Martinez Canillas wrote:
> Hello Emilio,
>
> Patch looks mostly good to me, I just have a few comments.
>
> On Mon, Sep 14, 2015 at 2:34 PM, Emilio L?pez
> <emilio.lopez@collabora.co.uk> wrote:
>> 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>
>> ---
>> Changes from v1:
>> - Use is_bin_visible instead of is_visible
>>
>> 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:
>>
>
> I would split the DT binding part from the actual implementation, see
> Documentation/devicetree/bindings/submitting-patches.txt.
Ok, I'll do
>> 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
>
> Why are you changing the Makefile? AFAIK += is usually used when the
> compilation is conditional based on a Kconfig symbol but since these
> are build unconditionally, I'll just keep it as foo := bar baz
As far as I'm aware, += is append[0]. It's used for stuff like
obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
because the left part will resolve to "obj-y" or similar, and you want
to add to it, not replace it. I only changed the Makefile here because
the line was growing too long, and I thought it looked neater this way;
it shouldn't cause any functional change apart from the intended one.
[0] https://www.gnu.org/software/make/manual/html_node/Appending.html
> Which makes me think, do we need a Kconfig option for this feature
> since not all machines have it?
Not all machines have a lightbar either; both of the features are
runtime-conditional. IMO, it makes more sense this way when you consider
a kernel will likely support multiple platforms.
>> 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..a0e8d38
>> --- /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);
>
> Shouldn't this be para_sz ? Since you are sending to the EC the whole
> struct ec_params_vbnvcontext and not only the op field.
>
> Or if the EC only expects to get the u32 op field, then I think your
> max payload calculation is not correct.
The params struct is the same for both read and write ops, so it has the
op flag and a buffer for the write op. During the read op I believe
there's no need to send this potentially-garbage-filled buffer to the
EC, so outsize is set accordingly here.
The vendor tree does it this way, but I can change it to send the full
buffer if you prefer so.
ec code is at
https://chromium.googlesource.com/chromiumos/platform/ec/+/master/common/system.c
L1086+ if you want to take a look
>> + 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);
>
> Why you need this? struct ec_response_vbnvcontext is really small AFAICT.
This is just me being paranoid about memcpy :) Indeed, the struct should
be way smaller than a page. I can drop it if you think it's too much.
>> + memcpy(buf, msg->data, resp_sz);
>> +
>> + kfree(msg);
>> + return resp_sz;
>> +}
>> +
Thanks!
Emilio
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
2015-09-15 19:16 ` Emilio López
@ 2015-09-15 19:43 ` Javier Martinez Canillas
-1 siblings, 0 replies; 28+ messages in thread
From: Javier Martinez Canillas @ 2015-09-15 19:43 UTC (permalink / raw)
To: Emilio López
Cc: Greg Kroah-Hartman, Olof Johansson, Kukjin Kim,
Krzysztof Kozłowski, Guenter Roeck, Linux Kernel,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
Hello Emilio,
On Tue, Sep 15, 2015 at 9:16 PM, Emilio López
<emilio.lopez@collabora.co.uk> wrote:
[snip]
>>>
>>> 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
>>
>>
>> Why are you changing the Makefile? AFAIK += is usually used when the
>> compilation is conditional based on a Kconfig symbol but since these
>> are build unconditionally, I'll just keep it as foo := bar baz
>
>
> As far as I'm aware, += is append[0]. It's used for stuff like
> obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
> because the left part will resolve to "obj-y" or similar, and you want to
> add to it, not replace it. I only changed the Makefile here because the line
> was growing too long, and I thought it looked neater this way; it shouldn't
> cause any functional change apart from the intended one.
>
> [0] https://www.gnu.org/software/make/manual/html_node/Appending.html
>
Yes, I know how Kbuild works. What I tried to say is that you usually
append based on a Kconfig symbol. In fact even you are mentioning such
an example.
So appending unconditionally like you are doing makes the Makefile
harder to read IMHO. If the line grows to long you can use a backlash
(\) char to split the line.
>> Which makes me think, do we need a Kconfig option for this feature
>> since not all machines have it?
>
>
> Not all machines have a lightbar either; both of the features are
> runtime-conditional. IMO, it makes more sense this way when you consider a
> kernel will likely support multiple platforms.
>
Ok, thanks for the clarification.
>
>>> 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..a0e8d38
>>> --- /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);
>>
>>
>> Shouldn't this be para_sz ? Since you are sending to the EC the whole
>> struct ec_params_vbnvcontext and not only the op field.
>>
>> Or if the EC only expects to get the u32 op field, then I think your
>> max payload calculation is not correct.
>
>
> The params struct is the same for both read and write ops, so it has the op
That's not true, struct ec_response_vbnvcontext has only the block
field while struct ec_param_vbnvcontext has both the op and block
fields.
> flag and a buffer for the write op. During the read op I believe there's no
> need to send this potentially-garbage-filled buffer to the EC, so outsize is
> set accordingly here.
Yes, I agree with you but then as I mentioned I think your payload
calculation is wrong since you want instead max(sizeof(struct
ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
allocating 4 bytes more than needed.
>
> The vendor tree does it this way, but I can change it to send the full
> buffer if you prefer so.
>
> ec code is at
> https://chromium.googlesource.com/chromiumos/platform/ec/+/master/common/system.c
> L1086+ if you want to take a look
>
>>> + 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);
>>
>>
>> Why you need this? struct ec_response_vbnvcontext is really small AFAICT.
>
>
> This is just me being paranoid about memcpy :) Indeed, the struct should be
> way smaller than a page. I can drop it if you think it's too much.
>
I think it can be dropped but it's up to you.
>>> + memcpy(buf, msg->data, resp_sz);
>>> +
>>> + kfree(msg);
>>> + return resp_sz;
>>> +}
>>> +
>
>
> Thanks!
> Emilio
with the needed changes, feel free to add my:
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Best regards,
Javier
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
@ 2015-09-15 19:43 ` Javier Martinez Canillas
0 siblings, 0 replies; 28+ messages in thread
From: Javier Martinez Canillas @ 2015-09-15 19:43 UTC (permalink / raw)
To: linux-arm-kernel
Hello Emilio,
On Tue, Sep 15, 2015 at 9:16 PM, Emilio L?pez
<emilio.lopez@collabora.co.uk> wrote:
[snip]
>>>
>>> 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
>>
>>
>> Why are you changing the Makefile? AFAIK += is usually used when the
>> compilation is conditional based on a Kconfig symbol but since these
>> are build unconditionally, I'll just keep it as foo := bar baz
>
>
> As far as I'm aware, += is append[0]. It's used for stuff like
> obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
> because the left part will resolve to "obj-y" or similar, and you want to
> add to it, not replace it. I only changed the Makefile here because the line
> was growing too long, and I thought it looked neater this way; it shouldn't
> cause any functional change apart from the intended one.
>
> [0] https://www.gnu.org/software/make/manual/html_node/Appending.html
>
Yes, I know how Kbuild works. What I tried to say is that you usually
append based on a Kconfig symbol. In fact even you are mentioning such
an example.
So appending unconditionally like you are doing makes the Makefile
harder to read IMHO. If the line grows to long you can use a backlash
(\) char to split the line.
>> Which makes me think, do we need a Kconfig option for this feature
>> since not all machines have it?
>
>
> Not all machines have a lightbar either; both of the features are
> runtime-conditional. IMO, it makes more sense this way when you consider a
> kernel will likely support multiple platforms.
>
Ok, thanks for the clarification.
>
>>> 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..a0e8d38
>>> --- /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);
>>
>>
>> Shouldn't this be para_sz ? Since you are sending to the EC the whole
>> struct ec_params_vbnvcontext and not only the op field.
>>
>> Or if the EC only expects to get the u32 op field, then I think your
>> max payload calculation is not correct.
>
>
> The params struct is the same for both read and write ops, so it has the op
That's not true, struct ec_response_vbnvcontext has only the block
field while struct ec_param_vbnvcontext has both the op and block
fields.
> flag and a buffer for the write op. During the read op I believe there's no
> need to send this potentially-garbage-filled buffer to the EC, so outsize is
> set accordingly here.
Yes, I agree with you but then as I mentioned I think your payload
calculation is wrong since you want instead max(sizeof(struct
ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
allocating 4 bytes more than needed.
>
> The vendor tree does it this way, but I can change it to send the full
> buffer if you prefer so.
>
> ec code is at
> https://chromium.googlesource.com/chromiumos/platform/ec/+/master/common/system.c
> L1086+ if you want to take a look
>
>>> + 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);
>>
>>
>> Why you need this? struct ec_response_vbnvcontext is really small AFAICT.
>
>
> This is just me being paranoid about memcpy :) Indeed, the struct should be
> way smaller than a page. I can drop it if you think it's too much.
>
I think it can be dropped but it's up to you.
>>> + memcpy(buf, msg->data, resp_sz);
>>> +
>>> + kfree(msg);
>>> + return resp_sz;
>>> +}
>>> +
>
>
> Thanks!
> Emilio
with the needed changes, feel free to add my:
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Best regards,
Javier
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
2015-09-15 19:43 ` Javier Martinez Canillas
@ 2015-09-15 20:22 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 28+ messages in thread
From: Greg Kroah-Hartman @ 2015-09-15 20:22 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Emilio López, Olof Johansson, Kukjin Kim,
Krzysztof Kozłowski, Guenter Roeck, Linux Kernel,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
On Tue, Sep 15, 2015 at 09:43:35PM +0200, Javier Martinez Canillas wrote:
> Hello Emilio,
>
> On Tue, Sep 15, 2015 at 9:16 PM, Emilio López
> <emilio.lopez@collabora.co.uk> wrote:
>
> [snip]
>
> >>>
> >>> 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
> >>
> >>
> >> Why are you changing the Makefile? AFAIK += is usually used when the
> >> compilation is conditional based on a Kconfig symbol but since these
> >> are build unconditionally, I'll just keep it as foo := bar baz
> >
> >
> > As far as I'm aware, += is append[0]. It's used for stuff like
> > obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
> > because the left part will resolve to "obj-y" or similar, and you want to
> > add to it, not replace it. I only changed the Makefile here because the line
> > was growing too long, and I thought it looked neater this way; it shouldn't
> > cause any functional change apart from the intended one.
> >
> > [0] https://www.gnu.org/software/make/manual/html_node/Appending.html
> >
>
> Yes, I know how Kbuild works. What I tried to say is that you usually
> append based on a Kconfig symbol. In fact even you are mentioning such
> an example.
> So appending unconditionally like you are doing makes the Makefile
> harder to read IMHO. If the line grows to long you can use a backlash
> (\) char to split the line.
Either format is just fine, don't get too picky here please.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
@ 2015-09-15 20:22 ` Greg Kroah-Hartman
0 siblings, 0 replies; 28+ messages in thread
From: Greg Kroah-Hartman @ 2015-09-15 20:22 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 15, 2015 at 09:43:35PM +0200, Javier Martinez Canillas wrote:
> Hello Emilio,
>
> On Tue, Sep 15, 2015 at 9:16 PM, Emilio L?pez
> <emilio.lopez@collabora.co.uk> wrote:
>
> [snip]
>
> >>>
> >>> 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
> >>
> >>
> >> Why are you changing the Makefile? AFAIK += is usually used when the
> >> compilation is conditional based on a Kconfig symbol but since these
> >> are build unconditionally, I'll just keep it as foo := bar baz
> >
> >
> > As far as I'm aware, += is append[0]. It's used for stuff like
> > obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
> > because the left part will resolve to "obj-y" or similar, and you want to
> > add to it, not replace it. I only changed the Makefile here because the line
> > was growing too long, and I thought it looked neater this way; it shouldn't
> > cause any functional change apart from the intended one.
> >
> > [0] https://www.gnu.org/software/make/manual/html_node/Appending.html
> >
>
> Yes, I know how Kbuild works. What I tried to say is that you usually
> append based on a Kconfig symbol. In fact even you are mentioning such
> an example.
> So appending unconditionally like you are doing makes the Makefile
> harder to read IMHO. If the line grows to long you can use a backlash
> (\) char to split the line.
Either format is just fine, don't get too picky here please.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
@ 2015-09-15 20:24 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-15 20:24 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Greg Kroah-Hartman, Olof Johansson, Kukjin Kim,
Krzysztof Kozłowski, Guenter Roeck, Linux Kernel,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
Hi Javier,
On 15/09/15 16:43, Javier Martinez Canillas wrote:
> Hello Emilio,
>
> On Tue, Sep 15, 2015 at 9:16 PM, Emilio López
> <emilio.lopez@collabora.co.uk> wrote:
>
> [snip]
>
>>>>
>>>> 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
>>>
>>>
>>> Why are you changing the Makefile? AFAIK += is usually used when the
>>> compilation is conditional based on a Kconfig symbol but since these
>>> are build unconditionally, I'll just keep it as foo := bar baz
>>
>>
>> As far as I'm aware, += is append[0]. It's used for stuff like
>> obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
>> because the left part will resolve to "obj-y" or similar, and you want to
>> add to it, not replace it. I only changed the Makefile here because the line
>> was growing too long, and I thought it looked neater this way; it shouldn't
>> cause any functional change apart from the intended one.
>>
>> [0] https://www.gnu.org/software/make/manual/html_node/Appending.html
>>
>
> Yes, I know how Kbuild works. What I tried to say is that you usually
> append based on a Kconfig symbol. In fact even you are mentioning such
> an example.
> So appending unconditionally like you are doing makes the Makefile
> harder to read IMHO. If the line grows to long you can use a backlash
> (\) char to split the line.
I guess it just boils down to personal preference; I don't feel that
strongly about it, I'll change it in v3
(...)
>>>> + 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);
>>>
>>>
>>> Shouldn't this be para_sz ? Since you are sending to the EC the whole
>>> struct ec_params_vbnvcontext and not only the op field.
>>>
>>> Or if the EC only expects to get the u32 op field, then I think your
>>> max payload calculation is not correct.
>>
>>
>> The params struct is the same for both read and write ops, so it has the op
>
> That's not true, struct ec_response_vbnvcontext has only the block
> field while struct ec_param_vbnvcontext has both the op and block
> fields.
The former is a response struct, not a params struct.
>> flag and a buffer for the write op. During the read op I believe there's no
>> need to send this potentially-garbage-filled buffer to the EC, so outsize is
>> set accordingly here.
>
> Yes, I agree with you but then as I mentioned I think your payload
> calculation is wrong since you want instead max(sizeof(struct
> ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
> allocating 4 bytes more than needed.
Yeah, I can see that. If I do that though, max(...) would be less than
the size of the full params struct, and casting data to it could lead to
subtle bugs in the future. I can change it and add a comment mentioning
this, deal?
(...)
> with the needed changes, feel free to add my:
>
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Ok, thanks!
Emilio
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
@ 2015-09-15 20:24 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-15 20:24 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Greg Kroah-Hartman, Olof Johansson, Kukjin Kim,
Krzysztof Kozłowski, Guenter Roeck, Linux Kernel,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hi Javier,
On 15/09/15 16:43, Javier Martinez Canillas wrote:
> Hello Emilio,
>
> On Tue, Sep 15, 2015 at 9:16 PM, Emilio López
> <emilio.lopez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org> wrote:
>
> [snip]
>
>>>>
>>>> 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
>>>
>>>
>>> Why are you changing the Makefile? AFAIK += is usually used when the
>>> compilation is conditional based on a Kconfig symbol but since these
>>> are build unconditionally, I'll just keep it as foo := bar baz
>>
>>
>> As far as I'm aware, += is append[0]. It's used for stuff like
>> obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
>> because the left part will resolve to "obj-y" or similar, and you want to
>> add to it, not replace it. I only changed the Makefile here because the line
>> was growing too long, and I thought it looked neater this way; it shouldn't
>> cause any functional change apart from the intended one.
>>
>> [0] https://www.gnu.org/software/make/manual/html_node/Appending.html
>>
>
> Yes, I know how Kbuild works. What I tried to say is that you usually
> append based on a Kconfig symbol. In fact even you are mentioning such
> an example.
> So appending unconditionally like you are doing makes the Makefile
> harder to read IMHO. If the line grows to long you can use a backlash
> (\) char to split the line.
I guess it just boils down to personal preference; I don't feel that
strongly about it, I'll change it in v3
(...)
>>>> + 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);
>>>
>>>
>>> Shouldn't this be para_sz ? Since you are sending to the EC the whole
>>> struct ec_params_vbnvcontext and not only the op field.
>>>
>>> Or if the EC only expects to get the u32 op field, then I think your
>>> max payload calculation is not correct.
>>
>>
>> The params struct is the same for both read and write ops, so it has the op
>
> That's not true, struct ec_response_vbnvcontext has only the block
> field while struct ec_param_vbnvcontext has both the op and block
> fields.
The former is a response struct, not a params struct.
>> flag and a buffer for the write op. During the read op I believe there's no
>> need to send this potentially-garbage-filled buffer to the EC, so outsize is
>> set accordingly here.
>
> Yes, I agree with you but then as I mentioned I think your payload
> calculation is wrong since you want instead max(sizeof(struct
> ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
> allocating 4 bytes more than needed.
Yeah, I can see that. If I do that though, max(...) would be less than
the size of the full params struct, and casting data to it could lead to
subtle bugs in the future. I can change it and add a comment mentioning
this, deal?
(...)
> with the needed changes, feel free to add my:
>
> Reviewed-by: Javier Martinez Canillas <javier-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
Ok, thanks!
Emilio
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
@ 2015-09-15 20:24 ` Emilio López
0 siblings, 0 replies; 28+ messages in thread
From: Emilio López @ 2015-09-15 20:24 UTC (permalink / raw)
To: linux-arm-kernel
Hi Javier,
On 15/09/15 16:43, Javier Martinez Canillas wrote:
> Hello Emilio,
>
> On Tue, Sep 15, 2015 at 9:16 PM, Emilio L?pez
> <emilio.lopez@collabora.co.uk> wrote:
>
> [snip]
>
>>>>
>>>> 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
>>>
>>>
>>> Why are you changing the Makefile? AFAIK += is usually used when the
>>> compilation is conditional based on a Kconfig symbol but since these
>>> are build unconditionally, I'll just keep it as foo := bar baz
>>
>>
>> As far as I'm aware, += is append[0]. It's used for stuff like
>> obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
>> because the left part will resolve to "obj-y" or similar, and you want to
>> add to it, not replace it. I only changed the Makefile here because the line
>> was growing too long, and I thought it looked neater this way; it shouldn't
>> cause any functional change apart from the intended one.
>>
>> [0] https://www.gnu.org/software/make/manual/html_node/Appending.html
>>
>
> Yes, I know how Kbuild works. What I tried to say is that you usually
> append based on a Kconfig symbol. In fact even you are mentioning such
> an example.
> So appending unconditionally like you are doing makes the Makefile
> harder to read IMHO. If the line grows to long you can use a backlash
> (\) char to split the line.
I guess it just boils down to personal preference; I don't feel that
strongly about it, I'll change it in v3
(...)
>>>> + 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);
>>>
>>>
>>> Shouldn't this be para_sz ? Since you are sending to the EC the whole
>>> struct ec_params_vbnvcontext and not only the op field.
>>>
>>> Or if the EC only expects to get the u32 op field, then I think your
>>> max payload calculation is not correct.
>>
>>
>> The params struct is the same for both read and write ops, so it has the op
>
> That's not true, struct ec_response_vbnvcontext has only the block
> field while struct ec_param_vbnvcontext has both the op and block
> fields.
The former is a response struct, not a params struct.
>> flag and a buffer for the write op. During the read op I believe there's no
>> need to send this potentially-garbage-filled buffer to the EC, so outsize is
>> set accordingly here.
>
> Yes, I agree with you but then as I mentioned I think your payload
> calculation is wrong since you want instead max(sizeof(struct
> ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
> allocating 4 bytes more than needed.
Yeah, I can see that. If I do that though, max(...) would be less than
the size of the full params struct, and casting data to it could lead to
subtle bugs in the future. I can change it and add a comment mentioning
this, deal?
(...)
> with the needed changes, feel free to add my:
>
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Ok, thanks!
Emilio
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
2015-09-15 20:24 ` Emilio López
@ 2015-09-15 22:26 ` Javier Martinez Canillas
-1 siblings, 0 replies; 28+ messages in thread
From: Javier Martinez Canillas @ 2015-09-15 22:26 UTC (permalink / raw)
To: Emilio López
Cc: Greg Kroah-Hartman, Olof Johansson, Kukjin Kim,
Krzysztof Kozłowski, Guenter Roeck, Linux Kernel,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
Hello Emilio,
On Tue, Sep 15, 2015 at 10:24 PM, Emilio López
<emilio.lopez@collabora.co.uk> wrote:
[snip]
>>>>> +
>>>>> + 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);
>>>>
>>>>
>>>>
>>>> Shouldn't this be para_sz ? Since you are sending to the EC the whole
>>>> struct ec_params_vbnvcontext and not only the op field.
>>>>
>>>> Or if the EC only expects to get the u32 op field, then I think your
>>>> max payload calculation is not correct.
>>>
>>>
>>>
>>> The params struct is the same for both read and write ops, so it has the
>>> op
>>
>>
>> That's not true, struct ec_response_vbnvcontext has only the block
>> field while struct ec_param_vbnvcontext has both the op and block
>> fields.
>
>
> The former is a response struct, not a params struct.
>
I misread read/write as request/response in the previous email, sorry
about that.
>>> flag and a buffer for the write op. During the read op I believe there's
>>> no
>>> need to send this potentially-garbage-filled buffer to the EC, so outsize
>>> is
>>> set accordingly here.
>>
>>
>> Yes, I agree with you but then as I mentioned I think your payload
>> calculation is wrong since you want instead max(sizeof(struct
>> ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
>> allocating 4 bytes more than needed.
>
>
> Yeah, I can see that. If I do that though, max(...) would be less than the
> size of the full params struct, and casting data to it could lead to subtle
> bugs in the future. I can change it and add a comment mentioning this, deal?
>
But by setting outsize to sizeof(params->op) you are allocating less
than the params struct anyways in the transport driver. Take a look
for example to cros_ec_cmd_xfer_i2c():
http://lxr.free-electrons.com/source/drivers/mfd/cros_ec_i2c.c#L187
But I don't have a strong opinion on this tbh, I was just pointing out
that it's strange that max(insize,outsize) does not match
msg->{insize,outsize}.
> (...)
>
>> with the needed changes, feel free to add my:
>>
>> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
>
>
> Ok, thanks!
>
> Emilio
Best regards,
Javier
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context
@ 2015-09-15 22:26 ` Javier Martinez Canillas
0 siblings, 0 replies; 28+ messages in thread
From: Javier Martinez Canillas @ 2015-09-15 22:26 UTC (permalink / raw)
To: linux-arm-kernel
Hello Emilio,
On Tue, Sep 15, 2015 at 10:24 PM, Emilio L?pez
<emilio.lopez@collabora.co.uk> wrote:
[snip]
>>>>> +
>>>>> + 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);
>>>>
>>>>
>>>>
>>>> Shouldn't this be para_sz ? Since you are sending to the EC the whole
>>>> struct ec_params_vbnvcontext and not only the op field.
>>>>
>>>> Or if the EC only expects to get the u32 op field, then I think your
>>>> max payload calculation is not correct.
>>>
>>>
>>>
>>> The params struct is the same for both read and write ops, so it has the
>>> op
>>
>>
>> That's not true, struct ec_response_vbnvcontext has only the block
>> field while struct ec_param_vbnvcontext has both the op and block
>> fields.
>
>
> The former is a response struct, not a params struct.
>
I misread read/write as request/response in the previous email, sorry
about that.
>>> flag and a buffer for the write op. During the read op I believe there's
>>> no
>>> need to send this potentially-garbage-filled buffer to the EC, so outsize
>>> is
>>> set accordingly here.
>>
>>
>> Yes, I agree with you but then as I mentioned I think your payload
>> calculation is wrong since you want instead max(sizeof(struct
>> ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
>> allocating 4 bytes more than needed.
>
>
> Yeah, I can see that. If I do that though, max(...) would be less than the
> size of the full params struct, and casting data to it could lead to subtle
> bugs in the future. I can change it and add a comment mentioning this, deal?
>
But by setting outsize to sizeof(params->op) you are allocating less
than the params struct anyways in the transport driver. Take a look
for example to cros_ec_cmd_xfer_i2c():
http://lxr.free-electrons.com/source/drivers/mfd/cros_ec_i2c.c#L187
But I don't have a strong opinion on this tbh, I was just pointing out
that it's strange that max(insize,outsize) does not match
msg->{insize,outsize}.
> (...)
>
>> with the needed changes, feel free to add my:
>>
>> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
>
>
> Ok, thanks!
>
> Emilio
Best regards,
Javier
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2015-09-15 22:26 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-14 12:34 [PATCH v2 0/3] platform/chrome: vboot context support Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-14 12:34 ` [PATCH v2 1/3] sysfs: Support is_visible() on binary attributes Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-14 15:33 ` Guenter Roeck
2015-09-14 15:33 ` Guenter Roeck
2015-09-14 12:34 ` [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-15 13:47 ` Javier Martinez Canillas
2015-09-15 13:47 ` Javier Martinez Canillas
2015-09-15 19:16 ` Emilio López
2015-09-15 19:16 ` Emilio López
2015-09-15 19:43 ` Javier Martinez Canillas
2015-09-15 19:43 ` Javier Martinez Canillas
2015-09-15 20:22 ` Greg Kroah-Hartman
2015-09-15 20:22 ` Greg Kroah-Hartman
2015-09-15 20:24 ` Emilio López
2015-09-15 20:24 ` Emilio López
2015-09-15 20:24 ` Emilio López
2015-09-15 22:26 ` Javier Martinez Canillas
2015-09-15 22:26 ` Javier Martinez Canillas
2015-09-14 12:34 ` [PATCH v2 3/3] ARM: dts: Enable EC vboot context support on Peach boards Emilio López
2015-09-14 12:34 ` Emilio López
2015-09-15 0:00 ` Krzysztof Kozlowski
2015-09-15 0:00 ` Krzysztof Kozlowski
2015-09-15 13:49 ` Javier Martinez Canillas
2015-09-15 13:49 ` Javier Martinez Canillas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.