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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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

Yours,
Linus Walleij