* [PATCH 1/9] eeepc-laptop: clean up coding style
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
@ 2014-09-17 21:47 ` Frans Klaver
2014-09-17 22:06 ` Joe Perches
2014-09-19 16:43 ` Darren Hart
2014-09-17 21:47 ` [PATCH 2/9] eeepc-laptop: change sysfs function names to API expectations Frans Klaver
` (8 subsequent siblings)
9 siblings, 2 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-17 21:47 UTC (permalink / raw)
To: Darren Hart
Cc: Frans Klaver, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
Correct indentation and brace usage to comply with
Documentation/CodingStyle.
Signed-off-by: Frans Klaver <fransklaver@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)
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/9] eeepc-laptop: clean up coding style
2014-09-17 21:47 ` [PATCH 1/9] eeepc-laptop: clean up coding style Frans Klaver
@ 2014-09-17 22:06 ` Joe Perches
2014-09-18 5:01 ` Frans Klaver
2014-09-19 16:43 ` Darren Hart
1 sibling, 1 reply; 20+ messages in thread
From: Joe Perches @ 2014-09-17 22:06 UTC (permalink / raw)
To: Frans Klaver
Cc: Darren Hart, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
On Wed, 2014-09-17 at 23:47 +0200, Frans Klaver wrote:
> Correct indentation and brace usage to comply with
> Documentation/CodingStyle.
>
> Signed-off-by: Frans Klaver <fransklaver@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;
> }
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;
}
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/9] eeepc-laptop: clean up coding style
2014-09-17 22:06 ` Joe Perches
@ 2014-09-18 5:01 ` Frans Klaver
2014-09-19 16:46 ` Darren Hart
0 siblings, 1 reply; 20+ messages in thread
From: Frans Klaver @ 2014-09-18 5:01 UTC (permalink / raw)
To: Joe Perches
Cc: Darren Hart, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
On 18 September 2014 00:06:52 CEST, Joe Perches <joe@perches.com> wrote:
>On Wed, 2014-09-17 at 23:47 +0200, Frans Klaver wrote:
>> Correct indentation and brace usage to comply with
>> Documentation/CodingStyle.
>>
>> Signed-off-by: Frans Klaver <fransklaver@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;
>> }
>
>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.
I fully agree.
Darren, do you still take this in one patch?
Frans
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/9] eeepc-laptop: clean up coding style
2014-09-18 5:01 ` Frans Klaver
@ 2014-09-19 16:46 ` Darren Hart
2014-09-19 17:17 ` Frans Klaver
0 siblings, 1 reply; 20+ messages in thread
From: Darren Hart @ 2014-09-19 16:46 UTC (permalink / raw)
To: Frans Klaver
Cc: Joe Perches, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
On Thu, Sep 18, 2014 at 07:01:25AM +0200, Frans Klaver wrote:
> On 18 September 2014 00:06:52 CEST, Joe Perches <joe@perches.com> wrote:
> >On Wed, 2014-09-17 at 23:47 +0200, Frans Klaver wrote:
> >> Correct indentation and brace usage to comply with
> >> Documentation/CodingStyle.
> >>
> >> Signed-off-by: Frans Klaver <fransklaver@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;
> >> }
> >
> >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.
>
> 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.
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/9] eeepc-laptop: clean up coding style
2014-09-19 16:46 ` Darren Hart
@ 2014-09-19 17:17 ` Frans Klaver
0 siblings, 0 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-19 17:17 UTC (permalink / raw)
To: Darren Hart
Cc: Joe Perches, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
On 19 September 2014 18:46:03 CEST, Darren Hart <dvhart@infradead.org> wrote:
>On Thu, Sep 18, 2014 at 07:01:25AM +0200, Frans Klaver wrote:
>> On 18 September 2014 00:06:52 CEST, Joe Perches <joe@perches.com>
>wrote:
>> >On Wed, 2014-09-17 at 23:47 +0200, Frans Klaver wrote:
>> >> Correct indentation and brace usage to comply with
>> >> Documentation/CodingStyle.
>> >>
>> >> Signed-off-by: Frans Klaver <fransklaver@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;
>> >> }
>> >
>> >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.
>>
>> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/9] eeepc-laptop: clean up coding style
2014-09-17 21:47 ` [PATCH 1/9] eeepc-laptop: clean up coding style Frans Klaver
2014-09-17 22:06 ` Joe Perches
@ 2014-09-19 16:43 ` Darren Hart
1 sibling, 0 replies; 20+ messages in thread
From: Darren Hart @ 2014-09-19 16:43 UTC (permalink / raw)
To: Frans Klaver
Cc: Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
On Wed, Sep 17, 2014 at 11:47:19PM +0200, Frans Klaver wrote:
> Correct indentation and brace usage to comply with
> Documentation/CodingStyle.
>
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
Queued, thanks!
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/9] eeepc-laptop: change sysfs function names to API expectations
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
2014-09-17 21:47 ` [PATCH 1/9] eeepc-laptop: clean up coding style Frans Klaver
@ 2014-09-17 21:47 ` Frans Klaver
2014-09-17 21:47 ` [PATCH 3/9] eeepc-laptop: use DEVICE_ATTR* to instantiate device_attributes Frans Klaver
` (7 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-17 21:47 UTC (permalink / raw)
To: Darren Hart
Cc: Frans Klaver, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
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 <fransklaver@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
};
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 3/9] eeepc-laptop: use DEVICE_ATTR* to instantiate device_attributes
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
2014-09-17 21:47 ` [PATCH 1/9] eeepc-laptop: clean up coding style Frans Klaver
2014-09-17 21:47 ` [PATCH 2/9] eeepc-laptop: change sysfs function names to API expectations Frans Klaver
@ 2014-09-17 21:47 ` Frans Klaver
2014-09-17 22:06 ` Greg Kroah-Hartman
2014-09-17 21:47 ` [PATCH 4/9] eeepc-laptop: pull out ACPI_STORE_FUNC and ACPI_SHOW_FUNC macros Frans Klaver
` (6 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Frans Klaver @ 2014-09-17 21:47 UTC (permalink / raw)
To: Darren Hart
Cc: Frans Klaver, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
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 <fransklaver@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,
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/9] eeepc-laptop: use DEVICE_ATTR* to instantiate device_attributes
2014-09-17 21:47 ` [PATCH 3/9] eeepc-laptop: use DEVICE_ATTR* to instantiate device_attributes Frans Klaver
@ 2014-09-17 22:06 ` Greg Kroah-Hartman
0 siblings, 0 replies; 20+ messages in thread
From: Greg Kroah-Hartman @ 2014-09-17 22:06 UTC (permalink / raw)
To: Frans Klaver
Cc: Darren Hart, Corentin Chary, Rafael Wysocki, acpi4asus-user,
platform-driver-x86, linux-kernel
On Wed, Sep 17, 2014 at 11:47:21PM +0200, Frans Klaver wrote:
> 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 <fransklaver@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)
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/9] eeepc-laptop: pull out ACPI_STORE_FUNC and ACPI_SHOW_FUNC macros
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
` (2 preceding siblings ...)
2014-09-17 21:47 ` [PATCH 3/9] eeepc-laptop: use DEVICE_ATTR* to instantiate device_attributes Frans Klaver
@ 2014-09-17 21:47 ` Frans Klaver
2014-09-17 21:47 ` [PATCH 5/9] eeepc-laptop: tell sysfs that the disp attribute is write-only Frans Klaver
` (5 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-17 21:47 UTC (permalink / raw)
To: Darren Hart
Cc: Frans Klaver, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
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 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);
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 5/9] eeepc-laptop: tell sysfs that the disp attribute is write-only
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
` (3 preceding siblings ...)
2014-09-17 21:47 ` [PATCH 4/9] eeepc-laptop: pull out ACPI_STORE_FUNC and ACPI_SHOW_FUNC macros Frans Klaver
@ 2014-09-17 21:47 ` Frans Klaver
2014-09-17 22:07 ` Greg Kroah-Hartman
2014-09-17 21:47 ` [PATCH 6/9] eeepc-laptop: pull out SENSOR_STORE_FUNC and SENSOR_SHOW_FUNC macros Frans Klaver
` (4 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Frans Klaver @ 2014-09-17 21:47 UTC (permalink / raw)
To: Darren Hart
Cc: Frans Klaver, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
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 <fransklaver@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;
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/9] eeepc-laptop: tell sysfs that the disp attribute is write-only
2014-09-17 21:47 ` [PATCH 5/9] eeepc-laptop: tell sysfs that the disp attribute is write-only Frans Klaver
@ 2014-09-17 22:07 ` Greg Kroah-Hartman
2014-09-18 5:04 ` Frans Klaver
0 siblings, 1 reply; 20+ messages in thread
From: Greg Kroah-Hartman @ 2014-09-17 22:07 UTC (permalink / raw)
To: Frans Klaver
Cc: Darren Hart, Corentin Chary, Rafael Wysocki, acpi4asus-user,
platform-driver-x86, linux-kernel
On Wed, Sep 17, 2014 at 11:47:23PM +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 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 <fransklaver@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;
Ah, you did what I asked on a previous patch here, nevermind :)
greg k-h
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/9] eeepc-laptop: tell sysfs that the disp attribute is write-only
2014-09-17 22:07 ` Greg Kroah-Hartman
@ 2014-09-18 5:04 ` Frans Klaver
0 siblings, 0 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-18 5:04 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Darren Hart, Corentin Chary, Rafael Wysocki, acpi4asus-user,
platform-driver-x86, linux-kernel
On 18 September 2014 00:07:53 CEST, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>On Wed, Sep 17, 2014 at 11:47:23PM +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 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 <fransklaver@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;
>
>Ah, you did what I asked on a previous patch here, nevermind :)
Yea, I thought I'd make more sense this way.
Thanks,
Frans
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/9] eeepc-laptop: pull out SENSOR_STORE_FUNC and SENSOR_SHOW_FUNC macros
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
` (4 preceding siblings ...)
2014-09-17 21:47 ` [PATCH 5/9] eeepc-laptop: tell sysfs that the disp attribute is write-only Frans Klaver
@ 2014-09-17 21:47 ` Frans Klaver
2014-09-17 21:47 ` [PATCH 7/9] eeepc-laptop: make fan1_input really read-only Frans Klaver
` (3 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-17 21:47 UTC (permalink / raw)
To: Darren Hart
Cc: Frans Klaver, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
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 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);
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 7/9] eeepc-laptop: make fan1_input really read-only
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
` (5 preceding siblings ...)
2014-09-17 21:47 ` [PATCH 6/9] eeepc-laptop: pull out SENSOR_STORE_FUNC and SENSOR_SHOW_FUNC macros Frans Klaver
@ 2014-09-17 21:47 ` Frans Klaver
2014-09-17 21:47 ` [PATCH 8/9] eeepc-laptop: check proper return values in get_cpufv Frans Klaver
` (2 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-17 21:47 UTC (permalink / raw)
To: Darren Hart
Cc: Frans Klaver, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
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 <fransklaver@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,
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 8/9] eeepc-laptop: check proper return values in get_cpufv
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
` (6 preceding siblings ...)
2014-09-17 21:47 ` [PATCH 7/9] eeepc-laptop: make fan1_input really read-only Frans Klaver
@ 2014-09-17 21:47 ` Frans Klaver
2014-09-17 21:47 ` [PATCH 9/9] eeepc-laptop: store_cpufv: return error if set_acpi fails Frans Klaver
2014-09-19 17:25 ` [PATCH v2 0/9] eeepc cleanup Darren Hart
9 siblings, 0 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-17 21:47 UTC (permalink / raw)
To: Darren Hart
Cc: Frans Klaver, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
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 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;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 9/9] eeepc-laptop: store_cpufv: return error if set_acpi fails
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
` (7 preceding siblings ...)
2014-09-17 21:47 ` [PATCH 8/9] eeepc-laptop: check proper return values in get_cpufv Frans Klaver
@ 2014-09-17 21:47 ` Frans Klaver
2014-09-19 17:25 ` [PATCH v2 0/9] eeepc cleanup Darren Hart
9 siblings, 0 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-17 21:47 UTC (permalink / raw)
To: Darren Hart
Cc: Frans Klaver, Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
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 <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 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;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 0/9] eeepc cleanup
2014-09-17 21:47 [PATCH v2 0/9] eeepc cleanup Frans Klaver
` (8 preceding siblings ...)
2014-09-17 21:47 ` [PATCH 9/9] eeepc-laptop: store_cpufv: return error if set_acpi fails Frans Klaver
@ 2014-09-19 17:25 ` Darren Hart
2014-09-19 17:33 ` Frans Klaver
9 siblings, 1 reply; 20+ messages in thread
From: Darren Hart @ 2014-09-19 17:25 UTC (permalink / raw)
To: Frans Klaver
Cc: Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
On Wed, Sep 17, 2014 at 11:47:18PM +0200, Frans Klaver wrote:
> 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:
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.
>
> https://github.com/fransklaver/linux.git tags/eeepc_cleanup_v2
Queued for testing.
Thanks Frans!
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 0/9] eeepc cleanup
2014-09-19 17:25 ` [PATCH v2 0/9] eeepc cleanup Darren Hart
@ 2014-09-19 17:33 ` Frans Klaver
0 siblings, 0 replies; 20+ messages in thread
From: Frans Klaver @ 2014-09-19 17:33 UTC (permalink / raw)
To: Darren Hart
Cc: Corentin Chary, Rafael Wysocki, Greg Kroah-Hartman,
acpi4asus-user, platform-driver-x86, linux-kernel
On 19 September 2014 19:25:38 CEST, Darren Hart <dvhart@infradead.org> wrote:
>On Wed, Sep 17, 2014 at 11:47:18PM +0200, Frans Klaver wrote:
>> 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:
>
>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 ;)
>>
>> https://github.com/fransklaver/linux.git tags/eeepc_cleanup_v2
>
>Queued for testing.
>
>Thanks Frans!
Thanks,
Frans
^ permalink raw reply [flat|nested] 20+ messages in thread