Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1626925 > unrolled thread

[PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Darren Hart <dvhart@infradead.org> - 2017-04-20 04:30 +0200
    [PATCH 9/9] platform/x86: hp-wmi: Cleanup exit paths Darren Hart <dvhart@infradead.org> - 2017-04-20 04:30 +0200
    [PATCH 5/9] platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions Darren Hart <dvhart@infradead.org> - 2017-04-20 04:30 +0200
    [PATCH 8/9] platform/x86: hp-wmi: Do not shadow errors in sysfs show functions Darren Hart <dvhart@infradead.org> - 2017-04-20 04:30 +0200
    [PATCH 7/9] platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros Darren Hart <dvhart@infradead.org> - 2017-04-20 04:30 +0200
    Re: [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-20 09:40 +0200
      Re: [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and  cleanups Darren Hart <dvhart@infradead.org> - 2017-04-20 22:30 +0200
    Re: [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups Carlo Caione <carlo@caione.org> - 2017-04-20 11:10 +0200

#1626925 — [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups

From: "Darren Hart (VMware)" <dvhart@infradead.org>

This series factors out some redundant code, cleans up a number of style issues,
modernizes the sysfs usage, and cleans up the return paths. All told, the driver
is reduced in size by 37 lines (3.6%).

I do not have an HP laptop, so I'm hoping Carlo can help out with some testing.
In particular we need to verify that hotkeys and sysfs continue to work as
before.

This series is also available here for convenience:
git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git hp-wmi


Darren Hart (VMware) (9):
  platform/x86: hp-wmi: Cleanup local variable declarations
  platform/x86: hp-wmi: Add bios_args initializer
  platform/x86: hp-wmi: Standardize enum usage for constants
  platform/x86: hp-wmi: Refactor redundant HPWMI_READ functions
  platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions
  platform/x86: hp-wmi: Refactor dock and tablet state fetchers
  platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros
  platform/x86: hp-wmi: Do not shadow errors in sysfs show functions
  platform/x86: hp-wmi: Cleanup exit paths

 drivers/platform/x86/hp-wmi.c | 385 +++++++++++++++++++-----------------------
 1 file changed, 174 insertions(+), 211 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1626926 — [PATCH 9/9] platform/x86: hp-wmi: Cleanup exit paths

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Several exit paths were more complex than they needed to be. Remove
superfluous conditionals, use labels common cleanup, do not shadow
negative error codes.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 63 ++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 90b8652..effb5d5 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -217,7 +217,7 @@ static int hp_wmi_perform_query(int query, int write, void *buffer,
 	struct bios_return *bios_return;
 	union acpi_object *obj;
 	int actual_outsize;
-	u32 rc;
+	int ret = 0;
 
 	if (WARN_ON(insize > sizeof(args.data)))
 		return -EINVAL;
@@ -229,32 +229,32 @@ static int hp_wmi_perform_query(int query, int write, void *buffer,
 
 	if (!obj)
 		return -EINVAL;
-	else if (obj->type != ACPI_TYPE_BUFFER) {
-		kfree(obj);
-		return -EINVAL;
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		ret = -EINVAL;
+		goto out_free;
 	}
 
 	bios_return = (struct bios_return *)obj->buffer.pointer;
-	rc = bios_return->return_code;
+	ret = bios_return->return_code;
 
-	if (rc) {
-		if (rc != HPWMI_RET_UNKNOWN_CMDTYPE)
-			pr_warn("query 0x%x returned error 0x%x\n", query, rc);
-		kfree(obj);
-		return rc;
+	if (ret) {
+		if (ret != HPWMI_RET_UNKNOWN_CMDTYPE)
+			pr_warn("query 0x%x returned error 0x%x\n", query, ret);
+		goto out_free;
 	}
 
-	if (!outsize) {
-		/* ignore output data */
-		kfree(obj);
-		return 0;
-	}
+	/* Ignore output data of zero size */
+	if (!outsize)
+		goto out_free;
 
 	actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return)));
 	memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize);
 	memset(buffer + actual_outsize, 0, outsize - actual_outsize);
+
+out_free:
 	kfree(obj);
-	return 0;
+	return ret;
 }
 
 static int hp_wmi_read_int(int query)
@@ -307,9 +307,8 @@ static int __init hp_wmi_enable_hotkeys(void)
 	int value = 0x6e;
 	int ret = hp_wmi_perform_query(HPWMI_BIOS_QUERY, HPWMI_WRITE, &value,
 				       sizeof(value), 0);
-	if (ret)
-		return ret < 0 ? ret : -EINVAL;
-	return 0;
+
+	return ret <= 0 ? ret : -EINVAL;
 }
 
 static int hp_wmi_set_block(void *data, bool blocked)
@@ -320,9 +319,8 @@ static int hp_wmi_set_block(void *data, bool blocked)
 
 	ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_WRITE,
 				   &query, sizeof(query), 0);
