[PATCH v2 0/9] eeepc cleanup

[Frans Klaver]

Here's the second installment cleaning up some things in the eeepc laptop
driver.

This depends on "eeepc-laptop: simplify parse_arg()".

For those interested, I keep a copy based on Darren's testing branch on
github:

https://github.com/fransklaver/linux.git tags/eeepc_cleanup_v2

v1..v2:
- squash coding style fixes
- drop patch moving to file permission macros, in favor of
- move towards better sysfs api usage
- drop changes to existing sysfs return values

Frans Klaver (9):
eeepc-laptop: clean up coding style
eeepc-laptop: change sysfs function names to API expectations
eeepc-laptop: use DEVICE_ATTR* to instantiate device_attributes
eeepc-laptop: pull out ACPI_STORE_FUNC and ACPI_SHOW_FUNC macros
eeepc-laptop: tell sysfs that the disp attribute is write-only
eeepc-laptop: pull out SENSOR_STORE_FUNC and SENSOR_SHOW_FUNC macros
eeepc-laptop: make fan1_input really read-only
eeepc-laptop: check proper return values in get_cpufv
eeepc-laptop: store_cpufv: return error if set_acpi fails

drivers/platform/x86/eeepc-laptop.c | 117 ++++++++++++++++++------------------
1 file changed, 59 insertions(+), 58 deletions(-)

[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.

This also causes EEEPC_CREATE_SENSOR_ATTR() to be only used for R/W
attributes.This enables us to drop the _mode argument from the macro
and use DEVICE_ATTR_RW() internally while we're at it. Append _RW to the
name for readability.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index ba251bb..e93a54e 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1039,7 +1039,7 @@ static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
}

#define EEEPC_SENSOR_SHOW_FUNC(_name, _get) \
- static ssize_t show_##_name(struct device *dev, \
+ static ssize_t _name##_show(struct device *dev, \
struct device_attribute *attr, \
char *buf) \
{ \
@@ -1047,23 +1047,27 @@ static ssize_t show_sys_hwmon(int (*get)(void), char *buf)
}

#define EEEPC_SENSOR_STORE_FUNC(_name, _set) \
- static ssize_t store_##_name(struct device *dev, \
+ static ssize_t _name##_store(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) \
+#define EEEPC_CREATE_SENSOR_ATTR_RW(_name, _get, _set) \
EEEPC_SENSOR_SHOW_FUNC(_name, _get) \
EEEPC_SENSOR_STORE_FUNC(_name, _set) \
- static DEVICE_ATTR(_name, _mode, show_##_name, store_##_name)
+ static DEVICE_ATTR_RW(_name)
+
+#define EEEPC_CREATE_SENSOR_ATTR_RO(_name, _get) \
+ EEEPC_SENSOR_SHOW_FUNC(_name, _get) \
+ static DEVICE_ATTR_RO(_name)

-EEEPC_CREATE_SENSOR_ATTR(fan1_input, S_IRUGO, eeepc_get_fan_rpm, NULL);
-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,
- eeepc_get_fan_ctrl, eeepc_set_fan_ctrl);
+EEEPC_CREATE_SENSOR_ATTR_RO(fan1_input, eeepc_get_fan_rpm);
+EEEPC_CREATE_SENSOR_ATTR_RW(pwm1, eeepc_get_fan_pwm,
+ eeepc_set_fan_pwm);
+EEEPC_CREATE_SENSOR_ATTR_RW(pwm1_enable, eeepc_get_fan_ctrl,
+ eeepc_set_fan_ctrl);

