[PATCH 00/13] eeepc-laptop cleanups

[PATCH 00/13] eeepc-laptop cleanups

[Frans Klaver]

Here's the first version of a patch series following discussion [1]. The series
applies on top of "eeepc-laptop: simplify parse_arg()"[2]. This also includes
some cleanups that weren't in the original discussion, but should probably be
taken into account.

[1] https://lkml.org/lkml/2014/9/3/865
[2] https://lkml.org/lkml/2014/9/10/742

For those interested, a proper branch is available at:
	https://github.com/fransklaver/linux wip/eeepc_cleanup_v1

Frans Klaver (13):
  eeepc-laptop: coding style: fix indentation
  eeepc-laptop: coding style: add curly braces around else compound
  eeepc-laptop: coding style: add curly braces around else compound
  eeepc-laptop: use symbolic permissions in device attributes
  eeepc-laptop: use DEVICE_ATTR to instantiate device_attributes
  eeepc-laptop: pull out ACPI_STORE_FUNC and ACPI_SHOW_FUNC macros
  eeepc-laptop: make disp attribute really write-only
  eeepc-laptop: pull out SENSOR_STORE_FUNC and SENSOR_SHOW_FUNC macros
  eeepc-laptop: make fan1_input really read-only
  eeepc-laptop: compare proper return values in get_cpufv
  eeepc-laptop: propagate errors from get_cpufv
  eeepc-laptop: store_cpufv: return error if set_acpi fails
  eeepc-laptop: return -ENXIO if acpi getter or setter fails

 drivers/platform/x86/eeepc-laptop.c | 118 +++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 56 deletions(-)

-- 
2.1.0

[PATCH 01/13] eeepc-laptop: coding style: fix indentation

[Frans Klaver]

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 90be993..982778f 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -544,7 +544,7 @@ static int eeepc_led_init(struct eeepc_laptop *eeepc)
 	eeepc->tpd_led.name = "eeepc::touchpad";
 	eeepc->tpd_led.brightness_set = tpd_led_set;
 	if (get_acpi(eeepc, CM_ASL_TPD) >= 0) /* if method is available */
-	  eeepc->tpd_led.brightness_get = tpd_led_get;
+		eeepc->tpd_led.brightness_get = tpd_led_get;
 	eeepc->tpd_led.max_brightness = 1;
 
 	rv = led_classdev_register(&eeepc->platform_device->dev,
-- 
2.1.0

[PATCH 02/13] eeepc-laptop: coding style: add curly braces around else compound

[Frans Klaver]

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 982778f..702fcbb 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -692,8 +692,9 @@ static int eeepc_register_rfkill_notifier(struct eeepc_laptop *eeepc,
 		 * changed during setup.
 		 */
 		eeepc_rfkill_hotplug(eeepc, handle);
-	} else
+	} else {
 		return -ENODEV;
+	}
 
 	return 0;
 }
-- 
2.1.0

[PATCH 03/13] eeepc-laptop: coding style: add curly braces around else compound

[Frans Klaver]

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 702fcbb..9d8dff9 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1425,8 +1425,9 @@ static int eeepc_acpi_add(struct acpi_device *device)
 		result = eeepc_backlight_init(eeepc);
 		if (result)
 			goto fail_backlight;
-	} else
+	} else {
 		pr_info("Backlight controlled by ACPI video driver\n");
+	}
 
 	result = eeepc_input_init(eeepc);
 	if (result)
-- 
2.1.0

[PATCH 04/13] eeepc-laptop: use symbolic permissions in device attributes

[Frans Klaver]

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 9d8dff9..f35d008 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -316,9 +316,9 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 		.store  = store_##_name,				\
 	}
 
-EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
-EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
-EEEPC_CREATE_DEVICE_ATTR(disp, 0200, CM_ASL_DISPLAYSWITCH);
+EEEPC_CREATE_DEVICE_ATTR(camera, S_IWUSR | S_IRUGO, CM_ASL_CAMERA);
+EEEPC_CREATE_DEVICE_ATTR(cardr, S_IWUSR | S_IRUGO, CM_ASL_CARDREADER);
+EEEPC_CREATE_DEVICE_ATTR(disp, S_IWUSR, CM_ASL_DISPLAYSWITCH);
 
 struct eeepc_cpufv {
 	int num;
@@ -423,7 +423,7 @@ static ssize_t store_cpufv_disabled(struct device *dev,
 static struct device_attribute dev_attr_cpufv = {
 	.attr = {
 		.name = "cpufv",
-		.mode = 0644 },
+		.mode = S_IWUSR | S_IRUGO },
 	.show   = show_cpufv,
 	.store  = store_cpufv
 };
@@ -431,14 +431,14 @@ static struct device_attribute dev_attr_cpufv = {
 static struct device_attribute dev_attr_available_cpufv = {
 	.attr = {
 		.name = "available_cpufv",
-		.mode = 0444 },
+		.mode = S_IRUGO },
 	.show   = show_available_cpufv
 };
 
 static struct device_attribute dev_attr_cpufv_disabled = {
 	.attr = {
 		.name = "cpufv_disabled",
-		.mode = 0644 },
+		.mode = S_IWUSR | S_IRUGO},
 	.show   = show_cpufv_disabled,
 	.store  = store_cpufv_disabled
 };
-- 
2.1.0

[PATCH 05/13] eeepc-laptop: use DEVICE_ATTR to instantiate device_attributes

[Frans Klaver]

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 35 +++++------------------------------
 1 file changed, 5 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index f35d008..8225b1e 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -308,13 +308,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 	{								\
 		return store_sys_acpi(dev, _cm, buf, count);		\
 	}								\
-	static struct device_attribute dev_attr_##_name = {		\
-		.attr = {						\
-			.name = __stringify(_name),			\
-			.mode = _mode },				\
-		.show   = show_##_name,					\
-		.store  = store_##_name,				\
-	}
+	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
 
 EEEPC_CREATE_DEVICE_ATTR(camera, S_IWUSR | S_IRUGO, CM_ASL_CAMERA);
 EEEPC_CREATE_DEVICE_ATTR(cardr, S_IWUSR | S_IRUGO, CM_ASL_CARDREADER);
@@ -420,29 +414,10 @@ static ssize_t store_cpufv_disabled(struct device *dev,
 }
 
 
-static struct device_attribute dev_attr_cpufv = {
-	.attr = {
-		.name = "cpufv",
-		.mode = S_IWUSR | S_IRUGO },
-	.show   = show_cpufv,
-	.store  = store_cpufv
-};
-
-static struct device_attribute dev_attr_available_cpufv = {
-	.attr = {
-		.name = "available_cpufv",
-		.mode = S_IRUGO },
-	.show   = show_available_cpufv
-};
-
-static struct device_attribute dev_attr_cpufv_disabled = {
-	.attr = {
-		.name = "cpufv_disabled",
-		.mode = S_IWUSR | S_IRUGO},
-	.show   = show_cpufv_disabled,
-	.store  = store_cpufv_disabled
-};
-
+static DEVICE_ATTR(cpufv, S_IWUSR | S_IRUGO, show_cpufv, store_cpufv);
+static DEVICE_ATTR(available_cpufv, S_IRUGO, show_available_cpufv, NULL);
+static DEVICE_ATTR(cpufv_disabled, S_IWUSR | S_IRUGO,
+		   show_cpufv_disabled, store_cpufv_disabled);
 
 static struct attribute *platform_attributes[] = {
 	&dev_attr_camera.attr,
-- 
2.1.0

[PATCH 06/13] eeepc-laptop: pull out ACPI_STORE_FUNC and ACPI_SHOW_FUNC macros

[Frans Klaver]

Pull out macros EEEPC_ACPI_STORE_FUNC and EEEPC_ACPI_SHOW_FUNC. These
macros define functions that call store_sys_acpi() and show_sys_acpi()
respectively. This helps prevent duplication later on.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 8225b1e..3c04b77 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -295,19 +295,25 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 	return sprintf(buf, "%d\n", value);
 }
 
