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@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.
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().
--
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/