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@xxxxxxxxx>
> >>>>> ---
> >>>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/