Hi, Arnd

Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build

On Mon, Jul 27, 2020 at 10:18 AM Anson Huang [off-list ref]
wrote:

quoted

quoted

Subject: Re: [PATCH V2 1/4] gpio: mxc: Support module build

On Wed, Jul 22, 2020 at 3:50 AM Anson Huang [off-list ref]
wrote:

quoted

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().

quoted

quoted

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 [off-list ref]
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.

quoted

quoted

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.

quoted

commit e188cbf7564fba80e8339b9406e8740f3e495c63
Author: Vladimir Zapolskiy [off-list ref]
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).

Yes, loadable module will make it even later, the assumption is userspace can load it
before any users depend on GPIO driver. Given that we have to support loadable module
for all SoC specific module, do you have any other suggestion of how to proceed this
requirement for SoC GPIO driver?

 

quoted

quoted

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.

My original idea of adding loadable module support for SoC specific module is, try
to keep it exactly same when the driver is built-in, but for GKI support, first, we need
to support GPIO driver built as module, and we definitely need to think about the module
load sequence to handle these dependency, but thinking about the common module widely
used by devices, such as pinctrl, clock and GPIO, maybe other modules need some patches
to handle the dependency, but that will be done later when we are working for those modules.

So, could you please help advise how to proceed it for this GPIO driver to support loadable module?

Thanks,
Anson



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel