Re: arm64 pwmbl(4): simplify ramp case
On Fri, Nov 11, 2022 at 06:48:21AM +, 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 - 1.6
> +++ pwmbl.c 11 Nov 2022 06:46:41 -
> @@ -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_tsc_max_level;
> uint32_tsc_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;
>
Re: arm64 pwmbl(4): simplify ramp case
> 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?
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 - 1.6
+++ pwmbl.c 11 Nov 2022 06:46:41 -
@@ -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_tsc_max_level;
uint32_tsc_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;
Re: arm64 pwmbl(4): simplify ramp case
On Mon, Jul 04, 2022 at 06:47:33PM +, Miod Vallat wrote:
> When the fdt does not provide a list of brightness states, pwmbl(4)
> builds a 256 state ramp (i.e. state[i] = i with 0 <= i < 256).
>
> The following diff keeps that behaviour, but gets rid of the malloc
> call for that ramp, since the values are trivially known.
>
> Compiles but not tested due to the lack of such hardware.
>
> Index: sys/dev/fdt/pwmbl.c
> ===
> RCS file: /OpenBSD/src/sys/dev/fdt/pwmbl.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 pwmbl.c
> --- sys/dev/fdt/pwmbl.c 24 Oct 2021 17:52:26 - 1.6
> +++ sys/dev/fdt/pwmbl.c 4 Jul 2022 18:45:16 -
> @@ -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_tsc_max_level;
> uint32_tsc_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 >= sizeof(uint32_t)) {
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?
Moving len to ssize_t doesn't fix it, but doing
if (len >= (int)sizeof(uint32_t)) {
works. So I wonder if
if (len > 0 && len >= sizeof(uint32_t)) {
would work as well. Or maybe let's just keep it as it is?
> 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");
> @@ -144,17 +140,22 @@ pwmbl_find_brightness(struct pwmbl_softc
> uint32_t mid;
> int i;
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?
Cheers,
Patrick
>
> - for (i = 0; i < sc->sc_nlevels - 1; i++) {
> - mid = (sc->sc_levels[i] + sc->sc_levels[i + 1]) / 2;
> - if (sc->sc_levels[i] <= level && level <= mid)
> + if (sc->sc_levels) {
> + for (i = 0; i < sc->sc_nlevels - 1; i++) {
> + mid = (sc->sc_levels[i] + sc->sc_levels[i + 1]) / 2;
> + if (sc->sc_levels[i] <= level && level <= mid)
> + return sc->sc_levels[i];
> + if (mid < level && level <= sc->sc_levels[i + 1])
> + return sc->sc_levels[i + 1];
> + }
> + if (level < sc->sc_levels[0])
> + return sc->sc_levels[0];
> + else
> return sc->sc_levels[i];
> - if (mid < level && level <= sc->sc_levels[i + 1])
> - return sc->sc_levels[i + 1];
> +
> + } else {
> + return level < sc->sc_nlevels ? level : sc->sc_nlevels - 1;
> }
> - if (level < sc->sc_levels[0])
> - return sc->sc_levels[0];
> - else
> - return sc->sc_levels[i];
> }
>
> int
>
arm64 pwmbl(4): simplify ramp case
When the fdt does not provide a list of brightness states, pwmbl(4)
builds a 256 state ramp (i.e. state[i] = i with 0 <= i < 256).
The following diff keeps that behaviour, but gets rid of the malloc
call for that ramp, since the values are trivially known.
Compiles but not tested due to the lack of such hardware.
Index: sys/dev/fdt/pwmbl.c
===
RCS file: /OpenBSD/src/sys/dev/fdt/pwmbl.c,v
retrieving revision 1.6
diff -u -p -r1.6 pwmbl.c
--- sys/dev/fdt/pwmbl.c 24 Oct 2021 17:52:26 - 1.6
+++ sys/dev/fdt/pwmbl.c 4 Jul 2022 18:45:16 -
@@ -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_tsc_max_level;
uint32_tsc_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 >= 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");
@@ -144,17 +140,22 @@ pwmbl_find_brightness(struct pwmbl_softc
uint32_t mid;
int i;
- for (i = 0; i < sc->sc_nlevels - 1; i++) {
- mid = (sc->sc_levels[i] + sc->sc_levels[i + 1]) / 2;
- if (sc->sc_levels[i] <= level && level <= mid)
+ if (sc->sc_levels) {
+ for (i = 0; i < sc->sc_nlevels - 1; i++) {
+ mid = (sc->sc_levels[i] + sc->sc_levels[i + 1]) / 2;
+ if (sc->sc_levels[i] <= level && level <= mid)
+ return sc->sc_levels[i];
+ if (mid < level && level <= sc->sc_levels[i + 1])
+ return sc->sc_levels[i + 1];
+ }
+ if (level < sc->sc_levels[0])
+ return sc->sc_levels[0];
+ else
return sc->sc_levels[i];
- if (mid < level && level <= sc->sc_levels[i + 1])
- return sc->sc_levels[i + 1];
+
+ } else {
+ return level < sc->sc_nlevels ? level : sc->sc_nlevels - 1;
}
- if (level < sc->sc_levels[0])
- return sc->sc_levels[0];
- else
- return sc->sc_levels[i];
}
int