-#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm)			\
+#define EEEPC_ACPI_SHOW_FUNC(_name, _cm)				\
 	static ssize_t show_##_name(struct device *dev,			\
 				    struct device_attribute *attr,	\
 				    char *buf)				\
 	{								\
 		return show_sys_acpi(dev, _cm, buf);			\
-	}								\
+	}
+
+#define EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
 	static ssize_t store_##_name(struct device *dev,		\
 				     struct device_attribute *attr,	\
 				     const char *buf, size_t count)	\
 	{								\
 		return store_sys_acpi(dev, _cm, buf, count);		\
-	}								\
+	}
+
+#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm)			\
+	EEEPC_ACPI_SHOW_FUNC(_name, _cm)				\
+	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
 	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
 
 EEEPC_CREATE_DEVICE_ATTR(camera, S_IWUSR | S_IRUGO, CM_ASL_CAMERA);
-- 
2.1.0

[PATCH 07/13] eeepc-laptop: make disp attribute really write-only

[Frans Klaver]

The disp attribute is write-only, but sysfs doesn't know this. Currently
show_sys_acpi() is mimicking sysfs behavior, if the underlying acpi call
should fail. This is not ideal; behaving like sysfs is better left to
sysfs.

Introduce EEEPC_CREATE_DEVICE_ATTR_WO() to instantiate a write-only
attribute, and declare the disp attribute with it. Sysfs makes sure
userspace can only write to disp at all times. This also means we can
back to propagating any errors from get_acpi().

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 3c04b77..4f6490d 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -291,7 +291,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 	int value = get_acpi(eeepc, cm);
 
 	if (value < 0)
-		return -EIO;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -316,9 +316,13 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
 	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
 	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
 
+#define EEEPC_CREATE_DEVICE_ATTR_WO(_name, _mode, _cm)			\
+	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
+	static DEVICE_ATTR(_name, _mode, NULL, store_##_name)
+
 EEEPC_CREATE_DEVICE_ATTR(camera, S_IWUSR | S_IRUGO, CM_ASL_CAMERA);
 EEEPC_CREATE_DEVICE_ATTR(cardr, S_IWUSR | S_IRUGO, CM_ASL_CARDREADER);
-EEEPC_CREATE_DEVICE_ATTR(disp, S_IWUSR, CM_ASL_DISPLAYSWITCH);
+EEEPC_CREATE_DEVICE_ATTR_WO(disp, S_IWUSR, CM_ASL_DISPLAYSWITCH);
 
 struct eeepc_cpufv {
 	int num;
-- 
2.1.0

[PATCH 08/13] eeepc-laptop: pull out SENSOR_STORE_FUNC and SENSOR_SHOW_FUNC macros

[Frans Klaver]

Pull out EEEPC_SENSOR_STORE_FUNC and EEEPC_SENSOR_SHOW_FUNC. These
macros define functions that call store_sys_hwmon() and show_sys_hwmon()
respectively. This helps prevent duplication later on.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 4f6490d..fbaa4d5 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1039,19 +1039,25 @@ static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
 	return sprintf(buf, "%d\n", get());
 }
 
-#define EEEPC_CREATE_SENSOR_ATTR(_name, _mode, _get, _set)		\
+#define EEEPC_SENSOR_SHOW_FUNC(_name, _get)				\
 	static ssize_t show_##_name(struct device *dev,			\
 				    struct device_attribute *attr,	\
 				    char *buf)				\
 	{								\
 		return show_sys_hwmon(_get, buf);			\
-	}								\
+	}
+
+#define EEEPC_SENSOR_STORE_FUNC(_name, _set)				\
 	static ssize_t store_##_name(struct device *dev,		\
 				     struct device_attribute *attr,	\
 				     const char *buf, size_t count)	\
 	{								\
 		return store_sys_hwmon(_set, buf, count);		\
-	}								\
+	}
+
+#define EEEPC_CREATE_SENSOR_ATTR(_name, _mode, _get, _set)		\
+	EEEPC_SENSOR_SHOW_FUNC(_name, _get)				\
+	EEEPC_SENSOR_STORE_FUNC(_name, _set)				\
 	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
 
 EEEPC_CREATE_SENSOR_ATTR(fan1_input, S_IRUGO, eeepc_get_fan_rpm, NULL);
-- 
2.1.0

[PATCH 09/13] eeepc-laptop: make fan1_input really read-only

[Frans Klaver]

In the instantiation of the fan1_input device attribute, NULL is passed
as set function to store_sys_hwmon. The function pointer is never
checked before dereferencing it. This is fine if we can guarantee that
it will never be called with an invalid pointer, but we can't. If
someone from user space decides to change the permissions on this
attribute and write to it, kernel will crash.

Introduce EEEPC_CREATE_SENSOR_ATTR_RO() to instantiate a read-only
attribute, and declare fan1_input with it. This ensures store_sys_hwmon
is never called with NULL parameters. If someone tries to write the
attribute, the system will at least keep its sanity.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index fbaa4d5..47488d3 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1060,7 +1060,11 @@ static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
 	EEEPC_SENSOR_STORE_FUNC(_name, _set)				\
 	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
 