-	if (ret)
-		return ret < 0 ? ret : -EINVAL;
-	return 0;
+
+	return ret <= 0 ? ret : -EINVAL;
 }
 
 static const struct rfkill_ops hp_wmi_rfkill_ops = {
@@ -357,11 +355,12 @@ static int hp_wmi_rfkill2_set_block(void *data, bool blocked)
 {
 	int rfkill_id = (int)(long)data;
 	char buffer[4] = { 0x01, 0x00, rfkill_id, !blocked };
+	int ret;
 
-	if (hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_WRITE,
-				   buffer, sizeof(buffer), 0))
-		return -EINVAL;
-	return 0;
+	ret = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_WRITE,
+				   buffer, sizeof(buffer), 0);
+
+	return ret <= 0 ? ret : -EINVAL;
 }
 
 static const struct rfkill_ops hp_wmi_rfkill2_ops = {
@@ -472,13 +471,17 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
 	u32 tmp;
 
 	ret = kstrtoul(buf, 10, &tmp2);
-	if (ret || tmp2 != 1)
-		return -EINVAL;
+	if (!ret && tmp2 != 1)
+		ret = -EINVAL;
+	if (ret)
+		goto out;
 
 	/* Clear the POST error code. It is kept until until cleared. */
 	tmp = (u32) tmp2;
 	ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_WRITE, &tmp,
 				       sizeof(tmp), sizeof(tmp));
+
+out:
 	if (ret)
 		return ret < 0 ? ret : -EINVAL;
 
@@ -683,7 +686,7 @@ static int __init hp_wmi_rfkill_setup(struct platform_device *device)
 	int err, wireless;
 
 	wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
-	if (wireless)
+	if (wireless < 0)
 		return wireless;
 
 	err = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_WRITE, &wireless,
@@ -769,7 +772,7 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
 	err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state,
 				   0, sizeof(state));
 	if (err)
-		return err;
+		return err < 0 ? err : -EINVAL;
 
 	if (state.count > HPWMI_MAX_RFKILL2_DEVICES) {
 		pr_warn("unable to parse 0x1b query output\n");
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1626927 — [PATCH 5/9] platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Use the new hp_wmi_read_int() function and add a WARN_ONCE() to the TBD
regarding passing the error through. These are used in a null return
function unfortunately.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 758c229..e46b61c 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -337,33 +337,25 @@ static const struct rfkill_ops hp_wmi_rfkill_ops = {
 static bool hp_wmi_get_sw_state(enum hp_wmi_radio r)
 {
 	int mask = 0x200 << (r * 8);
-	int wireless = 0;
 
-	hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ,
-			     &wireless, sizeof(wireless),
-			     sizeof(wireless));
+	int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
+
 	/* TBD: Pass error */
+	WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY");
 
-	if (wireless & mask)
-		return false;
-	else
-		return true;
+	return !(wireless & mask);
 }
 
 static bool hp_wmi_get_hw_state(enum hp_wmi_radio r)
 {
 	int mask = 0x800 << (r * 8);
-	int wireless = 0;
 
-	hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ,
-			     &wireless, sizeof(wireless),
-			     sizeof(wireless));
+	int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY);
+
 	/* TBD: Pass error */
+	WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY");
 
