Re: [PATCH v2 35/35] sh: mach-x3proto: rework ilsel_enable()

[Geert Uytterhoeven]

On Sun, Dec 3, 2023 at 8:34 PM Yury Norov <yury.norov@xxxxxxxxx> wrote:
> Fix opencoded find_and_set_bit(), which also suppresses potential
> KCSAN warning.
>
> CC: John Paul Adrian Glaubitz <glaubitz@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Yury Norov <yury.norov@xxxxxxxxx>

Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

> --- a/arch/sh/boards/mach-x3proto/ilsel.c
> +++ b/arch/sh/boards/mach-x3proto/ilsel.c
> @@ -99,8 +99,8 @@ int ilsel_enable(ilsel_source_t set)
>         }
>
>         do {
> -               bit = find_first_zero_bit(&ilsel_level_map, ILSEL_LEVELS);
> -       } while (test_and_set_bit(bit, &ilsel_level_map));
> +               bit = find_and_set_bit(&ilsel_level_map, ILSEL_LEVELS);
> +       } while (bit >= ILSEL_LEVELS);
>
>         __ilsel_enable(set, bit);

BTW, I don't think the old code worked as intended: the first time no
free bit is found, bit would have been ILSEL_LEVELS, and
test_and_set_bit() would have returned false, thus terminating the loop,
and continuing with an out-of-range bit value?  Hence to work correctly,
bit ILSEL_LEVELS of ilsel_level_map should have been initialized to one?
Or am I missing something?

The new code does not have that issue.

Anyway, this should probably never happen in real life.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds