linux-kernel - Re: [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL_JsqJRQ0VFLHJYo6XHnUnyhtn3X6F2ZedsftnsviO_wOtcGw@mail.gmail.com>
Date:   Mon, 21 Aug 2023 07:24:43 -0500
From:   Rob Herring <robh@...nel.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Frank Rowand <frowand.list@...il.com>,
        "Enrico Weigelt, metux IT consult" <info@...ux.net>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Petr Mladek <pmladek@...e.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/6] of: dynamic: Move dead property list check into
 property add/update functions

On Mon, Aug 21, 2023 at 5:49 AM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Fri, Aug 18, 2023 at 03:41:00PM -0500, Rob Herring wrote:
> > The changeset code checks for a property in the deadprops list when
> > adding/updating a property, but of_add_property() and
> > of_update_property() do not. As the users of these functions are pretty
> > simple, they have not hit this scenario or else the property lists
> > would get corrupted.
> >
> > With this there are 3 cases of removing a property from either deadprops
> > or properties lists, so add a helper to find and remove a matching
> > property.
>
> ...
>
> > v3:
> >  - Keep existing style in deadprops loop
>
> Not sure where exactly in the code that one, but...

That was your previous comment...

>
> ...
>
> >  int __of_remove_property(struct device_node *np, struct property *prop)
> >  {
> > -     struct property **next;
> > -
> > -     for (next = &np->properties; *next; next = &(*next)->next) {
> > -             if (*next == prop)
> > -                     break;
> > +     if (__of_remove_property_from_list(&np->properties, prop)) {
> > +             /* Found the property, add it to deadprops list */
> > +             prop->next = np->deadprops;
> > +             np->deadprops = prop;
> > +             return 0;
> >       }
> > -     if (*next == NULL)
> > -             return -ENODEV;
> > -
> > -     /* found the node */
> > -     *next = prop->next;
> > -     prop->next = np->deadprops;
> > -     np->deadprops = prop;
> >
> > -     return 0;
> > +     return -ENODEV;
> >  }
>
>
> ...if it's this one, I don't see how it's better than
>
>         if (!__of_remove_property_from_list(&np->properties, prop))
>                 return -ENODEV;

Because this way doesn't work well when we move the spinlock in here.
Maybe cleanup.h will help, but I'm not going to do that now. If we do,
then I'll do it for the whole subsystem/file.

Rob