linux-kernel - Re: [PATCH v1 1/3] pinctrl: intel: Replace ifdeffery by pm_sleep

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: <20240904111859.GE1532424@black.fi.intel.com>
Date: Wed, 4 Sep 2024 14:18:59 +0300
From: Mika Westerberg <mika.westerberg@...ux.intel.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
	Andy Shevchenko <andy@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v1 1/3] pinctrl: intel: Replace ifdeffery by
 pm_sleep_ptr() macro

On Wed, Sep 04, 2024 at 10:48:42AM +0300, Andy Shevchenko wrote:
> On Wed, Sep 4, 2024 at 10:47 AM Andy Shevchenko
> <andy.shevchenko@...il.com> wrote:
> > On Wed, Sep 4, 2024 at 8:05 AM Mika Westerberg
> > <mika.westerberg@...ux.intel.com> wrote:
> > > On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote:
> > > > Explicit ifdeffery is ugly and theoretically might be not synchronised
> > > > with the rest of functions that are assigned via pm_sleep_ptr() macro.
> > > > Replace ifdeffery by pm_sleep_ptr() macro to improve this.
> 
> ...
> 
> > > Can't we make this a stub when !PM_SLEEP?
> > >
> > > #ifdef CONFIG_PM_SLEEP
> > > static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> > > {
> > > ...
> > > }
> > > #else
> > > static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> > > {
> > >         return 0;
> > > }
> > > #endif
> >
> > There is no benefit. It's actually the opposite, i.e. it expands more ifdeffery.
> >
> > ...
> >
> > > > -     ret = intel_pinctrl_pm_init(pctrl);
> > > > +     ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0;
> > >
> > > Then this still looks like a function call and not like some weird
> > > conditional.
> >
> > I understand that, but the point is to make all PM callbacks use the
> > same approach against kernel configuration. Current state of affairs
> > is simple inconsistency, but it might, however quite unlikely, lead to
> > desynchronization between two pm_sleep_ptr() and ifdeffery approaches.
> >
> > Approach that I have before this one (and I kinda agree that ternary
> > here looks a bit weird) is to typedef the function and do something
> > like
> >
> > pinctrl-intel.h:
> 
> > typedef alloc_fn;
> 
> Actually typedef is not needed as it may be embedded in the below
> inline as it's used only once.
> 
> > static inline int ctx_alloc(pctrl, alloc_fn)
> > {
> >   if (alloc_fn)
> >     return alloc_fn(pctrl);
> >
> >   return 0;
> > }
> >
> > pinctrl-intel.c:
> >
> >   ret = ctx_alloc(pctrl, pm_sleep_ptr(_pm_init))
> >   if (ret)
> >     return ret;

I don't think this makes it any better :( We want the driver to be
readable for anyone, not just for you.

I prefer the stub and ifdeffery.