static struct attribute *hwmon_attrs[] = {
&dev_attr_pwm1.attr,

[Frans Klaver]

The result of set_acpi is left unchecked, but it may return errors. If
one occurs, send the error to the caller. There's no reason to lie about
it, if set_acpi fails.

Signed-off-by: Frans Klaver <***@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 875a43f..3f6c762 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -388,7 +388,9 @@ static ssize_t cpufv_store(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;
}

[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 <***@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 e93a54e..875a43f 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 -ENODEV;
+
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;
}

[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 <***@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 db26f78..c6d765f 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 _name##_show(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 _name##_store(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, _name##_show, _name##_store)

EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);

[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 <***@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 a85da4f..ba251bb 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1038,19 +1038,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);

[Frans Klaver]

Correct indentation and brace usage to comply with
Documentation/CodingStyle.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 3095d38..653999e 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,
@@ -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;
}
@@ -1424,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)

[Joe Perches]

Post by Frans Klaver
Correct indentation and brace usage to comply with
Documentation/CodingStyle.
---
drivers/platform/x86/eeepc-laptop.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 3095d38..653999e 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,
@@ -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;
}

This sort of code:

if (foo) {
[ do_something ]
} else
return -ERRVAL;

is generally better rewritten as:

if (!foo)
return -ERRVAL;

[ do_something ]

This gives immediacy to the error handler and
as well reduces unnecessary indentation.

So instead of:

static int eeepc_register_rfkill_notifier(struct eeepc_laptop *eeepc,
char *node)
{
acpi_status status;
acpi_handle handle;

status = acpi_get_handle(NULL, node, &handle);

if (ACPI_SUCCESS(status)) {
status = acpi_install_notify_handler(handle,
ACPI_SYSTEM_NOTIFY,
eeepc_rfkill_notify,
eeepc);
if (ACPI_FAILURE(status))
pr_warn("Failed to register notify on %s\n", node);

/*
* Refresh pci hotplug in case the rfkill state was
* changed during setup.
*/
eeepc_rfkill_hotplug(eeepc, handle);
} else
return -ENODEV;

return 0;
}

try something like:

static int eeepc_register_rfkill_notifier(struct eeepc_laptop *eeepc,
char *node)
{
acpi_handle handle;
acpi_status status = acpi_get_handle(NULL, node, &handle);

if (!ACPI_SUCCESS(status))
return -ENODEV;

status = acpi_install_notify_handler(handle,
ACPI_SYSTEM_NOTIFY,
eeepc_rfkill_notify,
eeepc);
if (ACPI_FAILURE(status))
pr_warn("Failed to register notify on %s\n", node);

/*
* Refresh pci hotplug in case the rfkill state was
* changed during setup.
*/
eeepc_rfkill_hotplug(eeepc, handle);

return 0;
}

[Frans Klaver]

Post by Frans Klaver

Post by Frans Klaver
Correct indentation and brace usage to comply with
Documentation/CodingStyle.
---
drivers/platform/x86/eeepc-laptop.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c

b/drivers/platform/x86/eeepc-laptop.c

Post by Frans Klaver
index 3095d38..653999e 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)

Post by Frans Klaver
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,
@@ -692,8 +692,9 @@ static int eeepc_register_rfkill_notifier(struct

eeepc_laptop *eeepc,

Post by Frans Klaver
* changed during setup.
*/
eeepc_rfkill_hotplug(eeepc, handle);
- } else
+ } else {
return -ENODEV;
+ }
return 0;
}

if (foo) {
[ do_something ]
} else
return -ERRVAL;
if (!foo)
return -ERRVAL;
[ do_something ]
This gives immediacy to the error handler and
as well reduces unnecessary indentation.

I fully agree.

Darren, do you still take this in one patch?

Frans

[Darren Hart]

Post by Frans Klaver

Post by Frans Klaver

Post by Frans Klaver
Correct indentation and brace usage to comply with
Documentation/CodingStyle.
---
drivers/platform/x86/eeepc-laptop.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c

b/drivers/platform/x86/eeepc-laptop.c

Post by Frans Klaver
index 3095d38..653999e 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)

Post by Frans Klaver
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,
@@ -692,8 +692,9 @@ static int eeepc_register_rfkill_notifier(struct

eeepc_laptop *eeepc,

Post by Frans Klaver
* changed during setup.
*/
eeepc_rfkill_hotplug(eeepc, handle);
- } else
+ } else {
return -ENODEV;
+ }
return 0;
}

