linux-kernel - Re: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

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_Jsq+DEWibue5DVB=gSNB7cvaE85rV5XPf8HPqQDz0DgjCXw@mail.gmail.com>
Date: Mon, 16 Dec 2024 11:39:54 -0600
From: Rob Herring <robh@...nel.org>
To: John Madieu <john.madieu.xa@...renesas.com>
Cc: arnd@...db.de, heiko@...ech.de, krzysztof.kozlowski@...aro.org, 
	lee@...nel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, liviu.dudau@....com, lpieralisi@...nel.org, 
	pankaj.dubey@...sung.com, peter.griffin@...aro.org, sudeep.holla@....com, 
	willmcvicker@...gle.com, biju.das.jz@...renesas.com
Subject: Re: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

On Sun, Dec 15, 2024 at 2:34 PM John Madieu
<john.madieu.xa@...renesas.com> wrote:
>
> Hi Rob,
>
> On Wed, 11 Dec 2024 14:57:14 -0600 Rob Herring wrote:
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index bfb1f69fcff1..e6df2825c14d 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -171,8 +171,10 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
> >                       break;
> >               }
> >
> > -     if (!syscon)
> > +     if (!syscon && of_device_is_compatible(np, "syscon"))
> >               syscon = of_syscon_register(np, check_res);
> > +     else
> > +             syscon = ERR_PTR(-EINVAL);
>
> The current modification will make device_node_get_regmap() return -EINVAL even
> for syscons that were already found in the syscon_list, which I believe is not
> the intended behavior.

Yes, it is. Doesn't Will's fix work for you?

>
> I suggest modifying it this way to maintain lookup functionality for registered
> syscons while implementing your intended changes:
>
> static struct regmap *device_node_get_regmap(struct device_node *np,
>                                              bool check_res)
> {
>         struct syscon *entry, *syscon = NULL;
>         struct regmap *regmap;
>
>         mutex_lock(&syscon_list_lock);
>
>         list_for_each_entry(entry, &syscon_list, list)
>                 if (entry->np == np) {
>                         syscon = entry;
>                         break;
>                 }
>
>         if (syscon) {
>                 regmap = syscon->regmap;
>                 mut ix_unlock(&syscon_list_lock);
>                 return regmap;
>         }
>
>         if (of_device_is_compatible(np, "syscon")) {
>                 syscon = of_syscon_register(np, check_res);
>                 mutex_unlock(&syscon_list_lock);
>                 if (IS_ERR(syscon))
>                         return ERR_CAST(syscon);
>                 return syscon->regmap;
>         }
>
>         mutex_unlock(&syscon_list_lock);

3 unlock calls is a sign the code structure could be improved. A goto
or a guard() for example. However, I think this is the same logic as
what Will suggested.

Rob