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, ¤t_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