| Message ID | 20250210-gpio-set-array-helper-v3-5-d6a673674da8@baylibre.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | gpiolib: add gpiod_multi_set_value_cansleep | expand |
On Mon, Feb 10, 2025 at 04:33:31PM -0600, David Lechner wrote: > Use bitmap_get_value8() instead of accessing the bitmap directly. > > Accessing the bitmap directly is not considered good practice. We now > have a helper function that can be used instead, so let's use it. Thank you, LGTM (one minor thing you may address, or keep it as in the current variant of the patch), Reviewed-by: Andy Shevchenko <andy@kernel.org> ... > +#include <linux/bitmap.h> > #include <linux/bitops.h> bitmap.h implies bitops.h
On Mon, Feb 10, 2025 at 04:33:31PM -0600, David Lechner wrote: > Use bitmap_get_value8() instead of accessing the bitmap directly. > > Accessing the bitmap directly is not considered good practice. We now > have a helper function that can be used instead, so let's use it. > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> u> Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > drivers/bus/ts-nbus.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c > index b4c9308caf0647a3261071d9527fffce77784af2..beac67f3b820377f8bb1fc4f4ee77e15ee240834 100644 > --- a/drivers/bus/ts-nbus.c > +++ b/drivers/bus/ts-nbus.c > @@ -10,6 +10,7 @@ > * TS-4600 SoM. > */ > > +#include <linux/bitmap.h> > #include <linux/bitops.h> > #include <linux/gpio/consumer.h> > #include <linux/kernel.h> > @@ -107,7 +108,7 @@ static void ts_nbus_reset_bus(struct ts_nbus *ts_nbus) > { > DECLARE_BITMAP(values, 8); > > - values[0] = 0; > + bitmap_set_value8(values, byte, 0); Hi David, byte doesn't appear to exist in the scope of this function. I tried this: bitmap_set_value8(values, 0, 8); But when compiling with GCC 14.2.0 I see warnings that values is used uninitialised - bitmap_set_value8() appears to rely on it being so. CC drivers/bus/ts-nbus.o In file included from drivers/bus/ts-nbus.c:13: In function ‘bitmap_write’, inlined from ‘ts_nbus_reset_bus’ at drivers/bus/ts-nbus.c:111:2: ./include/linux/bitmap.h:818:12: error: ‘values’ is used uninitialized [-Werror=uninitialized] 818 | map[index] &= (fit ? (~(mask << offset)) : ~BITMAP_FIRST_WORD_MASK(start)); | ~~~^~~~~~~ In file included from ./include/linux/kasan-checks.h:5, from ./include/asm-generic/rwonce.h:26, from ./arch/x86/include/generated/asm/rwonce.h:1, from ./include/linux/compiler.h:344, from ./include/linux/build_bug.h:5, from ./include/linux/bits.h:22, from ./include/linux/bitops.h:6, from ./include/linux/bitmap.h:8: drivers/bus/ts-nbus.c: In function ‘ts_nbus_reset_bus’: drivers/bus/ts-nbus.c:109:24: note: ‘values’ declared here 109 | DECLARE_BITMAP(values, 8); | ^~~~~~ ./include/linux/types.h:11:23: note: in definition of macro ‘DECLARE_BITMAP’ 11 | unsigned long name[BITS_TO_LONGS(bits)] | ^~~~ > > gpiod_multi_set_value_cansleep(ts_nbus->data, values); > gpiod_set_value_cansleep(ts_nbus->csn, 0); > @@ -151,7 +152,7 @@ static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte) > { > DECLARE_BITMAP(values, 8); > > - values[0] = byte; > + bitmap_set_value8(values, byte, 8); > > gpiod_multi_set_value_cansleep(ts_nbus->data, values); > } > > -- > 2.43.0 >
On Thu, Feb 20, 2025 at 12:17 PM Simon Horman <horms@kernel.org> wrote: > On Mon, Feb 10, 2025 at 04:33:31PM -0600, David Lechner wrote: ... > But when compiling with GCC 14.2.0 I see warnings that values > is used uninitialised - bitmap_set_value8() appears to rely on > it being so. > In file included from drivers/bus/ts-nbus.c:13: > In function ‘bitmap_write’, > inlined from ‘ts_nbus_reset_bus’ at drivers/bus/ts-nbus.c:111:2: > ./include/linux/bitmap.h:818:12: error: ‘values’ is used uninitialized [-Werror=uninitialized] > 818 | map[index] &= (fit ? (~(mask << offset)) : ~BITMAP_FIRST_WORD_MASK(start)); > | ~~~^~~~~~~ Heh, the compiler is dumb. Even if it's not initialised we do not care. ... Wondering if the bitmap_write() will work better...
On 2/20/25 6:06 AM, Andy Shevchenko wrote: > On Thu, Feb 20, 2025 at 12:17 PM Simon Horman <horms@kernel.org> wrote: >> On Mon, Feb 10, 2025 at 04:33:31PM -0600, David Lechner wrote: > > ... > >> But when compiling with GCC 14.2.0 I see warnings that values >> is used uninitialised - bitmap_set_value8() appears to rely on >> it being so. > >> In file included from drivers/bus/ts-nbus.c:13: >> In function ‘bitmap_write’, >> inlined from ‘ts_nbus_reset_bus’ at drivers/bus/ts-nbus.c:111:2: >> ./include/linux/bitmap.h:818:12: error: ‘values’ is used uninitialized [-Werror=uninitialized] >> 818 | map[index] &= (fit ? (~(mask << offset)) : ~BITMAP_FIRST_WORD_MASK(start)); >> | ~~~^~~~~~~ > > Heh, the compiler is dumb. Even if it's not initialised we do not care. > > ... > > Wondering if the bitmap_write() will work better... > It is already using that: #define bitmap_set_value8(map, value, start) \ bitmap_write(map, value, start, BITS_PER_BYTE)
On 2/20/25 4:17 AM, Simon Horman wrote: > On Mon, Feb 10, 2025 at 04:33:31PM -0600, David Lechner wrote: >> Use bitmap_get_value8() instead of accessing the bitmap directly. >> >> Accessing the bitmap directly is not considered good practice. We now >> have a helper function that can be used instead, so let's use it. >> >> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > u> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> drivers/bus/ts-nbus.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c >> index b4c9308caf0647a3261071d9527fffce77784af2..beac67f3b820377f8bb1fc4f4ee77e15ee240834 100644 >> --- a/drivers/bus/ts-nbus.c >> +++ b/drivers/bus/ts-nbus.c >> @@ -10,6 +10,7 @@ >> * TS-4600 SoM. >> */ >> >> +#include <linux/bitmap.h> >> #include <linux/bitops.h> >> #include <linux/gpio/consumer.h> >> #include <linux/kernel.h> >> @@ -107,7 +108,7 @@ static void ts_nbus_reset_bus(struct ts_nbus *ts_nbus) >> { >> DECLARE_BITMAP(values, 8); >> >> - values[0] = 0; >> + bitmap_set_value8(values, byte, 0); > > Hi David, > > byte doesn't appear to exist in the scope of this function. > > I tried this: > > bitmap_set_value8(values, 0, 8); > > But when compiling with GCC 14.2.0 I see warnings that values > is used uninitialised - bitmap_set_value8() appears to rely on > it being so. Ah yes, I see the problem (I don't think this driver compiles with allmodconfig so the compiler didn't catch it for me). > > CC drivers/bus/ts-nbus.o > In file included from drivers/bus/ts-nbus.c:13: > In function ‘bitmap_write’, > inlined from ‘ts_nbus_reset_bus’ at drivers/bus/ts-nbus.c:111:2: > ./include/linux/bitmap.h:818:12: error: ‘values’ is used uninitialized [-Werror=uninitialized] > 818 | map[index] &= (fit ? (~(mask << offset)) : ~BITMAP_FIRST_WORD_MASK(start)); > | ~~~^~~~~~~ > In file included from ./include/linux/kasan-checks.h:5, > from ./include/asm-generic/rwonce.h:26, > from ./arch/x86/include/generated/asm/rwonce.h:1, > from ./include/linux/compiler.h:344, > from ./include/linux/build_bug.h:5, > from ./include/linux/bits.h:22, > from ./include/linux/bitops.h:6, > from ./include/linux/bitmap.h:8: > drivers/bus/ts-nbus.c: In function ‘ts_nbus_reset_bus’: > drivers/bus/ts-nbus.c:109:24: note: ‘values’ declared here > 109 | DECLARE_BITMAP(values, 8); > | ^~~~~~ > ./include/linux/types.h:11:23: note: in definition of macro ‘DECLARE_BITMAP’ > 11 | unsigned long name[BITS_TO_LONGS(bits)] > | ^~~~ > > >> >> gpiod_multi_set_value_cansleep(ts_nbus->data, values); >> gpiod_set_value_cansleep(ts_nbus->csn, 0); >> @@ -151,7 +152,7 @@ static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte) >> { >> DECLARE_BITMAP(values, 8); We can fix by zero-initialing the bitmap. DECLARE_BITMAP(values, 8) = { }; Would you like me to send a new version of the patch? >> >> - values[0] = byte; >> + bitmap_set_value8(values, byte, 8); >> >> gpiod_multi_set_value_cansleep(ts_nbus->data, values); >> } >> >> -- >> 2.43.0 >>
On Thu, Feb 20, 2025 at 11:32:10AM -0600, David Lechner wrote: > On 2/20/25 4:17 AM, Simon Horman wrote: > > On Mon, Feb 10, 2025 at 04:33:31PM -0600, David Lechner wrote: > >> Use bitmap_get_value8() instead of accessing the bitmap directly. > >> > >> Accessing the bitmap directly is not considered good practice. We now > >> have a helper function that can be used instead, so let's use it. > >> > >> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > u> Signed-off-by: David Lechner <dlechner@baylibre.com> > >> --- > >> drivers/bus/ts-nbus.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c > >> index b4c9308caf0647a3261071d9527fffce77784af2..beac67f3b820377f8bb1fc4f4ee77e15ee240834 100644 > >> --- a/drivers/bus/ts-nbus.c > >> +++ b/drivers/bus/ts-nbus.c > >> @@ -10,6 +10,7 @@ > >> * TS-4600 SoM. > >> */ > >> > >> +#include <linux/bitmap.h> > >> #include <linux/bitops.h> > >> #include <linux/gpio/consumer.h> > >> #include <linux/kernel.h> > >> @@ -107,7 +108,7 @@ static void ts_nbus_reset_bus(struct ts_nbus *ts_nbus) > >> { > >> DECLARE_BITMAP(values, 8); > >> > >> - values[0] = 0; > >> + bitmap_set_value8(values, byte, 0); > > > > Hi David, > > > > byte doesn't appear to exist in the scope of this function. > > > > I tried this: > > > > bitmap_set_value8(values, 0, 8); > > > > But when compiling with GCC 14.2.0 I see warnings that values > > is used uninitialised - bitmap_set_value8() appears to rely on > > it being so. > > Ah yes, I see the problem (I don't think this driver compiles with > allmodconfig so the compiler didn't catch it for me). Thanks, that would explain things. FWIIW, I think you can exercise this with allmodconfig by simply running: make drivers/bus/ts-nbus.o > > > > > CC drivers/bus/ts-nbus.o > > In file included from drivers/bus/ts-nbus.c:13: > > In function ‘bitmap_write’, > > inlined from ‘ts_nbus_reset_bus’ at drivers/bus/ts-nbus.c:111:2: > > ./include/linux/bitmap.h:818:12: error: ‘values’ is used uninitialized [-Werror=uninitialized] > > 818 | map[index] &= (fit ? (~(mask << offset)) : ~BITMAP_FIRST_WORD_MASK(start)); > > | ~~~^~~~~~~ > > In file included from ./include/linux/kasan-checks.h:5, > > from ./include/asm-generic/rwonce.h:26, > > from ./arch/x86/include/generated/asm/rwonce.h:1, > > from ./include/linux/compiler.h:344, > > from ./include/linux/build_bug.h:5, > > from ./include/linux/bits.h:22, > > from ./include/linux/bitops.h:6, > > from ./include/linux/bitmap.h:8: > > drivers/bus/ts-nbus.c: In function ‘ts_nbus_reset_bus’: > > drivers/bus/ts-nbus.c:109:24: note: ‘values’ declared here > > 109 | DECLARE_BITMAP(values, 8); > > | ^~~~~~ > > ./include/linux/types.h:11:23: note: in definition of macro ‘DECLARE_BITMAP’ > > 11 | unsigned long name[BITS_TO_LONGS(bits)] > > | ^~~~ > > > > > >> > >> gpiod_multi_set_value_cansleep(ts_nbus->data, values); > >> gpiod_set_value_cansleep(ts_nbus->csn, 0); > >> @@ -151,7 +152,7 @@ static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte) > >> { > >> DECLARE_BITMAP(values, 8); > > We can fix by zero-initialing the bitmap. > > DECLARE_BITMAP(values, 8) = { }; Thanks, I confirmed that adding that to ts_nbus_reset_bus() makes the compiler happy. And it seems sensible to me. I guess that theoretically it should also be added to ts_nbus_write_byte(), although GCC has nothing to say about that either way. > Would you like me to send a new version of the patch? It's not really my call. But I would expect that is a good next step. > >> - values[0] = byte; > >> + bitmap_set_value8(values, byte, 8); > >> > >> gpiod_multi_set_value_cansleep(ts_nbus->data, values); > >> }
diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c index b4c9308caf0647a3261071d9527fffce77784af2..beac67f3b820377f8bb1fc4f4ee77e15ee240834 100644 --- a/drivers/bus/ts-nbus.c +++ b/drivers/bus/ts-nbus.c @@ -10,6 +10,7 @@ * TS-4600 SoM. */ +#include <linux/bitmap.h> #include <linux/bitops.h> #include <linux/gpio/consumer.h> #include <linux/kernel.h> @@ -107,7 +108,7 @@ static void ts_nbus_reset_bus(struct ts_nbus *ts_nbus) { DECLARE_BITMAP(values, 8); - values[0] = 0; + bitmap_set_value8(values, byte, 0); gpiod_multi_set_value_cansleep(ts_nbus->data, values); gpiod_set_value_cansleep(ts_nbus->csn, 0); @@ -151,7 +152,7 @@ static void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte) { DECLARE_BITMAP(values, 8); - values[0] = byte; + bitmap_set_value8(values, byte, 8); gpiod_multi_set_value_cansleep(ts_nbus->data, values); }
Use bitmap_get_value8() instead of accessing the bitmap directly. Accessing the bitmap directly is not considered good practice. We now have a helper function that can be used instead, so let's use it. Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/bus/ts-nbus.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)