if (foo) {
[ do_something ]
} else
return -ERRVAL;
if (!foo)
return -ERRVAL;
[ do_something ]
This gives immediacy to the error handler and
as well reduces unnecessary indentation.

I fully agree.
Darren, do you still take this in one patch?

I'll take the cleanup patch as is. I'm happy to take a separate patch to improve
the code flow.

[Frans Klaver]

Post by Frans Klaver

Post by Frans Klaver

Post by Frans Klaver

Post by Frans Klaver
Correct indentation and brace usage to comply with
Documentation/CodingStyle.
---
drivers/platform/x86/eeepc-laptop.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c

b/drivers/platform/x86/eeepc-laptop.c

Post by Frans Klaver
index 3095d38..653999e 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)

Post by Frans Klaver
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

*/

Post by Frans Klaver

Post by Frans Klaver

Post by Frans Klaver
- 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,
@@ -692,8 +692,9 @@ static int

eeepc_register_rfkill_notifier(struct

Post by Frans Klaver

Post by Frans Klaver
eeepc_laptop *eeepc,

Post by Frans Klaver
* changed during setup.
*/
eeepc_rfkill_hotplug(eeepc, handle);
- } else
+ } else {
return -ENODEV;
+ }
return 0;
}

if (foo) {
[ do_something ]
} else
return -ERRVAL;
if (!foo)
return -ERRVAL;
[ do_something ]
This gives immediacy to the error handler and
as well reduces unnecessary indentation.

I fully agree.
Darren, do you still take this in one patch?

I'll take the cleanup patch as is. I'm happy to take a separate patch to improve
the code flow.

Alright. Thanks.

Frans

[Darren Hart]

Post by Frans Klaver
Correct indentation and brace usage to comply with
Documentation/CodingStyle.

Queued, thanks!

[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 was introduced in 6dff29b63a5bf2eaf3 "eeepc-laptop:
disp attribute should be write-only". 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 removes the need for
mimicking the sysfs behavior in show_sys_acpi() and store_sys_acpi(),
but we'll stick with -EIO, as changing sysfs return values should not be
taken lightly.

This change also causes EEEPC_CREATE_DEVICE_ATTR() to be used only for
R/W attributes. This enables us to drop the _mode argument from the
macro and use DEVICE_ATTR_RW() internally while we're at it. Append _RW
to the name for readability.

Signed-off-by: Frans Klaver <***@gmail.com>
---
Here we're sticking with -EIO as return values. It should be said that the
commit mentioned above did change the error value from -ENODEV to -EIO. I'm
still in two minds about whether the show_sys_acpi and store_sys_acpi should go
back to returning ENODEV. We'll probably stick with -EIO, though, as there is
no strong reason other than "it was like that before".

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index c6d765f..a85da4f 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -311,14 +311,18 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
return store_sys_acpi(dev, _cm, buf, count); \
}

-#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm) \
+#define EEEPC_CREATE_DEVICE_ATTR_RW(_name, _cm) \
EEEPC_ACPI_SHOW_FUNC(_name, _cm) \
EEEPC_ACPI_STORE_FUNC(_name, _cm) \
- static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)
+ static DEVICE_ATTR_RW(_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);
+#define EEEPC_CREATE_DEVICE_ATTR_WO(_name, _cm) \
+ EEEPC_ACPI_STORE_FUNC(_name, _cm) \
+ static DEVICE_ATTR_WO(_name)
+
+EEEPC_CREATE_DEVICE_ATTR_RW(camera, CM_ASL_CAMERA);
+EEEPC_CREATE_DEVICE_ATTR_RW(cardr, CM_ASL_CARDREADER);
+EEEPC_CREATE_DEVICE_ATTR_WO(disp, CM_ASL_DISPLAYSWITCH);

