Message-ID: <CAK8P3a2CBYV2xEkedQYmzL4XgHPeu02=vmLffq+RWwvEvuUGKQ@mail.gmail.com>
Date:   Mon, 27 Jul 2020 12:44:03 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Anson Huang <anson.huang@....com>
Cc:     Russell King - ARM Linux <linux@...linux.org.uk>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Sascha Hauer <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Peter Chen <peter.chen@....com>,
        "oleksandr.suvorov@...adex.com" <oleksandr.suvorov@...adex.com>,
        Andreas Kemnade <andreas@...nade.info>,
        Peng Fan <peng.fan@....com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Olof Johansson <olof@...om.net>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Patrice Chotard <patrice.chotard@...com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Joel Stanley <joel@....id.au>, Lubomir Rintel <lkundrak@...sk>,
        Christian Gmeiner <christian.gmeiner@...il.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Leo Li <leoyang.li@....com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        "michael@...le.cc" <michael@...le.cc>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build

On Mon, Jul 27, 2020 at 10:18 AM Anson Huang <anson.huang@....com> wrote:
> > Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build
> >
> > On Wed, Jul 22, 2020 at 3:50 AM Anson Huang <Anson.Huang@....com>
> > wrote:
> > >
> > > Change config to tristate, add module device table, module author,
> > > description and license to support module build for i.MX GPIO driver.
> > >
> > > As this is a SoC GPIO module, it provides common functions for most of
> > > the peripheral devices, such as GPIO pins control, secondary interrupt
> > > controller for GPIO pins IRQ etc., without GPIO driver, most of the
> > > peripheral devices will NOT work properly, so GPIO module is similar
> > > with clock, pinctrl driver that should be loaded ONCE and never
> > > unloaded.
> > >
> > > Since MXC GPIO driver needs to have init function to register syscore
> > > ops once, here still use subsys_initcall(), NOT module_platform_driver().
> >
> > I'm not following this explanation.
> >
> > Why is this driver using syscore_ops rather than
> > SIMPLE_DEV_PM_OPS() or similar?
>
> Below is the original patch of using syscore_ops, it has explanation:
>
> commit 1a5287a3dbc34cd0c02c8f64c9131bd23cdfe2bb
> Author: Anson Huang <anson.huang@....com>
> Date:   Fri Nov 9 04:56:56 2018 +0000
>
>     gpio: mxc: move gpio noirq suspend/resume to syscore phase
>
>     During noirq suspend/resume phase, GPIO irq could arrive
>     and its registers like IMR will be changed by irq handle
>     process, to make the GPIO registers exactly when it is
>     powered ON after resume, move the GPIO noirq suspend/resume
>     callback to syscore suspend/resume phase, local irq is
>     disabled at this phase so GPIO registers are atomic.

The description makes sense, but this must be a problem that
other gpio/pinctrl irqchip drivers have as well.

Linus, could you have a look? I see these other pinctrl drivers
using SIMPLE_DEV_PM_OPS:

drivers/pinctrl/nomadik/pinctrl-nomadik.c:static
SIMPLE_DEV_PM_OPS(nmk_pinctrl_pm_ops,
drivers/pinctrl/pinctrl-rockchip.c:static
SIMPLE_DEV_PM_OPS(rockchip_pinctrl_dev_pm_ops,
rockchip_pinctrl_suspend,
drivers/pinctrl/pinctrl-stmfx.c:static
SIMPLE_DEV_PM_OPS(stmfx_pinctrl_dev_pm_ops,
drivers/pinctrl/qcom/pinctrl-msm.c:SIMPLE_DEV_PM_OPS(msm_pinctrl_dev_pm_ops,
msm_pinctrl_suspend,
drivers/pinctrl/spear/pinctrl-plgpio.c:static
SIMPLE_DEV_PM_OPS(plgpio_dev_pm_ops, plgpio_suspend, plgpio_resume);

Linus, can you have a look and see if that same problem applies
to all of the above?

It seems that some drivers use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS()
instead, which looks like it is meant to address the same problem, but
as I have not used that myself, I may be misunderstanding the problem
or what this one does.

> > Why do you need subsys_initcall() rather than a device_initcall()?
>
> The subsys_initcal() is done by below commit, the commit log has detail explanation.
>
>
> commit e188cbf7564fba80e8339b9406e8740f3e495c63
> Author: Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
> Date:   Thu Sep 8 04:48:15 2016 +0300
>
>     gpio: mxc: shift gpio_mxc_init() to subsys_initcall level

That commit made the initialization later not earlier, as it originally
was a postcore_initcall(). In the loadable module case, you make
it even later than that, possibly as the last module loaded when
booting up the system (followed by a storm of deferred probes).

> > If the subsys_initcall() is indeed required here instead of device_initcall(), how
> > can it work if the driver is a loadable module?
>
> My understanding is: there are two scenarios, one for built-in case, the other is for loadable module,
> the subsys_initcall() is for built-in case according to the upper commit, for loadable
> module, the user needs to handle the sequence of modules loaded.

I don't think we can rely on user space to coordinate module load order.
The modules are generally loaded in an arbitrary order during the
coldplug phase of the boot when user space looks at the available
devices and loads a module for each one of them in the order it
finds them in sysfs.

This means all drivers that rely on gpio, pinctrl or irqchip interfaces
exported from this driver have to be able to deal with them not being
there. This can also happen when the pinctrl driver is the only one
that is a loadable module, while everything else is built-in. While
that is not a configuration that users would likely choose intentionally,
I don't see a reason why it shouldn't work.

Using module_init() or builtin_platform_driver() here would make
give similar behavior for the built-in and modular cases and be
somewhat more consistent, so you don't run into bugs only when
the driver is a loadable module but make them obvious even to
existing users with a builtin driver.

       Arnd

Powered by blists - more mailing lists