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] [day] [month] [year] [list]

Message-ID:
 <OSBPR01MB27755C4089DF9546954E2422FF052@OSBPR01MB2775.jpnprd01.prod.outlook.com>
Date: Wed, 18 Dec 2024 10:50:14 +0000
From: John Madieu <john.madieu.xa@...renesas.com>
To: Rob Herring <robh@...nel.org>
CC: "arnd@...db.de" <arnd@...db.de>, "heiko@...ech.de" <heiko@...ech.de>,
	"krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
	"lee@...nel.org" <lee@...nel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "liviu.dudau@....com" <liviu.dudau@....com>,
	"lpieralisi@...nel.org" <lpieralisi@...nel.org>, "pankaj.dubey@...sung.com"
	<pankaj.dubey@...sung.com>, "peter.griffin@...aro.org"
	<peter.griffin@...aro.org>, "sudeep.holla@....com" <sudeep.holla@....com>,
	"willmcvicker@...gle.com" <willmcvicker@...gle.com>, Biju Das
	<biju.das.jz@...renesas.com>, Fabrizio Castro
	<fabrizio.castro.jz@...esas.com>
Subject: RE: [PATCH 3/3] mfd: syscon: Allow syscon nodes without a "syscon"
 compatible

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@...nel.org>
> Sent: Monday, December 16, 2024 6:40 PM
> 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 <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?

Did not see Will's answer while doing my tests. I however tested it and it
works. I did also test your v2 series, which worked with me as well.

I'll then drop syscon compatible string in my series and send the v2.

Thanks,
John

> 
> >
> > 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