Message-ID: <20200717121436.GA2953399@kroah.com>
Date:   Fri, 17 Jul 2020 14:14:36 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Anson Huang <anson.huang@....com>,
        John Stultz <john.stultz@...aro.org>,
        Russell King <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>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        "oleksandr.suvorov@...adex.com" <oleksandr.suvorov@...adex.com>,
        Adam Ford <aford173@...il.com>,
        Andreas Kemnade <andreas@...nade.info>,
        "hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Leo Li <leoyang.li@....com>, Vinod Koul <vkoul@...nel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Olof Johansson <olof@...om.net>,
        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>, Jon Corbet <corbet@....net>
Subject: Re: [PATCH 1/3] gpio: mxc: Support module build

On Fri, Jul 17, 2020 at 02:01:16PM +0200, Linus Walleij wrote:
> Greg, John,
> 
> we need some guidance here. See below.
> 
> On Thu, Jul 16, 2020 at 4:38 PM Anson Huang <anson.huang@....com> wrote:
> > [Me]
> > > On Wed, Jul 15, 2020 at 4:44 AM Anson Huang <anson.huang@....com>
> 
> > > > 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.

Why can't the module be unloaded?  Is it just because they never
implement the proper "remove all resources allocated" logic in a remove
function, or something else?

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

Sounds like a "lazy" 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.

I can see where a number of modules just can not ever be removed because
of resources and not being able to properly tear down, but that doesn't
mean that a driver author shouldn't at least try, right?

> 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;
> };

No, that's not what bind/unbind is really for.  That's a per-subsystem
choice as to if you want to allow devices to be added/removed from
drivers at runtime.  It has nothing to do with module load/unload.

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

Android is just finally pushing vendors to get their code upstream,
which is a good thing to see.  And building things as a module is an
even better thing as now it is finally allowing arm64 systems to be
built to support more than one specific hardware platform at runtime.

So moving drivers to modules is good.  If a module can be removed, even
better, but developers should not be lazy and just flat out not try at
all to make their code unloadable if at all possible.

Does that help?

thanks,

greg k-h

Powered by blists - more mailing lists