platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails

PATCH In review Giedrius Statkevicius 5 participants 8 messages Nov 28, 2014 – Dec 2, 2014 x86 platform drivers

Share & tools

Giedrius Statkevicius

author

2014-11-28 22:15 UTC #0 lore raw

PATCH 1/1 platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails

In hpwl_add() there is a unused variable err to which we assign the result of hp_wireless_input_setup() but we don't do anything depending on the result so print out a message that informs the user if add() (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't print anything in this case.

Signed-off-by Giedrius Statkevicius <[email protected]>

---
 drivers/platform/x86/hp-wireless.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/platform/x86/hp-wireless.c +3 -0

index 415348f..4e4cc8b 100644
@@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
 	int err;
 
 	err = hp_wireless_input_setup();
+	if (err)
+		pr_err("Failed to setup hp wireless hotkeys\n");
+
 	return err;
 }

Signature · 1 line

2.1.3

Borislav Petkov

2014-11-28 23:15 UTC #1 lore raw

Quoted 16 lines · Giedrius Statkevicius

In hpwl_add() there is a unused variable err to which we assign the result of hp_wireless_input_setup() but we don't do anything depending on the result so print out a message that informs the user if add() (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't print anything in this case.
Signed-off-by Giedrius Statkevicius <[email protected]>
---
 drivers/platform/x86/hp-wireless.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/platform/x86/hp-wireless.c +0 -0
index 415348f..4e4cc8b 100644

drivers/platform/x86/hp-wireless.c static int hpwl_add(struct acpi_device *device) · line 90

Quoted 6 lines

drivers/platform/x86/hp-wireless.c +3 -0

@@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
 	int err;
 
 	err = hp_wireless_input_setup();
+	if (err)
+		pr_err("Failed to setup hp wireless hotkeys\n");
+

I don't think there's need for that. Just do:

	return hp_wireless_input_setup();

and drop err completely.

The upper layer which calls the ->add() method should propagate the error properly.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Giedrius Statkevicius

author

2014-11-28 23:49 UTC #2 lore raw

Quoted 26 lines · Borislav Petkov

Quoted 16 lines · Giedrius Statkevicius
In hpwl_add() there is a unused variable err to which we assign the result of hp_wireless_input_setup() but we don't do anything depending on the result so print out a message that informs the user if add() (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't print anything in this case.
Signed-off-by Giedrius Statkevicius <[email protected]>
---
 drivers/platform/x86/hp-wireless.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/platform/x86/hp-wireless.c +0 -0
index 415348f..4e4cc8b 100644
drivers/platform/x86/hp-wireless.c static int hpwl_add(struct acpi_device *device) · line 90
Quoted 6 lines
drivers/platform/x86/hp-wireless.c +3 -0
@@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
 	int err;
 
 	err = hp_wireless_input_setup();
+	if (err)
+		pr_err("Failed to setup hp wireless hotkeys\n");
+
I don't think there's need for that. Just do:

Quoted 6 lines

	return hp_wireless_input_setup();
and drop err completely.
The upper layer which calls the ->add() method should propagate the error properly.

Looking at other platform drivers code some add() methods do give information to the user via pr_{err,warn,info} macros, some don't. My first patch to deal with this just removed the err variable like you have wrote but Darren Hart and Rafael J. Wysocki commented that maybe it should be better to inform the user about this error. That is why probably there was a variable for this in the first place but probably the original author just forgot to add a pr_err().

Borislav Petkov

2014-11-29 00:04 UTC #3 lore raw

Quoted 33 lines · Giedrius Statkevicius

Quoted 26 lines · Borislav Petkov
Quoted 16 lines · Giedrius Statkevicius
In hpwl_add() there is a unused variable err to which we assign the result of hp_wireless_input_setup() but we don't do anything depending on the result so print out a message that informs the user if add() (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't print anything in this case.
Signed-off-by Giedrius Statkevicius <[email protected]>
---
 drivers/platform/x86/hp-wireless.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/platform/x86/hp-wireless.c +0 -0
index 415348f..4e4cc8b 100644
drivers/platform/x86/hp-wireless.c static int hpwl_add(struct acpi_device *device) · line 90
Quoted 6 lines
drivers/platform/x86/hp-wireless.c +3 -0
@@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
 	int err;
 
 	err = hp_wireless_input_setup();
+	if (err)
+		pr_err("Failed to setup hp wireless hotkeys\n");
+
I don't think there's need for that. Just do:
Quoted 6 lines
	return hp_wireless_input_setup();
and drop err completely.
The upper layer which calls the ->add() method should propagate the error properly.

Quoted 7 lines

Looking at other platform drivers code some add() methods do give information to the user via pr_{err,warn,info} macros, some don't. My first patch to deal with this just removed the err variable like you have wrote but Darren Hart and Rafael J. Wysocki commented that maybe it should be better to inform the user about this error. That is why probably there was a variable for this in the first place but probably the original author just forgot to add a pr_err().

Ah, I see. Original author added to CC.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Alex Hung

2014-12-01 07:22 UTC #4 lore raw

Hi,