struct eeepc_cpufv {
int num;

[Greg Kroah-Hartman]

Post by 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
disp attribute should be write-only". 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 removes the need for
mimicking the sysfs behavior in show_sys_acpi() and store_sys_acpi(),
but we'll stick with -EIO, as changing sysfs return values should not be
taken lightly.
This change also causes EEEPC_CREATE_DEVICE_ATTR() to be used only for
R/W attributes. This enables us to drop the _mode argument from the
macro and use DEVICE_ATTR_RW() internally while we're at it. Append _RW
to the name for readability.
---
Here we're sticking with -EIO as return values. It should be said that the
commit mentioned above did change the error value from -ENODEV to -EIO. I'm
still in two minds about whether the show_sys_acpi and store_sys_acpi should go
back to returning ENODEV. We'll probably stick with -EIO, though, as there is
no strong reason other than "it was like that before".
drivers/platform/x86/eeepc-laptop.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index c6d765f..a85da4f 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -311,14 +311,18 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
return store_sys_acpi(dev, _cm, buf, count); \
}
-#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm) \
+#define EEEPC_CREATE_DEVICE_ATTR_RW(_name, _cm) \
EEEPC_ACPI_SHOW_FUNC(_name, _cm) \
EEEPC_ACPI_STORE_FUNC(_name, _cm) \
- static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)
+ static DEVICE_ATTR_RW(_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);
+#define EEEPC_CREATE_DEVICE_ATTR_WO(_name, _cm) \
+ EEEPC_ACPI_STORE_FUNC(_name, _cm) \
+ static DEVICE_ATTR_WO(_name)
+
+EEEPC_CREATE_DEVICE_ATTR_RW(camera, CM_ASL_CAMERA);
+EEEPC_CREATE_DEVICE_ATTR_RW(cardr, CM_ASL_CARDREADER);
+EEEPC_CREATE_DEVICE_ATTR_WO(disp, CM_ASL_DISPLAYSWITCH);
struct eeepc_cpufv {
int num;

Ah, you did what I asked on a previous patch here, nevermind :)

greg k-h

[Frans Klaver]

Post by Frans Klaver

Post by Frans Klaver
The disp attribute is write-only, but sysfs doesn't know this.

Currently

Post by Frans Klaver
show_sys_acpi() is mimicking sysfs behavior, if the underlying acpi

call

Post by Frans Klaver
disp attribute should be write-only". This is not ideal; behaving

like

Post by Frans Klaver
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 removes the need

for

Post by Frans Klaver
mimicking the sysfs behavior in show_sys_acpi() and store_sys_acpi(),
but we'll stick with -EIO, as changing sysfs return values should not

be

Post by Frans Klaver
taken lightly.
This change also causes EEEPC_CREATE_DEVICE_ATTR() to be used only

for

Post by Frans Klaver
R/W attributes. This enables us to drop the _mode argument from the
macro and use DEVICE_ATTR_RW() internally while we're at it. Append

_RW

Post by Frans Klaver
to the name for readability.
---
Here we're sticking with -EIO as return values. It should be said

that the

Post by Frans Klaver
commit mentioned above did change the error value from -ENODEV to

-EIO. I'm

Post by Frans Klaver
still in two minds about whether the show_sys_acpi and store_sys_acpi

should go

Post by Frans Klaver
back to returning ENODEV. We'll probably stick with -EIO, though, as

there is

Post by Frans Klaver
no strong reason other than "it was like that before".
drivers/platform/x86/eeepc-laptop.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c

b/drivers/platform/x86/eeepc-laptop.c

Post by Frans Klaver
index c6d765f..a85da4f 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -311,14 +311,18 @@ static ssize_t show_sys_acpi(struct device

*dev, int cm, char *buf)

