Re: acpitoshiba: remove dead code

[Claudio Jeker]

On Mon, Mar 16, 2020 at 09:29:43AM +0100, Jasper Lievisse Adriaanse wrote:
> Hi,
> 
> The type of brightness and video_output is uint32_t; therefore it
> can never be less than 0 (which is what HCI_LCD_BRIGHTNESS_MIN and
> HCI_VIDEO_OUTPUT_CYCLE_MIN are defined to). So trim the checks by
> removig the impossible cases.
> 
> Coverity CID 1453109, 1453169
> 
> OK?

I find such changes actually bad.  Currently the code is clear doing a
real range check, After the change you will wonder why
HCI_LCD_BRIGHTNESS_MIN is not in that check and need to start digging.
Now I do understand that the code right now is not correct since this is a
signed vs unsigned comparison but please look at more then just that bit.

e.g.
int
toshiba_find_brightness(struct acpitoshiba_softc *sc, int *new_blevel)
{
        int ret, current_blevel;

        ...
        ret = toshiba_set_brightness(sc, &current_blevel);
}

So this function is passing a signed int to a function expecting unsinged
int

int
toshiba_fn_key_brightness_down(struct acpitoshiba_softc *sc)
{
        uint32_t brightness_level;

        ...
                if (brightness_level-- == HCI_LCD_BRIGHTNESS_MIN)
                        brightness_level = HCI_LCD_BRIGHTNESS_MIN;
                else
                        ret = toshiba_set_brightness(sc, &brightness_level);
}

This code is also strange, especially since brightness_level is set but
not used again. I think this driver needs a bit more cleanup.
 
> Index: acpi/acpitoshiba.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/acpitoshiba.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 acpitoshiba.c
> --- acpi/acpitoshiba.c        13 Oct 2019 10:56:31 -0000      1.12
> +++ acpi/acpitoshiba.c        11 Mar 2020 11:35:23 -0000
> @@ -438,9 +438,8 @@ toshiba_set_brightness(struct acpitoshib
>       for (i = 0; i < HCI_WORDS; ++i)
>               args[i].type = AML_OBJTYPE_INTEGER;
>  
> -     if ((*brightness < HCI_LCD_BRIGHTNESS_MIN) ||
> -         (*brightness > HCI_LCD_BRIGHTNESS_MAX))
> -                    return (HCI_FAILURE);
> +     if (*brightness > HCI_LCD_BRIGHTNESS_MAX)
> +             return (HCI_FAILURE);
>  
>       *brightness <<= HCI_LCD_BRIGHTNESS_SHIFT;
>  
> @@ -534,8 +533,7 @@ toshiba_set_video_output(struct acpitosh
>  
>       bzero(args, sizeof(args));
>  
> -     if ((*video_output < HCI_VIDEO_OUTPUT_CYCLE_MIN) ||
> -         (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX))
> +     if (*video_output > HCI_VIDEO_OUTPUT_CYCLE_MAX)
>               return (HCI_FAILURE);
>  
>       *video_output |= HCI_VIDEO_OUTPUT_FLAG;
> -- 
> jasper
> 

-- 
:wq Claudio