From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 53B7BC433E0 for ; Fri, 17 Jul 2020 12:14:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 29FF7207E8 for ; Fri, 17 Jul 2020 12:14:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594988087; bh=kQgaBPzZfXga09Lqrx8wezDWPrDUuin2mS2Z1Z+whfg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=Iwl+GrzMbB4T2JWnDIIzWGdp0L1/ARw7RbRoNIU6ne6DqXYsf69dJL4uWC1DbL7Z1 ahtfh+xQeP56wZWkyXaz0x0B5fFB+zfSXUVr6ODZkru733Unh5FdrvTgq7FjPJ3e3g D1kz3ZycyxBLAbn3x3faKDQxRqIL0fWIbMt0Cc64= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726201AbgGQMOq (ORCPT ); Fri, 17 Jul 2020 08:14:46 -0400 Received: from mail.kernel.org ([198.145.29.99]:59646 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726113AbgGQMOq (ORCPT ); Fri, 17 Jul 2020 08:14:46 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9674D20717; Fri, 17 Jul 2020 12:14:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594988085; bh=kQgaBPzZfXga09Lqrx8wezDWPrDUuin2mS2Z1Z+whfg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=p6FRGlORHXpKEYBn0ItKKmJUsOqQE8Fxy+n52tof+J8hS401jHCZT/83Fls1fGYR/ FwNotv56Ks6M/2+FbxgGQ+zglvR2J4DqQhefMFE8ndjLGYz8xEy4KZA+dKNbcwMX7q 9hsVCffpc26VXWSO9GcSTe9kGWK3rgsLr6D94p1A= Date: Fri, 17 Jul 2020 14:14:36 +0200 From: Greg KH To: Linus Walleij Cc: Anson Huang , John Stultz , Russell King , Shawn Guo , Sascha Hauer , Sascha Hauer , Fabio Estevam , Catalin Marinas , Will Deacon , Bartosz Golaszewski , "oleksandr.suvorov@toradex.com" , Adam Ford , Andreas Kemnade , "hverkuil-cisco@xs4all.nl" , Bjorn Andersson , Leo Li , Vinod Koul , Geert Uytterhoeven , Olof Johansson , Linux ARM , "linux-kernel@vger.kernel.org" , "open list:GPIO SUBSYSTEM" , dl-linux-imx , Jon Corbet Subject: Re: [PATCH 1/3] gpio: mxc: Support module build Message-ID: <20200717121436.GA2953399@kroah.com> References: <1594164323-14920-1-git-send-email-Anson.Huang@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-gpio-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org 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 wrote: > > [Me] > > > On Wed, Jul 15, 2020 at 4:44 AM Anson Huang > > > > > 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