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

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

[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 <giedriuswork@gmail.com>
---
 drivers/platform/x86/hp-wireless.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/x86/hp-wireless.c b/drivers/platform/x86/hp-wireless.c
index 415348f..4e4cc8b 100644
--- a/drivers/platform/x86/hp-wireless.c
+++ b/drivers/platform/x86/hp-wireless.c
@@ -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;
 }
 
-- 
2.1.3

Re: [PATCH] platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails

[Borislav Petkov]

On Sat, Nov 29, 2014 at 12:14:27AM +0200, Giedrius Statkevicius wrote:
> 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 <giedriuswork@gmail.com>
> ---
>  drivers/platform/x86/hp-wireless.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/platform/x86/hp-wireless.c b/drivers/platform/x86/hp-wireless.c
> index 415348f..4e4cc8b 100644
> --- a/drivers/platform/x86/hp-wireless.c
> +++ b/drivers/platform/x86/hp-wireless.c
> @@ -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.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails

[Giedrius Statkevicius]

On 2014.11.29 01:15, Borislav Petkov wrote:
> On Sat, Nov 29, 2014 at 12:14:27AM +0200, Giedrius Statkevicius wrote:
>> 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 <giedriuswork@gmail.com>
>> ---
>>  drivers/platform/x86/hp-wireless.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/platform/x86/hp-wireless.c b/drivers/platform/x86/hp-wireless.c
>> index 415348f..4e4cc8b 100644
>> --- a/drivers/platform/x86/hp-wireless.c
>> +++ b/drivers/platform/x86/hp-wireless.c
>> @@ -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.
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().

Re: [PATCH] platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails

[Borislav Petkov]

On Sat, Nov 29, 2014 at 01:48:31AM +0200, Giedrius Statkevicius wrote:
> On 2014.11.29 01:15, Borislav Petkov wrote:
> > On Sat, Nov 29, 2014 at 12:14:27AM +0200, Giedrius Statkevicius wrote:
> >> 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 <giedriuswork@gmail.com>
> >> ---
> >>  drivers/platform/x86/hp-wireless.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/hp-wireless.c b/drivers/platform/x86/hp-wireless.c
> >> index 415348f..4e4cc8b 100644
> >> --- a/drivers/platform/x86/hp-wireless.c
> >> +++ b/drivers/platform/x86/hp-wireless.c
> >> @@ -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.
> 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.
--

Re: [PATCH] platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails

[Darren Hart]

On Sat, Nov 29, 2014 at 01:04:17AM +0100, Borislav Petkov wrote:
> On Sat, Nov 29, 2014 at 01:48:31AM +0200, Giedrius Statkevicius wrote:
> > On 2014.11.29 01:15, Borislav Petkov wrote:
> > > On Sat, Nov 29, 2014 at 12:14:27AM +0200, Giedrius Statkevicius wrote:
> > >> 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 <giedriuswork@gmail.com>
> > >> ---
> > >>  drivers/platform/x86/hp-wireless.c | 3 +++
> > >>  1 file changed, 3 insertions(+)
> > >>
> > >> diff --git a/drivers/platform/x86/hp-wireless.c b/drivers/platform/x86/hp-wireless.c
> > >> index 415348f..4e4cc8b 100644
> > >> --- a/drivers/platform/x86/hp-wireless.c
> > >> +++ b/drivers/platform/x86/hp-wireless.c
> > >> @@ -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.

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.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails

[Giedrius Statkevicius]

On 2014.11.26 00:57, Darren Hart wrote:
> On Sat, Nov 29, 2014 at 01:04:17AM +0100, Borislav Petkov wrote:
>> On Sat, Nov 29, 2014 at 01:48:31AM +0200, Giedrius Statkevicius wrote:
>>> On 2014.11.29 01:15, Borislav Petkov wrote:
>>>> On Sat, Nov 29, 2014 at 12:14:27AM +0200, Giedrius Statkevicius wrote:
>>>>> 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 <giedriuswork@gmail.com>
>>>>> ---
>>>>>  drivers/platform/x86/hp-wireless.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/x86/hp-wireless.c b/drivers/platform/x86/hp-wireless.c
>>>>> index 415348f..4e4cc8b 100644
>>>>> --- a/drivers/platform/x86/hp-wireless.c
>>>>> +++ b/drivers/platform/x86/hp-wireless.c
>>>>> @@ -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.
> 
> 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.
> 
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

Re: [PATCH] platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails

[Darren Hart]

On Tue, Dec 02, 2014 at 06:15:32PM +0200, Giedrius Statkevicius wrote:
> On 2014.11.26 00:57, Darren Hart wrote:
> > On Sat, Nov 29, 2014 at 01:04:17AM +0100, Borislav Petkov wrote:
> >> On Sat, Nov 29, 2014 at 01:48:31AM +0200, Giedrius Statkevicius wrote:
> >>> On 2014.11.29 01:15, Borislav Petkov wrote:
> >>>> On Sat, Nov 29, 2014 at 12:14:27AM +0200, Giedrius Statkevicius wrote:
> >>>>> 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 <giedriuswork@gmail.com>
> >>>>> ---
> >>>>>  drivers/platform/x86/hp-wireless.c | 3 +++
> >>>>>  1 file changed, 3 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/platform/x86/hp-wireless.c b/drivers/platform/x86/hp-wireless.c
> >>>>> index 415348f..4e4cc8b 100644
> >>>>> --- a/drivers/platform/x86/hp-wireless.c
> >>>>> +++ b/drivers/platform/x86/hp-wireless.c
> >>>>> @@ -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.
> > 
> > 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.
> > 
> 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.

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

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails

[Alex Hung]

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.



On Sat, Nov 29, 2014 at 8:04 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Sat, Nov 29, 2014 at 01:48:31AM +0200, Giedrius Statkevicius wrote:
>> On 2014.11.29 01:15, Borislav Petkov wrote:
>> > On Sat, Nov 29, 2014 at 12:14:27AM +0200, Giedrius Statkevicius wrote:
>> >> 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 <giedriuswork@gmail.com>
>> >> ---
>> >>  drivers/platform/x86/hp-wireless.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/drivers/platform/x86/hp-wireless.c b/drivers/platform/x86/hp-wireless.c
>> >> index 415348f..4e4cc8b 100644
>> >> --- a/drivers/platform/x86/hp-wireless.c
>> >> +++ b/drivers/platform/x86/hp-wireless.c
>> >> @@ -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.
>> 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.
> --



-- 
Cheers,
Alex Hung