Post by Frans Klaver
return store_sys_acpi(dev, _cm, buf, count); \
}
-#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm) \
+#define EEEPC_CREATE_DEVICE_ATTR_RW(_name, _cm) \
EEEPC_ACPI_SHOW_FUNC(_name, _cm) \
EEEPC_ACPI_STORE_FUNC(_name, _cm) \
- static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)
+ static DEVICE_ATTR_RW(_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);
+#define EEEPC_CREATE_DEVICE_ATTR_WO(_name, _cm) \
+ EEEPC_ACPI_STORE_FUNC(_name, _cm) \
+ static DEVICE_ATTR_WO(_name)
+
+EEEPC_CREATE_DEVICE_ATTR_RW(camera, CM_ASL_CAMERA);
+EEEPC_CREATE_DEVICE_ATTR_RW(cardr, CM_ASL_CARDREADER);
+EEEPC_CREATE_DEVICE_ATTR_WO(disp, CM_ASL_DISPLAYSWITCH);
struct eeepc_cpufv {
int num;

Ah, you did what I asked on a previous patch here, nevermind :)

Yea, I thought I'd make more sense this way.

Thanks,
Frans

[Frans Klaver]

The eeepc-laptop driver follows the function naming convention
<action>_<attrname>(), while the sysfs macros are built around the
convention <attrname>_<action>(). Rename the sysfs functions to the
convention used by sysfs. This makes it easier to use the available API
later on.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 653999e..0094449 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -296,13 +296,13 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
}

#define EEEPC_CREATE_DEVICE_ATTR(_name, _mode, _cm) \
- static ssize_t show_##_name(struct device *dev, \
+ static ssize_t _name##_show(struct device *dev, \
struct device_attribute *attr, \
char *buf) \
{ \
return show_sys_acpi(dev, _cm, buf); \
} \
- static ssize_t store_##_name(struct device *dev, \
+ static ssize_t _name##_store(struct device *dev, \
struct device_attribute *attr, \
const char *buf, size_t count) \
{ \
@@ -312,8 +312,8 @@ static ssize_t show_sys_acpi(struct device *dev, int cm, char *buf)
.attr = { \
.name = __stringify(_name), \
.mode = _mode }, \
- .show = show_##_name, \
- .store = store_##_name, \
+ .show = _name##_show, \
+ .store = _name##_store, \
}

EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
@@ -335,7 +335,7 @@ static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
return 0;
}

