On Fri, Nov 11, 2022 at 06:48:21AM +0000, Miod Vallat wrote:
> > This actually breaks my machine. malloc() is saying allocation too
> > large. OF_getproplen will return -1 on that. Is it possible that
> > len is treated as uint64_t as it is an int and sizeof is effectively
> > uint64_t?
>
> Ah, yes; size_t is unsigned and wider than int on 64-bit platforms,
> therefore int is converted to unsigned for the comparison. Casting
> sizeof to int will do.
>
> > Might be easier to have a check like:
> >
> > if (sc->sc_channels == NULL)
> > return level < sc->sc_nlevels ? level : sc->sc_nlevels - 1;
> >
> > Then you don't need to indent the whole block. Makes the diff smaller
> > and a bit easier to understand?
>
> Sure; what about this new version, then?
Works for me, thanks! ok patrick@
>
> Index: pwmbl.c
> ===================================================================
> RCS file: /OpenBSD/src/sys/dev/fdt/pwmbl.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 pwmbl.c
> --- pwmbl.c 24 Oct 2021 17:52:26 -0000 1.6
> +++ pwmbl.c 11 Nov 2022 06:46:41 -0000
> @@ -35,7 +35,7 @@ struct pwmbl_softc {
> struct device sc_dev;
> uint32_t *sc_pwm;
> int sc_pwm_len;
> - uint32_t *sc_levels;
> + uint32_t *sc_levels; /* NULL if simple ramp */
> int sc_nlevels;
> uint32_t sc_max_level;
> uint32_t sc_def_level;
> @@ -73,7 +73,7 @@ pwmbl_attach(struct device *parent, stru
> struct pwmbl_softc *sc = (struct pwmbl_softc *)self;
> struct fdt_attach_args *faa = aux;
> uint32_t *gpios;
> - int i, len;
> + int len;
>
> len = OF_getproplen(faa->fa_node, "pwms");
> if (len < 0) {
> @@ -95,7 +95,7 @@ pwmbl_attach(struct device *parent, stru
> }
>
> len = OF_getproplen(faa->fa_node, "brightness-levels");
> - if (len > 0) {
> + if (len >= (int)sizeof(uint32_t)) {
> sc->sc_levels = malloc(len, M_DEVBUF, M_WAITOK);
> OF_getpropintarray(faa->fa_node, "brightness-levels",
> sc->sc_levels, len);
> @@ -107,13 +107,9 @@ pwmbl_attach(struct device *parent, stru
> sc->sc_def_level = sc->sc_nlevels - 1;
> sc->sc_def_level = sc->sc_levels[sc->sc_def_level];
> } else {
> + /* No levels, assume a simple 0..255 ramp. */
> sc->sc_nlevels = 256;
> - sc->sc_levels = mallocarray(sc->sc_nlevels,
> - sizeof(uint32_t), M_DEVBUF, M_WAITOK);
> - for (i = 0; i < sc->sc_nlevels; i++)
> - sc->sc_levels[i] = i;
> - sc->sc_max_level = sc->sc_levels[sc->sc_nlevels - 1];
> - sc->sc_def_level = sc->sc_levels[sc->sc_nlevels - 1];
> + sc->sc_max_level = sc->sc_def_level = sc->sc_nlevels - 1;
> }
>
> printf("\n");
> @@ -143,6 +139,9 @@ pwmbl_find_brightness(struct pwmbl_softc
> {
> uint32_t mid;
> int i;
> +
> + if (sc->sc_levels == NULL)
> + return level < sc->sc_nlevels ? level : sc->sc_nlevels - 1;
>
> for (i = 0; i < sc->sc_nlevels - 1; i++) {
> mid = (sc->sc_levels[i] + sc->sc_levels[i + 1]) / 2;
>
