Re: [PATCH 2/2] of: some unittest overlays not untracked

[Geert Uytterhoeven]

Hi Frank,

On Thu, Mar 26, 2020 at 2:47 AM <frowand.list@xxxxxxxxx> wrote:
> From: Frank Rowand <frank.rowand@xxxxxxxx>
>
> kernel test robot reported "WARNING: held lock freed!" triggered by
> unittest_gpio_remove(), which should not have been called because
> the related gpio overlay was not tracked.  Another overlay that
> was tracked had previously used the same id as the gpio overlay
> but had not been untracked when the overlay was removed.  Thus the
> clean up function of_unittest_destroy_tracked_overlays() incorrectly
> attempted to remove the reused overlay id.
>
> Patch contents:
>
>   - Create tracking related helper functions
>   - Change BUG() to WARN_ON() for overlay id related issues
>   - Add some additional error checking for valid overlay id values
>   - Add the missing overlay untrack
>   - update comment on expectation that overlay ids are assigned in
>     sequence
>
> Fixes: 492a22aceb75 ("of: unittest: overlay: Keep track of created overlays")
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>

Looks good to me, so:
Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

Still, a few suggestions for future improvement below...

> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -1689,19 +1689,27 @@ static const char *overlay_name_from_nr(int nr)
>
>  static const char *bus_path = "/testcase-data/overlay-node/test-bus";
>
> -/* it is guaranteed that overlay ids are assigned in sequence */
> +/* FIXME: it is NOT guaranteed that overlay ids are assigned in sequence */
> +
>  #define MAX_UNITTEST_OVERLAYS  256
>  static unsigned long overlay_id_bits[BITS_TO_LONGS(MAX_UNITTEST_OVERLAYS)];

Obviously this should have used DECLARE_BITMAP() ;-)

>  static int overlay_first_id = -1;
>
> +static long of_unittest_overlay_tracked(int id)
> +{
> +       if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
> +               return 0;

Do we need all these checks on id? Can this really happen?
I guess doing it once in of_unittest_track_overlay(), and aborting all
of_unittests if it triggers should be sufficient?

> +       return overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id);

No need for BIT_{WORD,MASK}() calculations if you would use test_bit().

> +}
> +
>  static void of_unittest_track_overlay(int id)
>  {
>         if (overlay_first_id < 0)
>                 overlay_first_id = id;
>         id -= overlay_first_id;
>
> -       /* we shouldn't need that many */
> -       BUG_ON(id >= MAX_UNITTEST_OVERLAYS);
> +       if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
> +               return;
>         overlay_id_bits[BIT_WORD(id)] |= BIT_MASK(id);

set_bit()

>  }
>
> @@ -1710,7 +1718,8 @@ static void of_unittest_untrack_overlay(int id)
>         if (overlay_first_id < 0)
>                 return;
>         id -= overlay_first_id;
> -       BUG_ON(id >= MAX_UNITTEST_OVERLAYS);
> +       if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
> +               return;
>         overlay_id_bits[BIT_WORD(id)] &= ~BIT_MASK(id);

clear_bit()

>  }
>
> @@ -1726,7 +1735,7 @@ static void of_unittest_destroy_tracked_overlays(void)
>                 defers = 0;
>                 /* remove in reverse order */

If it is not guaranteed that overlay ids are assigned in sequence, the
reverse order is not really needed, so you could replace the bitmap and
your own tracking mechanism by DEFINE_IDR() and idr_for_each()?
And as IDRs are flexible, MAX_UNITTEST_OVERLAYS and all checks
could be removed, too.

>                 for (id = MAX_UNITTEST_OVERLAYS - 1; id >= 0; id--) {
> -                       if (!(overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id)))
> +                       if (!of_unittest_overlay_tracked(id))
>                                 continue;
>
>                         ovcs_id = id + overlay_first_id;
> @@ -1743,7 +1752,7 @@ static void of_unittest_destroy_tracked_overlays(void)
>                                 continue;
>                         }
>
> -                       overlay_id_bits[BIT_WORD(id)] &= ~BIT_MASK(id);
> +                       of_unittest_untrack_overlay(id);
>                 }
>         } while (defers > 0);
>  }

Gr{oetje,eeting}s,

                        Geert

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

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