I think adding a pr_err message has a benefit - if kernel prints this message, a develop can quickly identify where it gets wrong by a string search on entire kernel source code.

Other than this, both solutions are great.

Quoted 50 lines · Borislav Petkov

Quoted 33 lines · Giedrius Statkevicius
Quoted 26 lines · Borislav Petkov
Quoted 16 lines · Giedrius Statkevicius
In hpwl_add() there is a unused variable err to which we assign the result of hp_wireless_input_setup() but we don't do anything depending on the result so print out a message that informs the user if add() (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't print anything in this case.
Signed-off-by Giedrius Statkevicius <[email protected]>
---
 drivers/platform/x86/hp-wireless.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/platform/x86/hp-wireless.c +0 -0
index 415348f..4e4cc8b 100644
drivers/platform/x86/hp-wireless.c static int hpwl_add(struct acpi_device *device) · line 90
Quoted 6 lines
drivers/platform/x86/hp-wireless.c +3 -0
@@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
   int err;
 
   err = hp_wireless_input_setup();
+  if (err)
+          pr_err("Failed to setup hp wireless hotkeys\n");
+
I don't think there's need for that. Just do:
Quoted 6 lines
    return hp_wireless_input_setup();
and drop err completely.
The upper layer which calls the ->add() method should propagate the error properly.
Quoted 7 lines
Looking at other platform drivers code some add() methods do give information to the user via pr_{err,warn,info} macros, some don't. My first patch to deal with this just removed the err variable like you have wrote but Darren Hart and Rafael J. Wysocki commented that maybe it should be better to inform the user about this error. That is why probably there was a variable for this in the first place but probably the original author just forgot to add a pr_err().
Ah, I see. Original author added to CC.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine. --

Signature · 2 lines

Cheers,
Alex Hung

Darren Hart

2014-12-02 07:47 UTC #5 lore raw

In reply to Borislav Petkov

Quoted 28 lines · Borislav Petkov

Quoted 27 lines · Giedrius Statkevicius
Quoted 26 lines · Borislav Petkov
Quoted 16 lines · Giedrius Statkevicius
In hpwl_add() there is a unused variable err to which we assign the result of hp_wireless_input_setup() but we don't do anything depending on the result so print out a message that informs the user if add() (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't print anything in this case.
Signed-off-by Giedrius Statkevicius <[email protected]>
---
 drivers/platform/x86/hp-wireless.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/platform/x86/hp-wireless.c +0 -0
index 415348f..4e4cc8b 100644
drivers/platform/x86/hp-wireless.c static int hpwl_add(struct acpi_device *device) · line 90
Quoted 6 lines
drivers/platform/x86/hp-wireless.c +3 -0
@@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
 	int err;
 
 	err = hp_wireless_input_setup();
+	if (err)
+		pr_err("Failed to setup hp wireless hotkeys\n");
+
I don't think there's need for that. Just do:

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
	return hp_wireless_input_setup();
and drop err completely.
The upper layer which calls the ->add() method should propagate the error properly.

This is the key. If the caller already prints a message, we don't need to. If nothing is displayed and the user would be left confused, then an appropriate message should be displayed.

If the upper level already handles this, then we can just drop the unused variable.

Have a look and decide which is the most appropriate. And thanks for preparing a fix.

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Giedrius Statkevicius

2014-12-02 16:16 UTC #6 lore raw

Quoted 40 lines · Darren Hart

Quoted 28 lines · Borislav Petkov
Quoted 27 lines · Giedrius Statkevicius
Quoted 26 lines · Borislav Petkov
Quoted 16 lines · Giedrius Statkevicius
In hpwl_add() there is a unused variable err to which we assign the result of hp_wireless_input_setup() but we don't do anything depending on the result so print out a message that informs the user if add() (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't print anything in this case.
Signed-off-by Giedrius Statkevicius <[email protected]>
---
 drivers/platform/x86/hp-wireless.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/platform/x86/hp-wireless.c +0 -0
index 415348f..4e4cc8b 100644
drivers/platform/x86/hp-wireless.c static int hpwl_add(struct acpi_device *device) · line 90
Quoted 6 lines
drivers/platform/x86/hp-wireless.c +3 -0
@@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
 	int err;
 
 	err = hp_wireless_input_setup();
+	if (err)
+		pr_err("Failed to setup hp wireless hotkeys\n");
+
I don't think there's need for that. Just do:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
	return hp_wireless_input_setup();
and drop err completely.
The upper layer which calls the ->add() method should propagate the error properly.
This is the key. If the caller already prints a message, we don't need to. If nothing is displayed and the user would be left confused, then an appropriate message should be displayed.

Quoted 6 lines

If the upper level already handles this, then we can just drop the unused variable.
Have a look and decide which is the most appropriate. And thanks for preparing a fix.

To start with I've looked at all modules in drivers/platform/x86 that have a struct acpi_driver and calculated some statistics about whether the module informs the user if add() fails or not to find out the current policy on whether we should inform the user or not:

