Re: acpitoshiba: remove dead code
> On 16 Mar 2020, at 09:49, Stefan Sperling wrote: > > 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? > > These values ultimately come from aml_val2int() which returns int64_t, > i.e. a signed value. This driver currently copies that value to a uint32_t, > shifts it, and this shifted value ends up being copied to an int, and then > again to a uint32_t, which is then range-checked. > I don't understand why a range check is done only that late in the game, > after multiple integer type conversions that seem to be performed for no > good reason other than that the expected range will fit into an int. > > So an alternative fix could be to switch brightness values in this > driver to int64_t everywhere, i.e. in toshiba_get_brightness(), > toshiba_find_brightness(), toshiba_set_brightness(), etc. > Then you can keep all the code as it is. > > Or read an int64_t with aml_val2int(), shift and range-check it right away, > and only convert to a narrower type if the value is in the expected range. > Else error. Right, it seems that other drivers either use an uint64_t with associated shifts or int to match wsdisplay_params->curval. Also, it seems this passing around of ‘brightness’ could be avoided by making it a member of acpitoshiba_softc; same goes for 'video_output'.
Re: acpitoshiba: remove dead code
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? These values ultimately come from aml_val2int() which returns int64_t, i.e. a signed value. This driver currently copies that value to a uint32_t, shifts it, and this shifted value ends up being copied to an int, and then again to a uint32_t, which is then range-checked. I don't understand why a range check is done only that late in the game, after multiple integer type conversions that seem to be performed for no good reason other than that the expected range will fit into an int. So an alternative fix could be to switch brightness values in this driver to int64_t everywhere, i.e. in toshiba_get_brightness(), toshiba_find_brightness(), toshiba_set_brightness(), etc. Then you can keep all the code as it is. Or read an int64_t with aml_val2int(), shift and range-check it right away, and only convert to a narrower type if the value is in the expected range. Else error. > 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.c13 Oct 2019 10:56:31 - 1.12 > +++ acpi/acpitoshiba.c11 Mar 2020 11:35:23 - > @@ -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 > >
Re: acpitoshiba: remove dead code
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.c13 Oct 2019 10:56:31 - 1.12
> +++ acpi/acpitoshiba.c11 Mar 2020 11:35:23 -
> @@ -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
acpitoshiba: remove dead code
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? 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 - 1.12 +++ acpi/acpitoshiba.c 11 Mar 2020 11:35:23 - @@ -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