-static ssize_t show_available_cpufv(struct device *dev,
+static ssize_t available_cpufv_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
@@ -352,7 +352,7 @@ static ssize_t show_available_cpufv(struct device *dev,
return len;
}

-static ssize_t show_cpufv(struct device *dev,
+static ssize_t cpufv_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
@@ -364,7 +364,7 @@ static ssize_t show_cpufv(struct device *dev,
return sprintf(buf, "%#x\n", (c.num << 8) | c.cur);
}

-static ssize_t store_cpufv(struct device *dev,
+static ssize_t cpufv_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
@@ -385,7 +385,7 @@ static ssize_t store_cpufv(struct device *dev,
return count;
}

-static ssize_t show_cpufv_disabled(struct device *dev,
+static ssize_t cpufv_disabled_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
@@ -394,7 +394,7 @@ static ssize_t show_cpufv_disabled(struct device *dev,
return sprintf(buf, "%d\n", eeepc->cpufv_disabled);
}

-static ssize_t store_cpufv_disabled(struct device *dev,
+static ssize_t cpufv_disabled_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
@@ -424,23 +424,23 @@ static struct device_attribute dev_attr_cpufv = {
.attr = {
.name = "cpufv",
.mode = 0644 },
- .show = show_cpufv,
- .store = store_cpufv
+ .show = cpufv_show,
+ .store = cpufv_store
};

static struct device_attribute dev_attr_available_cpufv = {
.attr = {
.name = "available_cpufv",
.mode = 0444 },
- .show = show_available_cpufv
+ .show = available_cpufv_show
};

static struct device_attribute dev_attr_cpufv_disabled = {
.attr = {
.name = "cpufv_disabled",
.mode = 0644 },
- .show = show_cpufv_disabled,
- .store = store_cpufv_disabled
+ .show = cpufv_disabled_show,
+ .store = cpufv_disabled_store
};

[Frans Klaver]

Device attributes are instantiated manually, while we have DEVICE_ATTR*
macros available to do much of the work for us. Let's use them.

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

diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 0094449..db26f78 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 = _name##_show, \
- .store = _name##_store, \
- }
+ static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)

EEEPC_CREATE_DEVICE_ATTR(camera, 0644, CM_ASL_CAMERA);
EEEPC_CREATE_DEVICE_ATTR(cardr, 0644, CM_ASL_CARDREADER);
@@ -420,29 +414,9 @@ static ssize_t cpufv_disabled_store(struct device *dev,
}


-static struct device_attribute dev_attr_cpufv = {
- .attr = {
- .name = "cpufv",
- .mode = 0644 },
- .show = cpufv_show,
- .store = cpufv_store
-};
-
-static struct device_attribute dev_attr_available_cpufv = {
- .attr = {
- .name = "available_cpufv",
- .mode = 0444 },
- .show = available_cpufv_show
-};
-
-static struct device_attribute dev_attr_cpufv_disabled = {
- .attr = {
- .name = "cpufv_disabled",
- .mode = 0644 },
- .show = cpufv_disabled_show,
- .store = cpufv_disabled_store
-};
-
+static DEVICE_ATTR_RW(cpufv);
+static DEVICE_ATTR_RO(available_cpufv);
+static DEVICE_ATTR_RW(cpufv_disabled);

static struct attribute *platform_attributes[] = {
&dev_attr_camera.attr,

[Greg Kroah-Hartman]

Post by Frans Klaver
Device attributes are instantiated manually, while we have DEVICE_ATTR*
macros available to do much of the work for us. Let's use them.
---
drivers/platform/x86/eeepc-laptop.c | 34 ++++------------------------------
1 file changed, 4 insertions(+), 30 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 0094449..db26f78 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 = _name##_show, \
- .store = _name##_store, \
- }
+ static DEVICE_ATTR(_name, _mode, _name##_show, _name##_store)

Can you change this to be DEVICE_ATTR_RO()?

If not, split it up into 2 different macros, one using the _RW and one
_RO version, _never_ have the driver specify the mode of the sysfs file
please.

thanks,

greg k-h

[Darren Hart]

Post by Frans Klaver
Here's the second installment cleaning up some things in the eeepc laptop
driver.
This depends on "eeepc-laptop: simplify parse_arg()".
For those interested, I keep a copy based on Darren's testing branch on

Please note, "testing" is strictly for use with Fengguang's 0day robot. It
*will* rebase and be recreated willy nilly.

If you want to base on something I've queued, I suggest the "for-next" branch as
I expect that to be far more stable. Rabses should be *very* rare there.

Feel free to use "testing" if you need to, just be aware it will rebase.

Post by Frans Klaver
https://github.com/fransklaver/linux.git tags/eeepc_cleanup_v2

Queued for testing.

Thanks Frans!

[Frans Klaver]

Post by Frans Klaver

Post by Frans Klaver
Here's the second installment cleaning up some things in the eeepc

laptop

Post by Frans Klaver
driver.
This depends on "eeepc-laptop: simplify parse_arg()".
For those interested, I keep a copy based on Darren's testing branch

on
Please note, "testing" is strictly for use with Fengguang's 0day robot. It
*will* rebase and be recreated willy nilly.
If you want to base on something I've queued, I suggest the "for-next" branch as
I expect that to be far more stable. Rabses should be *very* rare there.
Feel free to use "testing" if you need to, just be aware it will rebase.

I'll keep that in mind for the next batch ;)

Post by Frans Klaver

Post by Frans Klaver
https://github.com/fransklaver/linux.git tags/eeepc_cleanup_v2

Queued for testing.
Thanks Frans!

Thanks,
Frans