-EEEPC_CREATE_SENSOR_ATTR(fan1_input, S_IRUGO, eeepc_get_fan_rpm, NULL);
+#define EEEPC_CREATE_SENSOR_ATTR_RO(_name, _mode, _get)			\
+	EEEPC_SENSOR_SHOW_FUNC(_name, _get)				\
+	static DEVICE_ATTR(_name, _mode, show_##_name, NULL)
+
+EEEPC_CREATE_SENSOR_ATTR_RO(fan1_input, S_IRUGO, eeepc_get_fan_rpm);
 EEEPC_CREATE_SENSOR_ATTR(pwm1, S_IRUGO | S_IWUSR,
 			 eeepc_get_fan_pwm, eeepc_set_fan_pwm);
 EEEPC_CREATE_SENSOR_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
-- 
2.1.0

[PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

In get_cpufv the return value of get_acpi is stored in the cpufv struct.
Right before this value is checked for errors, it is and'ed with 0xff.
This means c->cur can never be less than zero. Besides that, the actual
error value is ignored.

c->num is also and'ed with 0xff, which means we can ignore values below
zero.

Check the result of get_acpi() right away. While at it, propagate the
error if we got one.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 47488d3..828db56 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -332,9 +332,12 @@ struct eeepc_cpufv {
 static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
 {
 	c->cur = get_acpi(eeepc, CM_ASL_CPUFV);
+	if (c->cur < 0)
+		return c->cur;
+
 	c->num = (c->cur >> 8) & 0xff;
 	c->cur &= 0xff;
-	if (c->cur < 0 || c->num <= 0 || c->num > 12)
+	if (c->num == 0 || c->num > 12)
 		return -ENODEV;
 	return 0;
 }
-- 
2.1.0

[PATCH 11/13] eeepc-laptop: propagate errors from get_cpufv

[Frans Klaver]

In show_available_cpufv, show_cpufv, and store_cpufv, errors from
get_cpufv are mapped to -ENODEV. To paint a clear picture to callers,
propagate the errors instead.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 828db56..732b012 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -351,8 +351,9 @@ static ssize_t show_available_cpufv(struct device *dev,
 	int i;
 	ssize_t len = 0;
 
-	if (get_cpufv(eeepc, &c))
-		return -ENODEV;
+	i = get_cpufv(eeepc, &c);
+	if (i)
+		return i;
 	for (i = 0; i < c.num; i++)
 		len += sprintf(buf + len, "%d ", i);
 	len += sprintf(buf + len, "\n");
@@ -365,9 +366,11 @@ static ssize_t show_cpufv(struct device *dev,
 {
 	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
 	struct eeepc_cpufv c;
+	int rv;
 
-	if (get_cpufv(eeepc, &c))
-		return -ENODEV;
+	rv = get_cpufv(eeepc, &c);
+	if (rv)
+		return rv;
 	return sprintf(buf, "%#x\n", (c.num << 8) | c.cur);
 }
 
@@ -381,8 +384,9 @@ static ssize_t store_cpufv(struct device *dev,
 
 	if (eeepc->cpufv_disabled)
 		return -EPERM;
-	if (get_cpufv(eeepc, &c))
-		return -ENODEV;
+	rv = get_cpufv(eeepc, &c);
+	if (rv)
+		return rv;
 	rv = parse_arg(buf, &value);
 	if (rv < 0)
 		return rv;
-- 
2.1.0

[PATCH 12/13] eeepc-laptop: store_cpufv: return error if set_acpi fails

[Frans Klaver]

The result of set_acpi is left unchecked, but it may return errors. If
one occurs, send the error to the caller.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 drivers/platform/x86/eeepc-laptop.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 732b012..ad221e0 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -392,7 +392,9 @@ static ssize_t store_cpufv(struct device *dev,
 		return rv;
 	if (value < 0 || value >= c.num)
 		return -EINVAL;
-	set_acpi(eeepc, CM_ASL_CPUFV, value);
+	rv = set_acpi(eeepc, CM_ASL_CPUFV, value);
+	if (rv)
+		return rv;
 	return count;
 }
 
-- 
2.1.0

[PATCH 13/13] eeepc-laptop: return -ENXIO if acpi getter or setter fails

[Frans Klaver]

get_acpi() and set_acpi() return -ENODEV on error.  However, of the
available options, ENXIO seems more appropriate, since we're dealing
with both devices and addresses here:

	#define EIO              5      /* I/O error */
	#define ENXIO            6      /* No such device or address */
	#define ENODEV          19      /* No such device */

One could expect the same applies to acpi_setter_handle(), as this
follows a similar setup. Changing the return values here triggers the
error "Error removing rfkill notify handler" during boot, so we'll leave
that as is.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
Changing ENODEV to ENXIO in acpi_setter_handle() probably requires some extra
work to get it working properly. I'd propose to dive into that after we finish
this series, or we drop this patch from the series and move the whole return
value issue into a separate patch set. Either way, with the sysfs fixes in
place, I don't think this one is going to make a huge difference in the end.

 drivers/platform/x86/eeepc-laptop.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index ad221e0..064ab0d 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -215,9 +215,9 @@ static int set_acpi(struct eeepc_laptop *eeepc, int cm, int value)
 	const char *method = cm_setv[cm];
 
 	if (method == NULL)
-		return -ENODEV;
+		return -ENXIO;
 	if ((eeepc->cm_supported & (0x1 << cm)) == 0)
-		return -ENODEV;
+		return -ENXIO;
 
 	if (write_acpi_int(eeepc->handle, method, value))
 		pr_warn("Error writing %s\n", method);
@@ -230,9 +230,9 @@ static int get_acpi(struct eeepc_laptop *eeepc, int cm)
 	int value;
 
 	if (method == NULL)
-		return -ENODEV;
+		return -ENXIO;
 	if ((eeepc->cm_supported & (0x1 << cm)) == 0)
-		return -ENODEV;
+		return -ENXIO;
 
 	if (read_acpi_int(eeepc->handle, method, &value))
 		pr_warn("Error reading %s\n", method);
@@ -527,7 +527,7 @@ static int eeepc_led_init(struct eeepc_laptop *eeepc)
 {
 	int rv;
 
-	if (get_acpi(eeepc, CM_ASL_TPD) == -ENODEV)
+	if (get_acpi(eeepc, CM_ASL_TPD) == -ENXIO)
 		return 0;
 
 	eeepc->led_workqueue = create_singlethread_workqueue("led_workqueue");
-- 
2.1.0

Re: [PATCH 05/13] eeepc-laptop: use DEVICE_ATTR to instantiate device_attributes

[Greg Kroah-Hartman]

On Sat, Sep 13, 2014 at 01:06:44AM +0200, Frans Klaver wrote:
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 35 +++++------------------------------
>  1 file changed, 5 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index f35d008..8225b1e 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -308,13 +308,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
>  	{								\
>  		return store_sys_acpi(dev, _cm, buf, count);		\
>  	}								\
> -	static struct device_attribute dev_attr_##_name = {		\
> -		.attr = {						\
> -			.name = __stringify(_name),			\
> -			.mode = _mode },				\
> -		.show   = show_##_name,					\
> -		.store  = store_##_name,				\
> -	}
> +	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
>  
>  EEEPC_CREATE_DEVICE_ATTR(camera, S_IWUSR | S_IRUGO, CM_ASL_CAMERA);
>  EEEPC_CREATE_DEVICE_ATTR(cardr, S_IWUSR | S_IRUGO, CM_ASL_CARDREADER);
> @@ -420,29 +414,10 @@ static ssize_t store_cpufv_disabled(struct device *dev,
>  }
>  
>  
> -static struct device_attribute dev_attr_cpufv = {
> -	.attr = {
> -		.name = "cpufv",
> -		.mode = S_IWUSR | S_IRUGO },
> -	.show   = show_cpufv,
> -	.store  = store_cpufv
> -};
> -
> -static struct device_attribute dev_attr_available_cpufv = {
> -	.attr = {
> -		.name = "available_cpufv",
> -		.mode = S_IRUGO },
> -	.show   = show_available_cpufv
> -};
> -
> -static struct device_attribute dev_attr_cpufv_disabled = {
> -	.attr = {
> -		.name = "cpufv_disabled",
> -		.mode = S_IWUSR | S_IRUGO},
> -	.show   = show_cpufv_disabled,
> -	.store  = store_cpufv_disabled
> -};
> -
> +static DEVICE_ATTR(cpufv, S_IWUSR | S_IRUGO, show_cpufv, store_cpufv);

DEVICE_ATTR_RW()?

> +static DEVICE_ATTR(available_cpufv, S_IRUGO, show_available_cpufv, NULL);

DEVICE_ATTR_RO()?

> +static DEVICE_ATTR(cpufv_disabled, S_IWUSR | S_IRUGO,
> +		   show_cpufv_disabled, store_cpufv_disabled);

DEVICE_ATTR_RW()?

thanks,

greg k-h

Re: [PATCH 05/13] eeepc-laptop: use DEVICE_ATTR to instantiate device_attributes

[Frans Klaver]

On Fri, Sep 12, 2014 at 04:28:51PM -0700, Greg Kroah-Hartman wrote:
> On Sat, Sep 13, 2014 at 01:06:44AM +0200, Frans Klaver wrote:
> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> > ---
> >  drivers/platform/x86/eeepc-laptop.c | 35 +++++------------------------------
> >  1 file changed, 5 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > index f35d008..8225b1e 100644
> > --- a/drivers/platform/x86/eeepc-laptop.c
> > +++ b/drivers/platform/x86/eeepc-laptop.c
> > @@ -308,13 +308,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
> >  	{								\
> >  		return store_sys_acpi(dev, _cm, buf, count);		\
> >  	}								\
> > -	static struct device_attribute dev_attr_##_name = {		\
> > -		.attr = {						\
> > -			.name = __stringify(_name),			\
> > -			.mode = _mode },				\
> > -		.show   = show_##_name,					\
> > -		.store  = store_##_name,				\
> > -	}
> > +	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
> >  
> >  EEEPC_CREATE_DEVICE_ATTR(camera, S_IWUSR | S_IRUGO, CM_ASL_CAMERA);
> >  EEEPC_CREATE_DEVICE_ATTR(cardr, S_IWUSR | S_IRUGO, CM_ASL_CARDREADER);
> > @@ -420,29 +414,10 @@ static ssize_t store_cpufv_disabled(struct device *dev,
> >  }
> >  
> >  
> > -static struct device_attribute dev_attr_cpufv = {
> > -	.attr = {
> > -		.name = "cpufv",
> > -		.mode = S_IWUSR | S_IRUGO },
> > -	.show   = show_cpufv,
> > -	.store  = store_cpufv
> > -};
> > -
> > -static struct device_attribute dev_attr_available_cpufv = {
> > -	.attr = {
> > -		.name = "available_cpufv",
> > -		.mode = S_IRUGO },
> > -	.show   = show_available_cpufv
> > -};
> > -
> > -static struct device_attribute dev_attr_cpufv_disabled = {
> > -	.attr = {
> > -		.name = "cpufv_disabled",
> > -		.mode = S_IWUSR | S_IRUGO},
> > -	.show   = show_cpufv_disabled,
> > -	.store  = store_cpufv_disabled
> > -};
> > -
> > +static DEVICE_ATTR(cpufv, S_IWUSR | S_IRUGO, show_cpufv, store_cpufv);
> 
> DEVICE_ATTR_RW()?
> 
> > +static DEVICE_ATTR(available_cpufv, S_IRUGO, show_available_cpufv, NULL);
> 
> DEVICE_ATTR_RO()?
> 
> > +static DEVICE_ATTR(cpufv_disabled, S_IWUSR | S_IRUGO,
> > +		   show_cpufv_disabled, store_cpufv_disabled);
> 
> DEVICE_ATTR_RW()?

Yes, I could have expected that. v2 will use them across the board.

Thanks,
Frans

Re: [PATCH 03/13] eeepc-laptop: coding style: add curly braces around else compound

[Darren Hart]

On Sat, Sep 13, 2014 at 01:06:42AM +0200, Frans Klaver wrote:
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>

Hi Frans,

Please merge the 3 coding style fixes into one patch. There is no functional
change here, and each of the 3 is obvious. Please include a simple commit
message to the effect of:

"
eeepc-laptop: Coding style cleanup

Correct white space and brace usage per Documentation/CodingStyle.
"

As a point of policy, I will not merge commits without a message, no matter how
trivial.

Thanks,

> ---
>  drivers/platform/x86/eeepc-laptop.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 702fcbb..9d8dff9 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -1425,8 +1425,9 @@ static int eeepc_acpi_add(struct acpi_device *device)
>  		result = eeepc_backlight_init(eeepc);
>  		if (result)
>  			goto fail_backlight;
> -	} else
> +	} else {
>  		pr_info("Backlight controlled by ACPI video driver\n");
> +	}
>  
>  	result = eeepc_input_init(eeepc);
>  	if (result)
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 03/13] eeepc-laptop: coding style: add curly braces around else compound

[Frans Klaver]

On Mon, Sep 15, 2014 at 12:41:41PM -0700, Darren Hart wrote:
> On Sat, Sep 13, 2014 at 01:06:42AM +0200, Frans Klaver wrote:
> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> 
> Hi Frans,
> 
> Please merge the 3 coding style fixes into one patch. There is no functional
> change here, and each of the 3 is obvious. Please include a simple commit
> message to the effect of:
> 
> "
> eeepc-laptop: Coding style cleanup
> 
> Correct white space and brace usage per Documentation/CodingStyle.
> "
> 
> As a point of policy, I will not merge commits without a message, no matter how
> trivial.

That's fair enough. I'll do some sanity testing today or tomorrow and
roll v2, unless some other remarks come up in the mean time.

Thanks,
Frans

Re: [PATCH 07/13] eeepc-laptop: make disp attribute really write-only

[Darren Hart]

On Sat, Sep 13, 2014 at 01:06:46AM +0200, Frans Klaver wrote:
> The disp attribute is write-only, but sysfs doesn't know this. Currently
> show_sys_acpi() is mimicking sysfs behavior, if the underlying acpi call
> should fail. This is not ideal; behaving like sysfs is better left to
> sysfs.
> 
> Introduce EEEPC_CREATE_DEVICE_ATTR_WO() to instantiate a write-only
> attribute, and declare the disp attribute with it. Sysfs makes sure
> userspace can only write to disp at all times. This also means we can
> back to propagating any errors from get_acpi().
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 3c04b77..4f6490d 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -291,7 +291,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
>  	int value = get_acpi(eeepc, cm);
>  
>  	if (value < 0)
> -		return -EIO;
> +		return value;
>  	return sprintf(buf, "%d\n", value);
>  }
>  
> @@ -316,9 +316,13 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
>  	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
>  	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
>  
> +#define EEEPC_CREATE_DEVICE_ATTR_WO(_name, _mode, _cm)			\
> +	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
> +	static DEVICE_ATTR(_name, _mode, NULL, store_##_name)

Per Greg's previous reply, I presume DEVICE_ATTR_WO here?

> +
>  EEEPC_CREATE_DEVICE_ATTR(camera, S_IWUSR | S_IRUGO, CM_ASL_CAMERA);
>  EEEPC_CREATE_DEVICE_ATTR(cardr, S_IWUSR | S_IRUGO, CM_ASL_CARDREADER);
> -EEEPC_CREATE_DEVICE_ATTR(disp, S_IWUSR, CM_ASL_DISPLAYSWITCH);
> +EEEPC_CREATE_DEVICE_ATTR_WO(disp, S_IWUSR, CM_ASL_DISPLAYSWITCH);
>  
>  struct eeepc_cpufv {
>  	int num;
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 07/13] eeepc-laptop: make disp attribute really write-only

[Frans Klaver]

On Mon, Sep 15, 2014 at 01:00:07PM -0700, Darren Hart wrote:
> On Sat, Sep 13, 2014 at 01:06:46AM +0200, Frans Klaver wrote:
> > The disp attribute is write-only, but sysfs doesn't know this. Currently
> > show_sys_acpi() is mimicking sysfs behavior, if the underlying acpi call
> > should fail. This is not ideal; behaving like sysfs is better left to
> > sysfs.
> > 
> > Introduce EEEPC_CREATE_DEVICE_ATTR_WO() to instantiate a write-only
> > attribute, and declare the disp attribute with it. Sysfs makes sure
> > userspace can only write to disp at all times. This also means we can
> > back to propagating any errors from get_acpi().
> > 
> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> > ---
> >  drivers/platform/x86/eeepc-laptop.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > index 3c04b77..4f6490d 100644
> > --- a/drivers/platform/x86/eeepc-laptop.c
> > +++ b/drivers/platform/x86/eeepc-laptop.c
> > @@ -291,7 +291,7 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
> >  	int value = get_acpi(eeepc, cm);
> >  
> >  	if (value < 0)
> > -		return -EIO;
> > +		return value;
> >  	return sprintf(buf, "%d\n", value);
> >  }
> >  
> > @@ -316,9 +316,13 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
> >  	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
> >  	static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
> >  
> > +#define EEEPC_CREATE_DEVICE_ATTR_WO(_name, _mode, _cm)			\
> > +	EEEPC_ACPI_STORE_FUNC(_name, _cm)				\
> > +	static DEVICE_ATTR(_name, _mode, NULL, store_##_name)
> 
> Per Greg's previous reply, I presume DEVICE_ATTR_WO here?

Yes, already taken care of in all similar cases.

Thanks,
Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Sat, Sep 13, 2014 at 01:06:49AM +0200, Frans Klaver wrote:
> In get_cpufv the return value of get_acpi is stored in the cpufv struct.
> Right before this value is checked for errors, it is and'ed with 0xff.
> This means c->cur can never be less than zero. Besides that, the actual
> error value is ignored.
> 
> c->num is also and'ed with 0xff, which means we can ignore values below
> zero.
> 
> Check the result of get_acpi() right away. While at it, propagate the
> error if we got one.
> 
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 47488d3..828db56 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -332,9 +332,12 @@ struct eeepc_cpufv {
>  static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
>  {
>  	c->cur = get_acpi(eeepc, CM_ASL_CPUFV);
> +	if (c->cur < 0)
> +		return c->cur;
> +
>  	c->num = (c->cur >> 8) & 0xff;
>  	c->cur &= 0xff;
> -	if (c->cur < 0 || c->num <= 0 || c->num > 12)
> +	if (c->num == 0 || c->num > 12)
>  		return -ENODEV;
>  	return 0;

This patch is fine as is. However, Greg has supported propogating the error code
through to the sysfs interface (if I understand him correctly on an earlier post
to this list). This would require an addition change to this patch would
propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
error should probably return -ENXIO as I understand it.

However, there was a rather famous change in error code handling in which pulse
audio broke and Linus was very upset with one of his maintainers.

How do we know when it is acceptable to change which error code is returned?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 11/13] eeepc-laptop: propagate errors from get_cpufv

[Darren Hart]

On Sat, Sep 13, 2014 at 01:06:50AM +0200, Frans Klaver wrote:
> In show_available_cpufv, show_cpufv, and store_cpufv, errors from
> get_cpufv are mapped to -ENODEV. To paint a clear picture to callers,
> propagate the errors instead.
> 

Ooops, should have forwarded this one around instead. I generally agree with
this approach, but do want confirmation from Greg and others that this is
an acceptable change to the sysfs interface.

--
Darren

> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 828db56..732b012 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -351,8 +351,9 @@ static ssize_t show_available_cpufv(struct device *dev,
>  	int i;
>  	ssize_t len = 0;
>  
> -	if (get_cpufv(eeepc, &c))
> -		return -ENODEV;
> +	i = get_cpufv(eeepc, &c);
> +	if (i)
> +		return i;
>  	for (i = 0; i < c.num; i++)
>  		len += sprintf(buf + len, "%d ", i);
>  	len += sprintf(buf + len, "\n");
> @@ -365,9 +366,11 @@ static ssize_t show_cpufv(struct device *dev,
>  {
>  	struct eeepc_laptop *eeepc = dev_get_drvdata(dev);
>  	struct eeepc_cpufv c;
> +	int rv;
>  
> -	if (get_cpufv(eeepc, &c))
> -		return -ENODEV;
> +	rv = get_cpufv(eeepc, &c);
> +	if (rv)
> +		return rv;
>  	return sprintf(buf, "%#x\n", (c.num << 8) | c.cur);
>  }
>  
> @@ -381,8 +384,9 @@ static ssize_t store_cpufv(struct device *dev,
>  
>  	if (eeepc->cpufv_disabled)
>  		return -EPERM;
> -	if (get_cpufv(eeepc, &c))
> -		return -ENODEV;
> +	rv = get_cpufv(eeepc, &c);
> +	if (rv)
> +		return rv;
>  	rv = parse_arg(buf, &value);
>  	if (rv < 0)
>  		return rv;
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Greg Kroah-Hartman]

On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> On Sat, Sep 13, 2014 at 01:06:49AM +0200, Frans Klaver wrote:
> > In get_cpufv the return value of get_acpi is stored in the cpufv struct.
> > Right before this value is checked for errors, it is and'ed with 0xff.
> > This means c->cur can never be less than zero. Besides that, the actual
> > error value is ignored.
> > 
> > c->num is also and'ed with 0xff, which means we can ignore values below
> > zero.
> > 
> > Check the result of get_acpi() right away. While at it, propagate the
> > error if we got one.
> > 
> > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> > ---
> >  drivers/platform/x86/eeepc-laptop.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > index 47488d3..828db56 100644
> > --- a/drivers/platform/x86/eeepc-laptop.c
> > +++ b/drivers/platform/x86/eeepc-laptop.c
> > @@ -332,9 +332,12 @@ struct eeepc_cpufv {
> >  static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
> >  {
> >  	c->cur = get_acpi(eeepc, CM_ASL_CPUFV);
> > +	if (c->cur < 0)
> > +		return c->cur;
> > +
> >  	c->num = (c->cur >> 8) & 0xff;
> >  	c->cur &= 0xff;
> > -	if (c->cur < 0 || c->num <= 0 || c->num > 12)
> > +	if (c->num == 0 || c->num > 12)
> >  		return -ENODEV;
> >  	return 0;
> 
> This patch is fine as is. However, Greg has supported propogating the error code
> through to the sysfs interface (if I understand him correctly on an earlier post
> to this list). This would require an addition change to this patch would
> propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> error should probably return -ENXIO as I understand it.

I really have no idea at this point in time what to recommend.  How
about just stick with what is happening today so that:

> However, there was a rather famous change in error code handling in which pulse
> audio broke and Linus was very upset with one of his maintainers.

That doesn't happen :)

thanks,

greg k-h

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > On Sat, Sep 13, 2014 at 01:06:49AM +0200, Frans Klaver wrote:
> > > In get_cpufv the return value of get_acpi is stored in the cpufv struct.
> > > Right before this value is checked for errors, it is and'ed with 0xff.
> > > This means c->cur can never be less than zero. Besides that, the actual
> > > error value is ignored.
> > > 
> > > c->num is also and'ed with 0xff, which means we can ignore values below
> > > zero.
> > > 
> > > Check the result of get_acpi() right away. While at it, propagate the
> > > error if we got one.
> > > 
> > > Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> > > ---
> > >  drivers/platform/x86/eeepc-laptop.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> > > index 47488d3..828db56 100644
> > > --- a/drivers/platform/x86/eeepc-laptop.c
> > > +++ b/drivers/platform/x86/eeepc-laptop.c
> > > @@ -332,9 +332,12 @@ struct eeepc_cpufv {
> > >  static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
> > >  {
> > >  	c->cur = get_acpi(eeepc, CM_ASL_CPUFV);
> > > +	if (c->cur < 0)
> > > +		return c->cur;
> > > +
> > >  	c->num = (c->cur >> 8) & 0xff;
> > >  	c->cur &= 0xff;
> > > -	if (c->cur < 0 || c->num <= 0 || c->num > 12)
> > > +	if (c->num == 0 || c->num > 12)
> > >  		return -ENODEV;
> > >  	return 0;
> > 
> > This patch is fine as is. However, Greg has supported propogating the error code
> > through to the sysfs interface (if I understand him correctly on an earlier post
> > to this list). This would require an addition change to this patch would
> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > error should probably return -ENXIO as I understand it.
> 
> I really have no idea at this point in time what to recommend.  How
> about just stick with what is happening today so that:
> 
> > However, there was a rather famous change in error code handling in which pulse
> > audio broke and Linus was very upset with one of his maintainers.
> 
> That doesn't happen :)

So if I interpret that correctly, we're dropping the last patch
(ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
earlier, I already saw something else break because I returned ENXIO
instead of ENODEV.

Maybe it's a good idea to try and document the expected behavior
somewhere, if even Greg isn't sure what to do.

Thanks,
Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
>> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
>> >
>> > This patch is fine as is. However, Greg has supported propogating the error code
>> > through to the sysfs interface (if I understand him correctly on an earlier post
>> > to this list). This would require an addition change to this patch would
>> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
>> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
>> > error should probably return -ENXIO as I understand it.
>>
>> I really have no idea at this point in time what to recommend.  How
>> about just stick with what is happening today so that:
>>
>> > However, there was a rather famous change in error code handling in which pulse
>> > audio broke and Linus was very upset with one of his maintainers.
>>
>> That doesn't happen :)
>
> So if I interpret that correctly, we're dropping the last patch
> (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> earlier, I already saw something else break because I returned ENXIO
> instead of ENODEV.
>
> Maybe it's a good idea to try and document the expected behavior
> somewhere, if even Greg isn't sure what to do.

For good measure:

v2 will not change the return values at the sysfs interface, meaning
we will always return -ENODEV on error. I am going to try to keep as
much internal functions propagating errors as possible though, unless
someone strongly disagrees.

Thanks,
Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> >> >
> >> > This patch is fine as is. However, Greg has supported propogating the error code
> >> > through to the sysfs interface (if I understand him correctly on an earlier post
> >> > to this list). This would require an addition change to this patch would
> >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> >> > error should probably return -ENXIO as I understand it.
> >>
> >> I really have no idea at this point in time what to recommend.  How
> >> about just stick with what is happening today so that:
> >>
> >> > However, there was a rather famous change in error code handling in which pulse
> >> > audio broke and Linus was very upset with one of his maintainers.
> >>
> >> That doesn't happen :)
> >
> > So if I interpret that correctly, we're dropping the last patch
> > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > earlier, I already saw something else break because I returned ENXIO
> > instead of ENODEV.
> >
> > Maybe it's a good idea to try and document the expected behavior
> > somewhere, if even Greg isn't sure what to do.
> 
> For good measure:
> 
> v2 will not change the return values at the sysfs interface, meaning
> we will always return -ENODEV on error. I am going to try to keep as
> much internal functions propagating errors as possible though, unless
> someone strongly disagrees.
> 
> Thanks,
> Frans

I cornered Linus today and asked about this specifically. The policy is this:

Don't change the sysfs return codes without good reason. A good reason could be
a real bug or problem with the return codes. It could also be to consolidate
error handling which makes things more uniform, etc.

If this results in broken userspace, the maintainer will revert the change.

This is probably a good thing to add to sysfs-rules.txt. I'll prepare a patch.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Tue, Sep 16, 2014 at 01:52:47PM -0700, Darren Hart wrote:
> On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> > On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> > >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > >> >
> > >> > This patch is fine as is. However, Greg has supported propogating the error code
> > >> > through to the sysfs interface (if I understand him correctly on an earlier post
> > >> > to this list). This would require an addition change to this patch would
> > >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > >> > error should probably return -ENXIO as I understand it.
> > >>
> > >> I really have no idea at this point in time what to recommend.  How
> > >> about just stick with what is happening today so that:
> > >>
> > >> > However, there was a rather famous change in error code handling in which pulse
> > >> > audio broke and Linus was very upset with one of his maintainers.
> > >>
> > >> That doesn't happen :)
> > >
> > > So if I interpret that correctly, we're dropping the last patch
> > > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > > earlier, I already saw something else break because I returned ENXIO
> > > instead of ENODEV.
> > >
> > > Maybe it's a good idea to try and document the expected behavior
> > > somewhere, if even Greg isn't sure what to do.
> > 
> > For good measure:
> > 
> > v2 will not change the return values at the sysfs interface, meaning
> > we will always return -ENODEV on error. I am going to try to keep as
> > much internal functions propagating errors as possible though, unless
> > someone strongly disagrees.
> > 
> > Thanks,
> > Frans
> 
> I cornered Linus today and asked about this specifically. The policy is this:
> 
> Don't change the sysfs return codes without good reason. A good reason could be
> a real bug or problem with the return codes. It could also be to consolidate
> error handling which makes things more uniform, etc.
> 
> If this results in broken userspace, the maintainer will revert the change.

Alright, that is basically what I was expecting it to be. As it happens,
this also means that we'll have to decide what to do about returning
-EIO/-ENODEV/rv in show_sys_acpi and store_sys_acpi. The latter was
changed by Paul Bolle's "eeepc-laptop: simplify parse_arg()". The return
value of these functions is propagated to the sysfs interface.


> This is probably a good thing to add to sysfs-rules.txt. I'll prepare a patch.

Agreed.

While I was removing the patches changing the sysfs error behavior, I
also noticed that eeepc-laptop.c doesn't really seem to handle errors
uniformly. In some cases the callers of get_acpi and set_acpi don't even
seem to consider that these functions may return an error code (e.g.
eeepc_hotk_thaw()). I'd fix those in a new series though.

Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Tue, Sep 16, 2014 at 01:52:47PM -0700, Darren Hart wrote:
> On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> > On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> > >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > >> >
> > >> > This patch is fine as is. However, Greg has supported propogating the error code
> > >> > through to the sysfs interface (if I understand him correctly on an earlier post
> > >> > to this list). This would require an addition change to this patch would
> > >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > >> > error should probably return -ENXIO as I understand it.
> > >>
> > >> I really have no idea at this point in time what to recommend.  How
> > >> about just stick with what is happening today so that:
> > >>
> > >> > However, there was a rather famous change in error code handling in which pulse
> > >> > audio broke and Linus was very upset with one of his maintainers.
> > >>
> > >> That doesn't happen :)
> > >
> > > So if I interpret that correctly, we're dropping the last patch
> > > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > > earlier, I already saw something else break because I returned ENXIO
> > > instead of ENODEV.
> > >
> > > Maybe it's a good idea to try and document the expected behavior
> > > somewhere, if even Greg isn't sure what to do.
> > 
> > For good measure:
> > 
> > v2 will not change the return values at the sysfs interface, meaning
> > we will always return -ENODEV on error. I am going to try to keep as
> > much internal functions propagating errors as possible though, unless
> > someone strongly disagrees.
> > 
> > Thanks,
> > Frans
> 
> I cornered Linus today and asked about this specifically. The policy is this:
> 
> Don't change the sysfs return codes without good reason. A good reason could be
> a real bug or problem with the return codes. It could also be to consolidate
> error handling which makes things more uniform, etc.
> 
> If this results in broken userspace, the maintainer will revert the change.
> 
> This is probably a good thing to add to sysfs-rules.txt. I'll prepare a patch.
> 

What do people think of appending this to sysfs-rules.txt?

- When reading and writing sysfs device attribute files, avoid dependency
  on specific error codes wherever possible. This minimizes coupling to
  the error handling implemementation within the kernel.

  In general, failures to read or write sysfs device attributes shall
  propogate errors wherever possible. Common errors include, but are not
  limited to:

  -EIO: The read or store operation is not supported, typically returned by
        the sysfs system itself if the read or store pointer is NULL.

  -ENXIO: The read or store operation failed

  Error codes will not be changed without good reason, and should a change
  to error codes result in user-space breakage, it will be fixed, or the
  the offending change will be reverted.

  Userspace applications can, however, expect the format and contents of
  the attribute files to remain consistent in the absence of a version
  attribute change in the context of a given attributes.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Greg Kroah-Hartman]

On Tue, Sep 16, 2014 at 02:27:15PM -0700, Darren Hart wrote:
> On Tue, Sep 16, 2014 at 01:52:47PM -0700, Darren Hart wrote:
> > On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> > > On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > > > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> > > >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > > >> >
> > > >> > This patch is fine as is. However, Greg has supported propogating the error code
> > > >> > through to the sysfs interface (if I understand him correctly on an earlier post
> > > >> > to this list). This would require an addition change to this patch would
> > > >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > > >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > > >> > error should probably return -ENXIO as I understand it.
> > > >>
> > > >> I really have no idea at this point in time what to recommend.  How
> > > >> about just stick with what is happening today so that:
> > > >>
> > > >> > However, there was a rather famous change in error code handling in which pulse
> > > >> > audio broke and Linus was very upset with one of his maintainers.
> > > >>
> > > >> That doesn't happen :)
> > > >
> > > > So if I interpret that correctly, we're dropping the last patch
> > > > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > > > earlier, I already saw something else break because I returned ENXIO
> > > > instead of ENODEV.
> > > >
> > > > Maybe it's a good idea to try and document the expected behavior
> > > > somewhere, if even Greg isn't sure what to do.
> > > 
> > > For good measure:
> > > 
> > > v2 will not change the return values at the sysfs interface, meaning
> > > we will always return -ENODEV on error. I am going to try to keep as
> > > much internal functions propagating errors as possible though, unless
> > > someone strongly disagrees.
> > > 
> > > Thanks,
> > > Frans
> > 
> > I cornered Linus today and asked about this specifically. The policy is this:
> > 
> > Don't change the sysfs return codes without good reason. A good reason could be
> > a real bug or problem with the return codes. It could also be to consolidate
> > error handling which makes things more uniform, etc.
> > 
> > If this results in broken userspace, the maintainer will revert the change.
> > 
> > This is probably a good thing to add to sysfs-rules.txt. I'll prepare a patch.
> > 
> 
> What do people think of appending this to sysfs-rules.txt?
> 
> - When reading and writing sysfs device attribute files, avoid dependency
>   on specific error codes wherever possible. This minimizes coupling to
>   the error handling implemementation within the kernel.
> 
>   In general, failures to read or write sysfs device attributes shall
>   propogate errors wherever possible. Common errors include, but are not
>   limited to:
> 
>   -EIO: The read or store operation is not supported, typically returned by
>         the sysfs system itself if the read or store pointer is NULL.
> 
>   -ENXIO: The read or store operation failed
> 
>   Error codes will not be changed without good reason, and should a change
>   to error codes result in user-space breakage, it will be fixed, or the
>   the offending change will be reverted.
> 
>   Userspace applications can, however, expect the format and contents of
>   the attribute files to remain consistent in the absence of a version
>   attribute change in the context of a given attributes.

Looks reasonable, thanks.  Care to turn it into a patch that I can
apply?

thanks,

greg k-h

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Tue, Sep 16, 2014 at 02:27:15PM -0700, Darren Hart wrote:
> 
> - When reading and writing sysfs device attribute files, avoid dependency
>   on specific error codes wherever possible. This minimizes coupling to
>   the error handling implemementation within the kernel.
> 
>   In general, failures to read or write sysfs device attributes shall
>   propogate errors wherever possible. Common errors include, but are not
>   limited to:
> 
>   -EIO: The read or store operation is not supported, typically returned by
>         the sysfs system itself if the read or store pointer is NULL.
> 
>   -ENXIO: The read or store operation failed
> 
>   Error codes will not be changed without good reason, and should a change
>   to error codes result in user-space breakage, it will be fixed, or the
>   the offending change will be reverted.

sysfs-rules.txt is written for user space? In that case, reverting the
change is as much a fix as patching it.

> 
>   Userspace applications can, however, expect the format and contents of
>   the attribute files to remain consistent in the absence of a version
>   attribute change in the context of a given attributes.

...attribute.

That's it for the nit-picking.

Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Tue, Sep 16, 2014 at 11:40:37PM +0200, Frans Klaver wrote:
> On Tue, Sep 16, 2014 at 02:27:15PM -0700, Darren Hart wrote:
> > 
> > - When reading and writing sysfs device attribute files, avoid dependency
> >   on specific error codes wherever possible. This minimizes coupling to
> >   the error handling implemementation within the kernel.
> > 
> >   In general, failures to read or write sysfs device attributes shall
> >   propogate errors wherever possible. Common errors include, but are not
> >   limited to:
> > 
> >   -EIO: The read or store operation is not supported, typically returned by
> >         the sysfs system itself if the read or store pointer is NULL.
> > 
> >   -ENXIO: The read or store operation failed
> > 
> >   Error codes will not be changed without good reason, and should a change
> >   to error codes result in user-space breakage, it will be fixed, or the
> >   the offending change will be reverted.
> 
> sysfs-rules.txt is written for user space? In that case, reverting the
> change is as much a fix as patching it.
> 
> > 
> >   Userspace applications can, however, expect the format and contents of
> >   the attribute files to remain consistent in the absence of a version
> >   attribute change in the context of a given attributes.

Agreed, but I wanted to make this clear to kernel devs reading this as well,
rather than duplicating this blurb elsewhere.

> 
> ...attribute.
> 
> That's it for the nit-picking.

Thanks for the plural catch.

I'll send to Greg as a patch.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Tue, Sep 16, 2014 at 11:10:01PM +0200, Frans Klaver wrote:
> On Tue, Sep 16, 2014 at 01:52:47PM -0700, Darren Hart wrote:
> > On Tue, Sep 16, 2014 at 01:54:25PM +0200, Frans Klaver wrote:
> > > On Mon, Sep 15, 2014 at 11:55 PM, Frans Klaver <fransklaver@gmail.com> wrote:
> > > > On Mon, Sep 15, 2014 at 02:51:25PM -0700, Greg Kroah-Hartman wrote:
> > > >> On Mon, Sep 15, 2014 at 02:49:02PM -0700, Darren Hart wrote:
> > > >> >
> > > >> > This patch is fine as is. However, Greg has supported propogating the error code
> > > >> > through to the sysfs interface (if I understand him correctly on an earlier post
> > > >> > to this list). This would require an addition change to this patch would
> > > >> > propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
> > > >> > store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
> > > >> > error should probably return -ENXIO as I understand it.
> > > >>
> > > >> I really have no idea at this point in time what to recommend.  How
> > > >> about just stick with what is happening today so that:
> > > >>
> > > >> > However, there was a rather famous change in error code handling in which pulse
> > > >> > audio broke and Linus was very upset with one of his maintainers.
> > > >>
> > > >> That doesn't happen :)
> > > >
> > > > So if I interpret that correctly, we're dropping the last patch
> > > > (ENODEV -> ENXIO) from the series? That's fine by me. As mentioned
> > > > earlier, I already saw something else break because I returned ENXIO
> > > > instead of ENODEV.
> > > >
> > > > Maybe it's a good idea to try and document the expected behavior
> > > > somewhere, if even Greg isn't sure what to do.
> > > 
> > > For good measure:
> > > 
> > > v2 will not change the return values at the sysfs interface, meaning
> > > we will always return -ENODEV on error. I am going to try to keep as
> > > much internal functions propagating errors as possible though, unless
> > > someone strongly disagrees.
> > > 
> > > Thanks,
> > > Frans
> > 
> > I cornered Linus today and asked about this specifically. The policy is this:
> > 
> > Don't change the sysfs return codes without good reason. A good reason could be
> > a real bug or problem with the return codes. It could also be to consolidate
> > error handling which makes things more uniform, etc.
> > 
> > If this results in broken userspace, the maintainer will revert the change.
> 
> Alright, that is basically what I was expecting it to be. As it happens,
> this also means that we'll have to decide what to do about returning
> -EIO/-ENODEV/rv in show_sys_acpi and store_sys_acpi. The latter was
> changed by Paul Bolle's "eeepc-laptop: simplify parse_arg()". The return
> value of these functions is propagated to the sysfs interface.

Yes, that change to store_sys_acpi needs to be reverted. I don't think
show_sys_acpi is actually changed in that patch.

Paul, can you resend that patch with the store_sys_acpi() -EIO return restored?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Henrique de Moraes Holschuh]

On Tue, 16 Sep 2014, Darren Hart wrote:
> - When reading and writing sysfs device attribute files, avoid dependency
>   on specific error codes wherever possible. This minimizes coupling to
>   the error handling implemementation within the kernel.
> 
>   In general, failures to read or write sysfs device attributes shall
>   propogate errors wherever possible. Common errors include, but are not
>   limited to:
> 
>   -EIO: The read or store operation is not supported, typically returned by
>         the sysfs system itself if the read or store pointer is NULL.
> 
>   -ENXIO: The read or store operation failed

from errno(3):
       EIO             Input/output error (POSIX.1)
       ENXIO           No such device or address (POSIX.1)

It makes sense to retry EIO.  ENXIO means there's nobody listening at the
time, and isn't usually retried.

The device-based interfaces get it right.  A typical example is the
cpu-based devices, where ENXIO means "no such processor", while EIO means
"whatever you're trying to do failed",  so a MSR read would return ENXIO if
the processor core is offline/doesn't exist, and EIO if the processor core
is there, but raised a #GP when the MSR read was attempted.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Frans Klaver]

On Wed, Sep 17, 2014 at 12:34 PM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Tue, 16 Sep 2014, Darren Hart wrote:
>> - When reading and writing sysfs device attribute files, avoid dependency
>>   on specific error codes wherever possible. This minimizes coupling to
>>   the error handling implemementation within the kernel.
>>
>>   In general, failures to read or write sysfs device attributes shall
>>   propogate errors wherever possible. Common errors include, but are not
>>   limited to:
>>
>>   -EIO: The read or store operation is not supported, typically returned by
>>         the sysfs system itself if the read or store pointer is NULL.
>>
>>   -ENXIO: The read or store operation failed
>
> from errno(3):
>        EIO             Input/output error (POSIX.1)
>        ENXIO           No such device or address (POSIX.1)
>
> It makes sense to retry EIO.  ENXIO means there's nobody listening at the
> time, and isn't usually retried.
>
> The device-based interfaces get it right.  A typical example is the
> cpu-based devices, where ENXIO means "no such processor", while EIO means
> "whatever you're trying to do failed",  so a MSR read would return ENXIO if
> the processor core is offline/doesn't exist, and EIO if the processor core
> is there, but raised a #GP when the MSR read was attempted.

Here's something I don't quite understand. How should one then
distinguish between sysfs's use of EIO "can't (read from|write to)
this file", and this example's EIO "something went wrong, you might
want to try again"? Why not use EAGAIN "Resource temporarily
unavailable" in the case where trying again makes sense? I wouldn't
normally retry the last operation if I was just told something
actually went wrong.

I've only been at it for a couple of weeks, but I get the impression
that sysfs has never really been guided regarding error codes, or has
gone to live its own life now kept in check with "don't change the
errors, it may break userspace". Does that make sense?

Thanks,
Frans

Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get_cpufv

[Darren Hart]

On Wed, Sep 17, 2014 at 01:57:33PM +0200, Frans Klaver wrote:
> On Wed, Sep 17, 2014 at 12:34 PM, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
> > On Tue, 16 Sep 2014, Darren Hart wrote:
> >> - When reading and writing sysfs device attribute files, avoid dependency
> >>   on specific error codes wherever possible. This minimizes coupling to
> >>   the error handling implemementation within the kernel.
> >>
> >>   In general, failures to read or write sysfs device attributes shall
> >>   propogate errors wherever possible. Common errors include, but are not
> >>   limited to:
> >>
> >>   -EIO: The read or store operation is not supported, typically returned by
> >>         the sysfs system itself if the read or store pointer is NULL.
> >>
> >>   -ENXIO: The read or store operation failed
> >
> > from errno(3):
> >        EIO             Input/output error (POSIX.1)
> >        ENXIO           No such device or address (POSIX.1)
> >
> > It makes sense to retry EIO.  ENXIO means there's nobody listening at the
> > time, and isn't usually retried.
> >
> > The device-based interfaces get it right.  A typical example is the
> > cpu-based devices, where ENXIO means "no such processor", while EIO means
> > "whatever you're trying to do failed",  so a MSR read would return ENXIO if
> > the processor core is offline/doesn't exist, and EIO if the processor core
> > is there, but raised a #GP when the MSR read was attempted.
> 
> Here's something I don't quite understand. How should one then
> distinguish between sysfs's use of EIO "can't (read from|write to)
> this file", and this example's EIO "something went wrong, you might
> want to try again"? Why not use EAGAIN "Resource temporarily
> unavailable" in the case where trying again makes sense? I wouldn't
> normally retry the last operation if I was just told something
> actually went wrong.
> 
> I've only been at it for a couple of weeks, but I get the impression
> that sysfs has never really been guided regarding error codes, or has
> gone to live its own life now kept in check with "don't change the
> errors, it may break userspace". Does that make sense?

Right, this was the distinction I was trying to make with the above description.
Henrique's points are valid, but based on the sysfs subsystem already using EIO
in the way that it does, I felt the above made sense.

That said, I'm not personally tied to them, it's just what I have derived from
recent discussions on the subject and what I observed in existing usage.

-- 
Darren Hart
Intel Open Source Technology Center