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 platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