-	if (wireless & mask)
-		return false;
-	else
-		return true;
+	return !(wireless & mask);
 }
 
 static int hp_wmi_rfkill2_set_block(void *data, bool blocked)
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1626928 — [PATCH 8/9] platform/x86: hp-wmi: Do not shadow errors in sysfs show functions

From: "Darren Hart (VMware)" <dvhart@infradead.org>

The new hp_wmi_read_int function returns a negative value in case of
error, pass this on directly rather than always replacing it with
-EINVAL.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index ccacd1a..90b8652 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -402,7 +402,7 @@ static ssize_t display_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_read_int(HPWMI_DISPLAY_QUERY);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -411,7 +411,7 @@ static ssize_t hddtemp_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -420,7 +420,7 @@ static ssize_t als_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_read_int(HPWMI_ALS_QUERY);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -429,7 +429,7 @@ static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_hw_state(HPWMI_DOCK_MASK);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -438,7 +438,7 @@ static ssize_t tablet_show(struct device *dev, struct device_attribute *attr,
 {
 	int value = hp_wmi_hw_state(HPWMI_TABLET_MASK);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -448,7 +448,7 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
 	/* Get the POST error code of previous boot failure. */
 	int value = hp_wmi_read_int(HPWMI_POSTCODEERROR_QUERY);
 	if (value < 0)
-		return -EINVAL;
+		return value;
 	return sprintf(buf, "0x%x\n", value);
 }
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1626929 — [PATCH 7/9] platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros

From: "Darren Hart (VMware)" <dvhart@infradead.org>

Use the DEVICE_ATTR_(RO|RW) macros, ranaming the show and store
functions accordingly.

Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/hp-wmi.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 89d6278..ccacd1a 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -397,7 +397,7 @@ static int hp_wmi_rfkill2_refresh(void)
 	return 0;
 }
 
