Re: [PATCH 1/3] gpio: mxc: Support module build

[John Stultz]

On Fri, Jul 17, 2020 at 5:01 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> Greg, John,
>
> we need some guidance here. See below.
>
> On Thu, Jul 16, 2020 at 4:38 PM Anson Huang <anson.huang@xxxxxxx> wrote:
> > [Me]
> > > On Wed, Jul 15, 2020 at 4:44 AM Anson Huang <anson.huang@xxxxxxx>
>
> > > > I tried to replace the subsys_initcall() with
> > > > module_platform_driver(), but met issue about "
> > > > register_syscore_ops(&mxc_gpio_syscore_ops);" which is called in
> > > > gpio_mxc_init() function, this function should be called ONLY once,
> > > > moving it to .probe function is NOT working, so we may need to keep the
> > > > gpio_mxc_init(), that is another reason that we may need to keep
> > > > subsys_initcall()?
> > >
> > > This looks a bit dangerous to keep like this while allowing this code to be used
> > > from a module.
> > >
> > > What happens if you insmod and rmmod this a few times, really?
> > > How is this tested?
> > >
> > > This is not really modularized if that isn't working, just that modprobing once
> > > works isn't real modularization IMO, it seems more like a quick and dirty way
> > > to get Androids GKI somewhat working with the module while not properly
> > > making the module a module.
> > >
> > > You need input from the driver maintainers on how to handle this.
> >
> > As far as I know, some general/critical modules are NOT supporting rmmod, like
> > clk, pinctrl, gpio etc., and I am NOT sure whether Android GKI need to support
> > rmmod for these system-wide-used module, I will ask them for more detail about
> > this.
> >
> > The requirement I received is to support loadable module, but so far no hard requirement
> > to support module remove for gpio driver, so, is it OK to add it step by step, and this patch
> > series ONLY to support module build and one time modprobe?
>
> While I am a big fan of the Android GKI initiative this needs to be aligned
> with the Linux core maintainers, so let's ask Greg. I am also paging
> John Stultz on this: he is close to this action.
>
> They both know the Android people very well.
>
> So there is a rationale like this going on: in order to achieve GKI goals
> and have as much as possible of the Linux kernel stashed into loadable
> kernel modules, it has been elevated to modus operandi amongst
> the developers pushing this change that it is OK to pile up a load of
> modules that cannot ever be unloaded.
>
> This is IIUC regardless of whether all consumers of the module are
> actually gone: it would be OK to say make it impossible to rmmod
> a clk driver even of zero clocks from that driver is in use. So it is not
> dependency-graph problem, it is a "load once, never remove" approach.
>
> This rationale puts me as subsystem maintainer in an unpleasant spot:
> it is really hard to tell case-to-case whether that change really is a
> technical advantage for the kernel per se or whether it is done for the
> greater ecosystem of Android.
>
> Often I would say it makes it possible to build a smaller kernel vmlinux
> so OK that is an advantage. On the other hand I have an inkling that I
> should be pushing developers to make sure that rmmod works.
>
> As a minimum requirement I would expect this to be marked by
>
> struct device_driver {
>    (...)
>     /* This module absolutely cannot be unbound */
>    .suppress_bind_attrs = true;
> };
>
> So that noone would be able to try to unbind this (could even be an
> attack vector!)
>
> What is our broader reasoning when it comes to this? (I might have
> missed some mail thread here.)

Sorry for being a little late here, was out for a few days.

So yea, wrt to some of the Android GKI related efforts I've been
involved with, loading once and not unloading is fine for the usage
model.

I can understand it being a bit ugly compared to drivers with proper
unloading support, and I think for most new driver submissions,
maintainers can reasonably push to see proper unloading being
implemented.

But there are some pragmatic cases with low-level drivers (as you
mentioned: clk, pinctrl, gpio, etc) where sorting out the unloading is
particularly complicated, or there is some missing infrastructure, and
in those cases being able to load a "permanent" module seems to me
like a clear benefit.  After all, it seems a bit strange to enforce
that drivers be unloadable when the same code was considered ok to be
merged as a built-in.

So I think there's a reasonable case for the preference order to be:
"built-in" < "permanent module" < "unloadable module".

And of course, it can be more complicated, as enabling a driver to be
a module can have rippling effects on other code that may call into
it. But I think maintainers have the best sense of how to draw the
line there.

thanks
-john