Module             Does it use any pr_/dev_ function to give info?
asus-laptop        Yes
classmate-laptop   No
dell-smo8800       Yes
dell-wireless      No
eeepc-laptop       Yes
fujitsu-laptop     Yes
hp-wireless        No
hp_accel           Yes
intel-rst          No
intel-smartconnect Yes
intel_menlow       No
panasonic-laptop   No (but it uses ACPI_DEBUG_PRINT)
pvpanic            No
sony-laptop        Yes (has a message for failing to setup input devices)
topstar-laptop     No
toshiba_acpi       Yes
toshiba_haps       Yes
toshiba_bluetooth  Yes
wmi                Yes
xo15-ebook         Yes

That being said it looks like most add()s inform the user. So maybe we should make a patch that does it for other modules with struct acpi_driver? That being said, I've also took a look at if any messages are printed out about add() failing.

There is acpi_device_probe() that's hooked into acpi_bus_type which defines names and some actions of a bus. There it simply returns any non-zero value:

990 ret = acpi_drv->ops.add(acpi_dev);
991 if (ret)
992 return ret;

Delving a bit deeper I've found about driver_probe_device() and really_probe(). And in these lines I think is what the user would get if the probe failed:

330 } else if (ret != -ENODEV && ret != -ENXIO) {
331 /* driver matched but the probe failed */
332 printk(KERN_WARNING
333 "%s: probe of %s failed with error %d\n",
334 drv->name, dev_name(dev), ret);

So the user would get only the numerical value of a error and thus a pr_err or printing any other message in the add() is useful to make more sense of what really happened and allow to quickly find where the issue happened. In the end I think that this patch is better than the last one (the one where add() simply returns the result of another function).

Please correct me if I'm wrong.

Thanks,
Giedrius

Darren Hart

2014-12-02 17:20 UTC #7 lore raw

Quoted 97 lines · Giedrius Statkevicius

Quoted 40 lines · Darren Hart
Quoted 28 lines · Borislav Petkov
Quoted 27 lines · Giedrius Statkevicius
Quoted 26 lines · Borislav Petkov
Quoted 16 lines · Giedrius Statkevicius
In hpwl_add() there is a unused variable err to which we assign the result of hp_wireless_input_setup() but we don't do anything depending on the result so print out a message that informs the user if add() (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't print anything in this case.
Signed-off-by Giedrius Statkevicius <[email protected]>
---
 drivers/platform/x86/hp-wireless.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/platform/x86/hp-wireless.c +0 -0
index 415348f..4e4cc8b 100644
drivers/platform/x86/hp-wireless.c static int hpwl_add(struct acpi_device *device) · line 90
Quoted 6 lines
drivers/platform/x86/hp-wireless.c +3 -0
@@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
 	int err;
 
 	err = hp_wireless_input_setup();
+	if (err)
+		pr_err("Failed to setup hp wireless hotkeys\n");
+
I don't think there's need for that. Just do:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
	return hp_wireless_input_setup();
and drop err completely.
The upper layer which calls the ->add() method should propagate the error properly.
This is the key. If the caller already prints a message, we don't need to. If nothing is displayed and the user would be left confused, then an appropriate message should be displayed.
Quoted 6 lines
If the upper level already handles this, then we can just drop the unused variable.
Have a look and decide which is the most appropriate. And thanks for preparing a fix.
To start with I've looked at all modules in drivers/platform/x86 that have a struct acpi_driver and calculated some statistics about whether the module informs the user if add() fails or not to find out the current policy on whether we should inform the user or not:
Module             Does it use any pr_/dev_ function to give info?
asus-laptop        Yes
classmate-laptop   No
dell-smo8800       Yes
dell-wireless      No
eeepc-laptop       Yes
fujitsu-laptop     Yes
hp-wireless        No
hp_accel           Yes
intel-rst          No
intel-smartconnect Yes
intel_menlow       No
panasonic-laptop   No (but it uses ACPI_DEBUG_PRINT)
pvpanic            No
sony-laptop        Yes (has a message for failing to setup input devices)
topstar-laptop     No
toshiba_acpi       Yes
toshiba_haps       Yes
toshiba_bluetooth  Yes
wmi                Yes
xo15-ebook         Yes
That being said it looks like most add()s inform the user. So maybe we should make a patch that does it for other modules with struct acpi_driver? That being said, I've also took a look at if any messages are printed out about add() failing.
There is acpi_device_probe() that's hooked into acpi_bus_type which defines names and some actions of a bus. There it simply returns any non-zero value:
990 ret = acpi_drv->ops.add(acpi_dev);
991 if (ret)
992 return ret;
Delving a bit deeper I've found about driver_probe_device() and really_probe(). And in these lines I think is what the user would get if the probe failed:
330 } else if (ret != -ENODEV && ret != -ENXIO) {
331 /* driver matched but the probe failed */
332 printk(KERN_WARNING
333 "%s: probe of %s failed with error %d\n",
334 drv->name, dev_name(dev), ret);

Quoted 7 lines

So the user would get only the numerical value of a error and thus a pr_err or printing any other message in the add() is useful to make more sense of what really happened and allow to quickly find where the issue happened. In the end I think that this patch is better than the last one (the one where add() simply returns the result of another function).
Please correct me if I'm wrong.

I'd agree. I'll queue it up, thanks for the detailed investigation Giedrius.

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center