-static ssize_t show_display(struct device *dev, struct device_attribute *attr,
+static ssize_t display_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
 	int value = hp_wmi_read_int(HPWMI_DISPLAY_QUERY);
@@ -406,7 +406,7 @@ static ssize_t show_display(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_hddtemp(struct device *dev, struct device_attribute *attr,
+static ssize_t hddtemp_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
 	int value = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY);
@@ -415,7 +415,7 @@ static ssize_t show_hddtemp(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_als(struct device *dev, struct device_attribute *attr,
+static ssize_t als_show(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {
 	int value = hp_wmi_read_int(HPWMI_ALS_QUERY);
@@ -424,7 +424,7 @@ static ssize_t show_als(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_dock(struct device *dev, struct device_attribute *attr,
+static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
 	int value = hp_wmi_hw_state(HPWMI_DOCK_MASK);
@@ -433,8 +433,8 @@ static ssize_t show_dock(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_tablet(struct device *dev, struct device_attribute *attr,
-			 char *buf)
+static ssize_t tablet_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
 {
 	int value = hp_wmi_hw_state(HPWMI_TABLET_MASK);
 	if (value < 0)
@@ -442,8 +442,8 @@ static ssize_t show_tablet(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", value);
 }
 
-static ssize_t show_postcode(struct device *dev, struct device_attribute *attr,
-			 char *buf)
+static ssize_t postcode_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
 {
 	/* Get the POST error code of previous boot failure. */
 	int value = hp_wmi_read_int(HPWMI_POSTCODEERROR_QUERY);
@@ -452,8 +452,8 @@ static ssize_t show_postcode(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "0x%x\n", value);
 }
 
-static ssize_t set_als(struct device *dev, struct device_attribute *attr,
-		       const char *buf, size_t count)
+static ssize_t als_store(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
 {
 	u32 tmp = simple_strtoul(buf, NULL, 10);
 	int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_WRITE, &tmp,
@@ -464,8 +464,8 @@ static ssize_t set_als(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static ssize_t set_postcode(struct device *dev, struct device_attribute *attr,
-		       const char *buf, size_t count)
+static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
 {
 	long unsigned int tmp2;
 	int ret;
@@ -485,12 +485,12 @@ static ssize_t set_postcode(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static DEVICE_ATTR(display, S_IRUGO, show_display, NULL);
-static DEVICE_ATTR(hddtemp, S_IRUGO, show_hddtemp, NULL);
-static DEVICE_ATTR(als, S_IRUGO | S_IWUSR, show_als, set_als);
-static DEVICE_ATTR(dock, S_IRUGO, show_dock, NULL);
-static DEVICE_ATTR(tablet, S_IRUGO, show_tablet, NULL);
-static DEVICE_ATTR(postcode, S_IRUGO | S_IWUSR, show_postcode, set_postcode);
+static DEVICE_ATTR_RO(display);
+static DEVICE_ATTR_RO(hddtemp);
+static DEVICE_ATTR_RW(als);
+static DEVICE_ATTR_RO(dock);
+static DEVICE_ATTR_RO(tablet);
+static DEVICE_ATTR_RW(postcode);
 
 static void hp_wmi_notify(u32 value, void *context)
 {
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1627167

On Thu, Apr 20, 2017 at 5:25 AM, Darren Hart <dvhart@infradead.org> wrote:
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
>
> This series factors out some redundant code, cleans up a number of style issues,
> modernizes the sysfs usage, and cleans up the return paths. All told, the driver
> is reduced in size by 37 lines (3.6%).
>
> I do not have an HP laptop, so I'm hoping Carlo can help out with some testing.
> In particular we need to verify that hotkeys and sysfs continue to work as
> before.
>

Series looks good to me except patch 2. So,

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

with above exception.

> This series is also available here for convenience:
> git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git hp-wmi
>
>
> Darren Hart (VMware) (9):
>   platform/x86: hp-wmi: Cleanup local variable declarations
>   platform/x86: hp-wmi: Add bios_args initializer
>   platform/x86: hp-wmi: Standardize enum usage for constants
>   platform/x86: hp-wmi: Refactor redundant HPWMI_READ functions
>   platform/x86: hp-wmi: Cleanup wireless get_(hw|sw)state functions
>   platform/x86: hp-wmi: Refactor dock and tablet state fetchers
>   platform/x86: hp-wmi: Use DEVICE_ATTR_(RO|RW) helper macros
>   platform/x86: hp-wmi: Do not shadow errors in sysfs show functions
>   platform/x86: hp-wmi: Cleanup exit paths
>
>  drivers/platform/x86/hp-wmi.c | 385 +++++++++++++++++++-----------------------
>  1 file changed, 174 insertions(+), 211 deletions(-)
>
> --
> 2.9.3
>



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1627788 — Re: [PATCH v1 0/9] platform/x86: hp-wmi: Driver refactoring and cleanups

On Thu, Apr 20, 2017 at 10:38:56AM +0300, Andy Shevchenko wrote:
> On Thu, Apr 20, 2017 at 5:25 AM, Darren Hart <dvhart@infradead.org> wrote:
> > From: "Darren Hart (VMware)" <dvhart@infradead.org>
> >
> > This series factors out some redundant code, cleans up a number of style issues,
> > modernizes the sysfs usage, and cleans up the return paths. All told, the driver
> > is reduced in size by 37 lines (3.6%).
> >
> > I do not have an HP laptop, so I'm hoping Carlo can help out with some testing.
> > In particular we need to verify that hotkeys and sysfs continue to work as
> > before.
> >
> 
> Series looks good to me except patch 2. So,
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> with above exception.

I was on the fence with this one, which is why I separated it out. I'll drop it.
Thanks for the review.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1627227

On Thu, Apr 20, 2017 at 4:25 AM, Darren Hart <dvhart@infradead.org> wrote:
>
> From: "Darren Hart (VMware)" <dvhart@infradead.org>
>
> This series factors out some redundant code, cleans up a number of style issues,
> modernizes the sysfs usage, and cleans up the return paths. All told, the driver
> is reduced in size by 37 lines (3.6%).
>
> I do not have an HP laptop, so I'm hoping Carlo can help out with some testing.
> In particular we need to verify that hotkeys and sysfs continue to work as
> before.

On my HP 240 G5:

Tested-by: Carlo Caione <carlo@endlessm.com>

Cheers,

-- 
Carlo Caione

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web