Re: [PATCH 14/19] sony-laptop: fix handling sony_nc_hotkeys_decode result

[Darren Hart <dvhart@infradead.org>]

On Thu, Sep 24, 2015 at 04:00:22PM +0200, Andrzej Hajda wrote:
> The function can return negative value.
> 
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/assign_signed_to_unsigned.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2046107
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Sorry for the delay Andrsej, and thank you for your patch. Given my delay, I've
made a couple of changes myself rather than asking you to resubmit. Please
review and let me know if you have any concerns.

First, The description above is incomplete and relies on context from the URL
to fully explain the problem you are fixing. In the future, please ensure the
commit message is self-sufficient.

I have changed the message to read:

    sony-laptop: Fix handling sony_nc_hotkeys_decode result

    sony_nv_hotkeys_decode can return a negative value. real_ev is a u32 variable.
    The check for real_ev > 0 is incorrect.

    Use an intermediate ret variable.

    The problem has been detected using proposed semantic patch
    scripts/coccinelle/tests/assign_signed_to_unsigned.cocci [1].

    [1]: http://permalink.gmane.org/gmane.linux.kernel/2046107

    Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
    [dvhart: clarify commit msg, drop superfluous else block]
    Signed-off-by: Darren Hart <dvhart@linux.intel.com>

See below for an additional change.

> ---
> Hi,
> 
> To avoid problems with too many mail recipients I have sent whole
> patchset only to LKML. Anyway patches have no dependencies.
> 
> Regards
> Andrzej
> ---
>  drivers/platform/x86/sony-laptop.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
> index aeb80d1..d8a2115 100644
> --- a/drivers/platform/x86/sony-laptop.c
> +++ b/drivers/platform/x86/sony-laptop.c
> @@ -1204,6 +1204,8 @@ static void sony_nc_notify(struct acpi_device *device, u32 event)
>  {
>  	u32 real_ev = event;
>  	u8 ev_type = 0;
> +	int ret;
> +
>  	dprintk("sony_nc_notify, event: 0x%.2x\n", event);
>  
>  	if (event >= 0x90) {
> @@ -1225,13 +1227,15 @@ static void sony_nc_notify(struct acpi_device *device, u32 event)
>  		case 0x0100:
>  		case 0x0127:
>  			ev_type = HOTKEY;
> -			real_ev = sony_nc_hotkeys_decode(event, handle);
> +			ret = sony_nc_hotkeys_decode(event, handle);
>  
> -			if (real_ev > 0)
> -				sony_laptop_report_input_event(real_ev);
> -			else
> +			if (ret > 0) {
> +				sony_laptop_report_input_event(ret);
> +				real_ev = ret;
> +			} else {
>  				/* restore the original event for reporting */
>  				real_ev = event;
> +			}

This 4 line else block is superfluous. real_ev is initialized to event and only changed here if ret > 0. Therefore, there is no need to set real_ev to event again. I have simply dropped the else block

-- 
Darren Hart
Intel Open Source Technology Center