linux-kernel - Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get

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: <CAGETcx-904Cr11nVW6PC=OqWnwM-ZC-DdEUa8+7JmhsH3UOqHw@mail.gmail.com>
Date:   Fri, 22 Jan 2021 12:58:24 -0800
From:   Saravana Kannan <saravanak@...gle.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Calvin Johnson <calvin.johnson@....nxp.com>,
        Grant Likely <grant.likely@....com>,
        Jeremy Linton <jeremy.linton@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Cristi Sovaiala <cristian.sovaiala@....com>,
        Florin Laurentiu Chiculita <florinlaurentiu.chiculita@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Pieter Jansen Van Vuuren <pieter.jansenvv@...boosystems.io>,
        Jon <jon@...id-run.com>,
        Diana Madalina Craciun <diana.craciun@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        Laurentiu Tudor <laurentiu.tudor@....com>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        "linux.cj" <linux.cj@...il.com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@...gle.com> wrote:
> >
> > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > >
> > > On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
> > > <andy.shevchenko@...il.com> wrote:
> > > >
> > > > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
> > > > > <andy.shevchenko@...il.com> wrote:
> > > > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> > > > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> > > > > > > <calvin.johnson@....nxp.com> wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > > > > > > > +       if (!(ret && is_acpi_node(fwnode)))
> > > > > > > > +               return ret;
> > > > > > > > +
> > > > > > > > +#ifdef CONFIG_ACPI
> > > > > > > > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > > > > > > > +                                      METHOD_NAME__ADR, NULL, &adr);
> > > > > > > > +       if (ACPI_FAILURE(status))
> > > > > > > > +               return -EINVAL;
> > > > > > > > +       *id = (u32)adr;
> > > > > > > > +#endif
> > > > > > > > +       return 0;
> > > >
> > > > > > > Also ACPI and DT
> > > > > > > aren't mutually exclusive if I'm not mistaken.
> > > > > >
> > > > > > That's why we try 'reg' property for both cases first.
> > > > > >
> > > > > > is_acpi_fwnode() conditional is that what I don't like though.
> > > > >
> > > > > I'm not sure what you mean here, care to elaborate?
> > > >
> > > > I meant is_acpi_node(fwnode) in the conditional.
> > > >
> > > > I think it's redundant and we can simple do something like this:
> > > >
> > > >   if (ret) {
> > > > #ifdef ACPI
> > > >     ...
> > > > #else
> > > >     return ret;
> > > > #endif
> > > >   }
> > > >   return 0;
> > > >
> > > > --
> > >
> > > Right, that should work.  And I'd prefer it too.
> >
> > Rafael,
> >
> > I'd rather this new function be an ops instead of a bunch of #ifdef or
> > if (acpi) checks. Thoughts?
>
> Well, it looks more like a helper function than like an op and I'm not
> even sure how many potential users of it will expect that _ADR should
> be evaluated in the absence of the "reg" property.
>
> It's just that the "reg" property happens to be kind of an _ADR
> equivalent in this particular binding AFAICS.

I agree it is not clear how useful this helper function is going to be.

But in general, to me, any time the wrapper/helper functions in
drivers/base/property.c need to do something like this:

if (ACPI)
   ACPI specific code
if (OF)
   OF specific code

I think the code should be pushed to the fwnode ops. That's one of the
main point of fwnode. So that firmware specific stuff is done by
firmware specific code. Also, when adding support for new firmware,
it's pretty clear what support the firmware needs to implement.
Instead of having to go fix up a bunch of code all over the place.

So fwnode_ops->get_id() would be the OP ACPI and OF would implement.
And then we can have a wrapper in drivers/base/property.c.

-Saravana