[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ddde9dca-6333-ba16-e97d-299fde06aaab@gmail.com>
Date: Thu, 26 Mar 2020 05:20:25 -0500
From: Frank Rowand <frowand.list@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Rob Herring <robh+dt@...nel.org>,
Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Alan Tull <atull@...nel.org>
Subject: Re: [PATCH 2/2] of: some unittest overlays not untracked
On 3/26/20 3:21 AM, Geert Uytterhoeven wrote:
> Hi Frank,
>
> On Thu, Mar 26, 2020 at 2:47 AM <frowand.list@...il.com> wrote:
>> From: Frank Rowand <frank.rowand@...y.com>
>>
>> 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@...el.com>
>> Signed-off-by: Frank Rowand <frank.rowand@...y.com>
>
> Looks good to me, so:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be>
>
> 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?
Yes, that would be a better location to validate the id. All of these
checks will go away when I get rid of the bitmap (see below).
>
>> + return overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id);
>
> No need for BIT_{WORD,MASK}() calculations if you would use test_bit().
I was trying to not get too carried away with cleaning up the tracking
code data structure in this patch. In general, I would say that using
a bitmap is an over optimization given the very small number of overlays
that are tracked. Long term I want to change it to a simpler form.
>
>> +}
>> +
>> 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.
The id is actually allocted in the drivers/of/overlay.c via idr.
Thanks for the thougthful review.
-Frank
>
>> 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
>
Powered by blists - more mailing lists