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=0.9 required=3.0 tests=DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 4A36AC3A59E for ; Wed, 21 Aug 2019 16:28:31 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 15A3F206DD for ; Wed, 21 Aug 2019 16:28:31 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="JFTo06sB"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="uFvB4ztT" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 15A3F206DD Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=TbM77uRwGYiUAdfE3uBC5Mq82Xlb6CDdo06YAL75P1I=; b=JFTo06sBPto4uGnP/Le1UDVhg DDN4eANap9UQkX3XNsIp7Bw7u28GZfp0RGNomo6zma1DKmIz//0B32Bu/rRF3Go3hcGoNygMtvIaS aj+m+kRj55isGHNm7IXRX1bDZodY/LE2TkFdh2P8JRAom4ZSJuBiDqBeXt5LiDUGIJplCgzDUDzXU p35dfXcvI7bqg26zi3orsLf4cV/Kj1Vk7Vx0bAvBpuE7VepPPLW3ioGoU8wMl0iv/0BkDsyn0Hq9o +L6wJCLrynvB2UF3F/hjtKiXCtfn32TGZwRyAe7Jp9xiwLdRl+rqOqJipWF3nnr5qqNZO4oeh4ikg ZZODnNlmQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1i0TTD-0008Ak-Vu; Wed, 21 Aug 2019 16:28:24 +0000 Received: from mail-wr1-x444.google.com ([2a00:1450:4864:20::444]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1i0TT9-000881-3t for linux-amlogic@lists.infradead.org; Wed, 21 Aug 2019 16:28:22 +0000 Received: by mail-wr1-x444.google.com with SMTP id z11so2643733wrt.4 for ; Wed, 21 Aug 2019 09:28:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=ywUCIB+cRj4cRKcIpwpQFY+NrjfdLRDV76eNoo3hv/0=; b=uFvB4ztTYbbnWUcXOBHe8H8LAboUxJ40gdIQ8giCnWFtsb9UgOEjxHLhk991t4jTGN eCF76EMqqZcwUUggeElvFMdSv3nd9GMdRRD9/ueZ3l1sGFh5qp4rzV/F9exy0Pan40hz 4PQXQbcn/bS4V4Ul6j8X/xkS1A8t6hbKtC0+feu4M4x0ug7E9eohpDOGPbdTZEdGE5xp oWXf5eGy9tXHQQNCAJ9qKHkVI+1qxH9DjXQVHnnzGFDT8vXyGGF07ivgT28hGYHqtMYz uRt7OuhZuQdSsp1AC48M9oSsPdaOf8XzF4sRuu6fi+yDIBmpequ4bcfS0OekejftB9Zu GYlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=ywUCIB+cRj4cRKcIpwpQFY+NrjfdLRDV76eNoo3hv/0=; b=s12TTgh5FKP7QL2fZIhWWmoZvfhyKwQ0uZOBuTnVbNSmXHGww0eWEVzL/mPqibmrx7 7sB/wP3qo4/19oveG1S2w03CheOoIqW5BLE8OW+57PKcQa85ZosAS86eDjka5hXOIamo fqCiICqLm0qGcG636FDY61xWScHPiCrI6OtOhBNMCNsJ7/Mi8JeYghc8KBG28ayYRCcx fTiUGbBHWcRZ6AvGU0EWvUnR6tmG8x5BMriCbpUmuDt2FHgkfd4WalLYIYIgFaNPgj4i hHG7o3/jnoHAMzdHkp3MrTisiGo4W5LgFw36uH62p5AD+gCO2+QTlYM+v8dAw5ERArUw 5OsA== X-Gm-Message-State: APjAAAUmd5cWPDMfBa4f2+cV7CMydcpRMOE5DtSImWhav9DlzHNr5IU7 ud/jG9WP6PXTkynIUJVmU+p2Zw== X-Google-Smtp-Source: APXvYqzzf/pBZTUAqrSrh8E6fLbQ03SLEZocUzQ/q912ZaLgwow4co7GX1wxWZBSfdK4XkjU4huGbA== X-Received: by 2002:adf:e750:: with SMTP id c16mr2099535wrn.199.1566404897059; Wed, 21 Aug 2019 09:28:17 -0700 (PDT) Received: from google.com ([2a00:79e0:d:210:e8f7:125b:61e9:733d]) by smtp.gmail.com with ESMTPSA id u130sm1026138wmg.28.2019.08.21.09.28.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Aug 2019 09:28:16 -0700 (PDT) Date: Wed, 21 Aug 2019 17:28:12 +0100 From: Matthias Maennich To: Guenter Roeck Subject: Re: [PATCH v3 11/11] RFC: watchdog: export core symbols in WATCHDOG_CORE namespace Message-ID: <20190821162812.GB77665@google.com> References: <20190813121733.52480-1-maennich@google.com> <20190821114955.12788-1-maennich@google.com> <20190821114955.12788-12-maennich@google.com> <20190821145911.GA6521@roeck-us.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190821145911.GA6521@roeck-us.net> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190821_092819_180915_ABCC573C X-CRM114-Status: GOOD ( 28.35 ) X-BeenThere: linux-amlogic@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Tomer Maimon , lucas.de.marchi@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arch@vger.kernel.org, linux-samsung-soc@vger.kernel.org, Kevin Hilman , Michal Simek , Ludovic Desroches , mingo@redhat.com, geert@linux-m68k.org, NXP Linux Team , Tomas Winkler , Jean Delvare , Sascha Hauer , tglx@linutronix.de, michal.lkml@markovi.net, Scott Branden , Andrew Jeffery , gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Pengutronix Kernel Team , Alexandre Belloni , linux-aspeed@lists.ozlabs.org, yamada.masahiro@socionext.com, Thierry Reding , Alexandre Torgue , Chunyan Zhang , Jonathan Hunter , Kukjin Kim , kernel-team@android.com, sspatil@google.com, linux-watchdog@vger.kernel.org, linux-kbuild@vger.kernel.org, linux-arm-msm@vger.kernel.org, pombredanne@nexb.com, linux-m68k@lists.linux-m68k.org, linux-rpi-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, maco@android.com, linux-arm-kernel@lists.infradead.org, Barry Song , Johannes Thumshirn , oneukum@suse.com, Patrice Chotard , Stefan Wahren , Maxime Coquelin , kstewart@linuxfoundation.org, usb-storage@lists.one-eyed-alien.net, linux-tegra@vger.kernel.org, patches@opensource.cirrus.com, joel@joelfernandes.org, sam@ravnborg.org, linux-rtc@vger.kernel.org, Florian Fainelli , Benjamin Fair , Eric Anholt , Krzysztof Kozlowski , Nancy Yuen , Chen-Yu Tsai , bcm-kernel-feedback-list@broadcom.com, Joel Stanley , stern@rowland.harvard.edu, arnd@arndb.de, Ray Jui , Vladimir Zapolskiy , Orson Zhai , linux-hwmon@vger.kernel.org, Support Opensource , Andreas Werner , Avi Fishman , maco@google.com, jeyu@kernel.org, Shawn Guo , Baruch Siach , Mans Rullgard , Maxime Ripard , Jerry Hoemann , Tali Perry , hpa@zytor.com, linux-scsi@vger.kernel.org, openbmc@lists.ozlabs.org, x86@kernel.org, Andy Gross , Marc Gonzalez , William Breathitt Gray , linux-mediatek@lists.infradead.org, Fabio Estevam , Matthias Brugger , Wim Van Sebroeck , Alessandro Zummo , Baolin Wang , Patrick Venture , Nicolas Ferre , linux-modules@vger.kernel.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-amlogic" Errors-To: linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org Hi Guenter! On Wed, 21 Aug, 07:59, Guenter Roeck wrote: >On Wed, Aug 21, 2019 at 12:49:26PM +0100, Matthias Maennich wrote: >> Modules using these symbols are required to explicitly import the >> namespace. This patch was generated with the following steps and serves >> as a reference to use the symbol namespace feature: >> >> 1) Use EXPORT_SYMBOL_NS* macros instead of EXPORT_SYMBOL* for symbols >> in watchdog_core.c >> 2) make (see warnings during modpost about missing imports) >> 3) make nsdeps >> >> I used 'allmodconfig' for the above steps to ensure all occurrences are >> patched. >> >> Defining DEFAULT_SYMBOL_NAMESPACE in the Makefile is not trivial in this >> case as not only watchdog_core is defined in drivers/watchdog/Makefile. >> Hence this patch uses the variant of using the EXPORT_SYMBOL_NS* macros >> to export into a different namespace. >> >I don't have the context, and thus I am missing the point of this patch >set. Whatever it is supposed to accomplish, it seems extreme to me >to require extra code in each driver for it. > Unfortunately, get_maintainer.pl has helped me too much and this series got blocked by some mailing lists due to the large amount of recipients. Following versions will be sent to the previous audience + the linux-watchdog list. For context, the full series (including previous versions) can be found on lore at https://lore.kernel.org/lkml/20180716122125.175792-1-maco@android.com/ and the cover letter for v3 has made it to linux-amlogic https://lore.kernel.org/linux-amlogic/20190821114955.12788-1-maennich@google.com/ >Anyway, WATCHDOG_CORE would be the default namespace (if it is what >I think it is) for watchdog drivers, even though not all watchdog drivers >use it. As such, I am missing an explanation why defining it in Makefile >is not trivial. "... as not only watchdog_core is defined in >drivers/watchdog/Makefile" does not mean anything to me and is not a real True, that is a bit out of context. Especially considering you did not receive any other messages of that series. Defining a namespace a symbol should be exported to can be done in different ways. All of them effectively change the EXPORT_SYMBOL* macro's behaviour. The method I am referring to is using ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=WATCHDOG_CORE directly in drivers/watchdog/Makefile. Since this would also apply the namespace to exports in non-core modules it would be incorrect. Thus I used the method of applying the namespace directly by changing the EXPORT_SYMBOL macro expansion. >explanation. Also, it is not immediately obvious to me why "select >WATCHDOG_CORE" in Kconfig would not automatically imply that WATCHDOG_CORE >is used by a given driver, and why it is impossible to use that >information to avoid the per-driver changes. > One intention of this patch series is to make exporting and using of namespaces explicit. As such, the subsystem exporting symbols is defining the namespace it exports to and the module using a namespace is supposed to explicitly declare its usage via import. In case of watchdog (and probably other cases) it might make sense to find a way to implicitly import the namespace for in-tree drivers in the same area. >I am also missing an explanation why WATCHDOG_CORE is going to be a >separate namespace to start with. Maybe that discussion has happened, >but I don't recall being advised or asked or told about it. Are we also >going to have a new HWMON_CORE namespace ? And the same for each other >subsystem in the kernel ? > This very patch is an RFC to demonstrate how Symbol Namespaces would be used based on the current implementation (the other RFC as part of this series is for the introduction of the namespace USB_STORAGE). WATCHDOG_CORE serves as one of two examples. I do not think the two RFC patches should be merged along with this series. >Since this is being added to the watchdog API, it will have to be >documented accordingly. Watchdog driver writers, both inside and outside >the watchdog subsystem, will need to know that they now have to add an >additional boilerplate declaration into their drivers. > Completely agree. This is just an RFC that omits these details as it purely focuses on the introduction and consequences of such a namespace to demonstrate how the feature works. >Last but not least, combining patches affecting multiple subsystems in a >single patch will make it difficult to apply and will likely result in >conflicts. Personally I would prefer a split into one patch per affected >subsystem. Also, please keep in mind that new pending watchdog drivers >won't have the new boilerplate. I understand the point. Especially as I am already now affected by the long list of recipients when sending this patch. The problem with single patches here is, that once a symbol is exported into a namespace, all modules using it have to declare that import to avoid a warning at compile time and module load time. Hence the all-in-one approach. Luckily, the patch series also provides a way to address such a warning (via `make nsdeps`) that creates the necessary source code fix as a single line per module and namespace right after MODULE_LICENSE(). That is how this patch was created in the first place. Cheers, Matthias _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthias Maennich Date: Wed, 21 Aug 2019 17:28:12 +0100 Subject: [PATCH v3 11/11] RFC: watchdog: export core symbols in WATCHDOG_CORE namespace In-Reply-To: <20190821145911.GA6521@roeck-us.net> References: <20190813121733.52480-1-maennich@google.com> <20190821114955.12788-1-maennich@google.com> <20190821114955.12788-12-maennich@google.com> <20190821145911.GA6521@roeck-us.net> Message-ID: <20190821162812.GB77665@google.com> List-Id: To: linux-aspeed@lists.ozlabs.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Hi Guenter! On Wed, 21 Aug, 07:59, Guenter Roeck wrote: >On Wed, Aug 21, 2019 at 12:49:26PM +0100, Matthias Maennich wrote: >> Modules using these symbols are required to explicitly import the >> namespace. This patch was generated with the following steps and serves >> as a reference to use the symbol namespace feature: >> >> 1) Use EXPORT_SYMBOL_NS* macros instead of EXPORT_SYMBOL* for symbols >> in watchdog_core.c >> 2) make (see warnings during modpost about missing imports) >> 3) make nsdeps >> >> I used 'allmodconfig' for the above steps to ensure all occurrences are >> patched. >> >> Defining DEFAULT_SYMBOL_NAMESPACE in the Makefile is not trivial in this >> case as not only watchdog_core is defined in drivers/watchdog/Makefile. >> Hence this patch uses the variant of using the EXPORT_SYMBOL_NS* macros >> to export into a different namespace. >> >I don't have the context, and thus I am missing the point of this patch >set. Whatever it is supposed to accomplish, it seems extreme to me >to require extra code in each driver for it. > Unfortunately, get_maintainer.pl has helped me too much and this series got blocked by some mailing lists due to the large amount of recipients. Following versions will be sent to the previous audience + the linux-watchdog list. For context, the full series (including previous versions) can be found on lore at https://lore.kernel.org/lkml/20180716122125.175792-1-maco at android.com/ and the cover letter for v3 has made it to linux-amlogic https://lore.kernel.org/linux-amlogic/20190821114955.12788-1-maennich at google.com/ >Anyway, WATCHDOG_CORE would be the default namespace (if it is what >I think it is) for watchdog drivers, even though not all watchdog drivers >use it. As such, I am missing an explanation why defining it in Makefile >is not trivial. "... as not only watchdog_core is defined in >drivers/watchdog/Makefile" does not mean anything to me and is not a real True, that is a bit out of context. Especially considering you did not receive any other messages of that series. Defining a namespace a symbol should be exported to can be done in different ways. All of them effectively change the EXPORT_SYMBOL* macro's behaviour. The method I am referring to is using ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=WATCHDOG_CORE directly in drivers/watchdog/Makefile. Since this would also apply the namespace to exports in non-core modules it would be incorrect. Thus I used the method of applying the namespace directly by changing the EXPORT_SYMBOL macro expansion. >explanation. Also, it is not immediately obvious to me why "select >WATCHDOG_CORE" in Kconfig would not automatically imply that WATCHDOG_CORE >is used by a given driver, and why it is impossible to use that >information to avoid the per-driver changes. > One intention of this patch series is to make exporting and using of namespaces explicit. As such, the subsystem exporting symbols is defining the namespace it exports to and the module using a namespace is supposed to explicitly declare its usage via import. In case of watchdog (and probably other cases) it might make sense to find a way to implicitly import the namespace for in-tree drivers in the same area. >I am also missing an explanation why WATCHDOG_CORE is going to be a >separate namespace to start with. Maybe that discussion has happened, >but I don't recall being advised or asked or told about it. Are we also >going to have a new HWMON_CORE namespace ? And the same for each other >subsystem in the kernel ? > This very patch is an RFC to demonstrate how Symbol Namespaces would be used based on the current implementation (the other RFC as part of this series is for the introduction of the namespace USB_STORAGE). WATCHDOG_CORE serves as one of two examples. I do not think the two RFC patches should be merged along with this series. >Since this is being added to the watchdog API, it will have to be >documented accordingly. Watchdog driver writers, both inside and outside >the watchdog subsystem, will need to know that they now have to add an >additional boilerplate declaration into their drivers. > Completely agree. This is just an RFC that omits these details as it purely focuses on the introduction and consequences of such a namespace to demonstrate how the feature works. >Last but not least, combining patches affecting multiple subsystems in a >single patch will make it difficult to apply and will likely result in >conflicts. Personally I would prefer a split into one patch per affected >subsystem. Also, please keep in mind that new pending watchdog drivers >won't have the new boilerplate. I understand the point. Especially as I am already now affected by the long list of recipients when sending this patch. The problem with single patches here is, that once a symbol is exported into a namespace, all modules using it have to declare that import to avoid a warning at compile time and module load time. Hence the all-in-one approach. Luckily, the patch series also provides a way to address such a warning (via `make nsdeps`) that creates the necessary source code fix as a single line per module and namespace right after MODULE_LICENSE(). That is how this patch was created in the first place. Cheers, Matthias From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthias Maennich Subject: Re: [PATCH v3 11/11] RFC: watchdog: export core symbols in WATCHDOG_CORE namespace Date: Wed, 21 Aug 2019 17:28:12 +0100 Message-ID: <20190821162812.GB77665@google.com> References: <20190813121733.52480-1-maennich@google.com> <20190821114955.12788-1-maennich@google.com> <20190821114955.12788-12-maennich@google.com> <20190821145911.GA6521@roeck-us.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20190821145911.GA6521-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+glpam-linux-mediatek=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Guenter Roeck Cc: Tomer Maimon , lucas.de.marchi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-stm32-XDFAJ8BFU24N7RejjzZ/Li2xQDfSxrLKVpNB7YpNyf8@public.gmane.org, linux-arch-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Kevin Hilman , Michal Simek , Ludovic Desroches , mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org, NXP Linux Team , Tomas Winkler , Jean Delvare , Sascha Hauer , tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org, michal.lkml-yyZNWGI4GtDR7s880joybQ@public.gmane.org, Scott Branden , Andrew Jeffery , gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Pengutronix Kernel Team , Alexandre Belloni , linux-aspeed-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org, Thierry Reding List-Id: linux-arch.vger.kernel.org Hi Guenter! On Wed, 21 Aug, 07:59, Guenter Roeck wrote: >On Wed, Aug 21, 2019 at 12:49:26PM +0100, Matthias Maennich wrote: >> Modules using these symbols are required to explicitly import the >> namespace. This patch was generated with the following steps and serves >> as a reference to use the symbol namespace feature: >> >> 1) Use EXPORT_SYMBOL_NS* macros instead of EXPORT_SYMBOL* for symbols >> in watchdog_core.c >> 2) make (see warnings during modpost about missing imports) >> 3) make nsdeps >> >> I used 'allmodconfig' for the above steps to ensure all occurrences are >> patched. >> >> Defining DEFAULT_SYMBOL_NAMESPACE in the Makefile is not trivial in this >> case as not only watchdog_core is defined in drivers/watchdog/Makefile. >> Hence this patch uses the variant of using the EXPORT_SYMBOL_NS* macros >> to export into a different namespace. >> >I don't have the context, and thus I am missing the point of this patch >set. Whatever it is supposed to accomplish, it seems extreme to me >to require extra code in each driver for it. > Unfortunately, get_maintainer.pl has helped me too much and this series got blocked by some mailing lists due to the large amount of recipients. Following versions will be sent to the previous audience + the linux-watchdog list. For context, the full series (including previous versions) can be found on lore at https://lore.kernel.org/lkml/20180716122125.175792-1-maco-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org/ and the cover letter for v3 has made it to linux-amlogic https://lore.kernel.org/linux-amlogic/20190821114955.12788-1-maennich-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org/ >Anyway, WATCHDOG_CORE would be the default namespace (if it is what >I think it is) for watchdog drivers, even though not all watchdog drivers >use it. As such, I am missing an explanation why defining it in Makefile >is not trivial. "... as not only watchdog_core is defined in >drivers/watchdog/Makefile" does not mean anything to me and is not a real True, that is a bit out of context. Especially considering you did not receive any other messages of that series. Defining a namespace a symbol should be exported to can be done in different ways. All of them effectively change the EXPORT_SYMBOL* macro's behaviour. The method I am referring to is using ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=WATCHDOG_CORE directly in drivers/watchdog/Makefile. Since this would also apply the namespace to exports in non-core modules it would be incorrect. Thus I used the method of applying the namespace directly by changing the EXPORT_SYMBOL macro expansion. >explanation. Also, it is not immediately obvious to me why "select >WATCHDOG_CORE" in Kconfig would not automatically imply that WATCHDOG_CORE >is used by a given driver, and why it is impossible to use that >information to avoid the per-driver changes. > One intention of this patch series is to make exporting and using of namespaces explicit. As such, the subsystem exporting symbols is defining the namespace it exports to and the module using a namespace is supposed to explicitly declare its usage via import. In case of watchdog (and probably other cases) it might make sense to find a way to implicitly import the namespace for in-tree drivers in the same area. >I am also missing an explanation why WATCHDOG_CORE is going to be a >separate namespace to start with. Maybe that discussion has happened, >but I don't recall being advised or asked or told about it. Are we also >going to have a new HWMON_CORE namespace ? And the same for each other >subsystem in the kernel ? > This very patch is an RFC to demonstrate how Symbol Namespaces would be used based on the current implementation (the other RFC as part of this series is for the introduction of the namespace USB_STORAGE). WATCHDOG_CORE serves as one of two examples. I do not think the two RFC patches should be merged along with this series. >Since this is being added to the watchdog API, it will have to be >documented accordingly. Watchdog driver writers, both inside and outside >the watchdog subsystem, will need to know that they now have to add an >additional boilerplate declaration into their drivers. > Completely agree. This is just an RFC that omits these details as it purely focuses on the introduction and consequences of such a namespace to demonstrate how the feature works. >Last but not least, combining patches affecting multiple subsystems in a >single patch will make it difficult to apply and will likely result in >conflicts. Personally I would prefer a split into one patch per affected >subsystem. Also, please keep in mind that new pending watchdog drivers >won't have the new boilerplate. I understand the point. Especially as I am already now affected by the long list of recipients when sending this patch. The problem with single patches here is, that once a symbol is exported into a namespace, all modules using it have to declare that import to avoid a warning at compile time and module load time. Hence the all-in-one approach. Luckily, the patch series also provides a way to address such a warning (via `make nsdeps`) that creates the necessary source code fix as a single line per module and namespace right after MODULE_LICENSE(). That is how this patch was created in the first place. Cheers, Matthias From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=google.com (client-ip=2a00:1450:4864:20::443; helo=mail-wr1-x443.google.com; envelope-from=maennich@google.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="uFvB4ztT"; dkim-atps=neutral Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 46DClD03SqzDqkV for ; Thu, 22 Aug 2019 02:28:23 +1000 (AEST) Received: by mail-wr1-x443.google.com with SMTP id b16so2618545wrq.9 for ; Wed, 21 Aug 2019 09:28:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=ywUCIB+cRj4cRKcIpwpQFY+NrjfdLRDV76eNoo3hv/0=; b=uFvB4ztTYbbnWUcXOBHe8H8LAboUxJ40gdIQ8giCnWFtsb9UgOEjxHLhk991t4jTGN eCF76EMqqZcwUUggeElvFMdSv3nd9GMdRRD9/ueZ3l1sGFh5qp4rzV/F9exy0Pan40hz 4PQXQbcn/bS4V4Ul6j8X/xkS1A8t6hbKtC0+feu4M4x0ug7E9eohpDOGPbdTZEdGE5xp oWXf5eGy9tXHQQNCAJ9qKHkVI+1qxH9DjXQVHnnzGFDT8vXyGGF07ivgT28hGYHqtMYz uRt7OuhZuQdSsp1AC48M9oSsPdaOf8XzF4sRuu6fi+yDIBmpequ4bcfS0OekejftB9Zu GYlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=ywUCIB+cRj4cRKcIpwpQFY+NrjfdLRDV76eNoo3hv/0=; b=pM1+/Xv9cx/POBelRS9KLWyDAncWYn3sdFjeXFg6nx2thC5tVm66sDL7/hrZghIVyE RdjC2tBAeIbgh7VfjVTv7f3oY/jmjyiePy7JAUmZBSZETW01xKzpyWwAi+lP1drpTyll URCH4G8jfu69Ue1ZH4fYuV+aXD2clKjZ2aGNNF2SatQEYbACcH+fTtrZ5bFuwr/lD4xp OD4iRs6E5B/JTYq+N6SANI4dJN+e4eq1nnzplGLyxcIqq2xupuxv6rr871ZBZmU33AU9 zXdqc7h7mkxwD06kUSZ0HIjcS7CWf/2D3sZxTa55S3mHBzg6rtmM7RDUDGgpe0UJtY+4 ABUQ== X-Gm-Message-State: APjAAAXv8GwJjkS7mYsvY7/yOrsxhyMPyZ1JvlhfarW5JIbE6ePClH4c i66nBiQskgk14tbm4jOkYBoiUg== X-Google-Smtp-Source: APXvYqzzf/pBZTUAqrSrh8E6fLbQ03SLEZocUzQ/q912ZaLgwow4co7GX1wxWZBSfdK4XkjU4huGbA== X-Received: by 2002:adf:e750:: with SMTP id c16mr2099535wrn.199.1566404897059; Wed, 21 Aug 2019 09:28:17 -0700 (PDT) Received: from google.com ([2a00:79e0:d:210:e8f7:125b:61e9:733d]) by smtp.gmail.com with ESMTPSA id u130sm1026138wmg.28.2019.08.21.09.28.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Aug 2019 09:28:16 -0700 (PDT) Date: Wed, 21 Aug 2019 17:28:12 +0100 From: Matthias Maennich To: Guenter Roeck Cc: linux-kernel@vger.kernel.org, kernel-team@android.com, arnd@arndb.de, geert@linux-m68k.org, gregkh@linuxfoundation.org, hpa@zytor.com, jeyu@kernel.org, joel@joelfernandes.org, kstewart@linuxfoundation.org, linux-arch@vger.kernel.org, linux-kbuild@vger.kernel.org, linux-m68k@lists.linux-m68k.org, linux-modules@vger.kernel.org, linux-scsi@vger.kernel.org, linux-usb@vger.kernel.org, lucas.de.marchi@gmail.com, maco@android.com, maco@google.com, michal.lkml@markovi.net, mingo@redhat.com, oneukum@suse.com, pombredanne@nexb.com, sam@ravnborg.org, sspatil@google.com, stern@rowland.harvard.edu, tglx@linutronix.de, usb-storage@lists.one-eyed-alien.net, x86@kernel.org, yamada.masahiro@socionext.com, Jean Delvare , Alessandro Zummo , Alexandre Belloni , Wim Van Sebroeck , Joel Stanley , Andrew Jeffery , Nicolas Ferre , Ludovic Desroches , Eric Anholt , Stefan Wahren , Florian Fainelli , Ray Jui , Scott Branden , bcm-kernel-feedback-list@broadcom.com, Support Opensource , Baruch Siach , William Breathitt Gray , Jerry Hoemann , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Vladimir Zapolskiy , Tomas Winkler , Johannes Thumshirn , Andreas Werner , Kevin Hilman , Matthias Brugger , Avi Fishman , Tomer Maimon , Tali Perry , Patrick Venture , Nancy Yuen , Benjamin Fair , Michal Simek , Andy Gross , Kukjin Kim , Krzysztof Kozlowski , Barry Song , Orson Zhai , Baolin Wang , Chunyan Zhang , Patrice Chotard , Maxime Coquelin , Alexandre Torgue , Maxime Ripard , Chen-Yu Tsai , Marc Gonzalez , Mans Rullgard , Thierry Reding , Jonathan Hunter , linux-hwmon@vger.kernel.org, linux-rtc@vger.kernel.org, linux-watchdog@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-aspeed@lists.ozlabs.org, linux-rpi-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-mediatek@lists.infradead.org, openbmc@lists.ozlabs.org, linux-arm-msm@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-tegra@vger.kernel.org, patches@opensource.cirrus.com Subject: Re: [PATCH v3 11/11] RFC: watchdog: export core symbols in WATCHDOG_CORE namespace Message-ID: <20190821162812.GB77665@google.com> References: <20190813121733.52480-1-maennich@google.com> <20190821114955.12788-1-maennich@google.com> <20190821114955.12788-12-maennich@google.com> <20190821145911.GA6521@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20190821145911.GA6521@roeck-us.net> User-Agent: Mutt/1.10.1 (2018-07-13) X-Mailman-Approved-At: Mon, 02 Sep 2019 10:34:52 +1000 X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Aug 2019 16:28:24 -0000 Hi Guenter! On Wed, 21 Aug, 07:59, Guenter Roeck wrote: >On Wed, Aug 21, 2019 at 12:49:26PM +0100, Matthias Maennich wrote: >> Modules using these symbols are required to explicitly import the >> namespace. This patch was generated with the following steps and serves >> as a reference to use the symbol namespace feature: >> >> 1) Use EXPORT_SYMBOL_NS* macros instead of EXPORT_SYMBOL* for symbols >> in watchdog_core.c >> 2) make (see warnings during modpost about missing imports) >> 3) make nsdeps >> >> I used 'allmodconfig' for the above steps to ensure all occurrences are >> patched. >> >> Defining DEFAULT_SYMBOL_NAMESPACE in the Makefile is not trivial in this >> case as not only watchdog_core is defined in drivers/watchdog/Makefile. >> Hence this patch uses the variant of using the EXPORT_SYMBOL_NS* macros >> to export into a different namespace. >> >I don't have the context, and thus I am missing the point of this patch >set. Whatever it is supposed to accomplish, it seems extreme to me >to require extra code in each driver for it. > Unfortunately, get_maintainer.pl has helped me too much and this series got blocked by some mailing lists due to the large amount of recipients. Following versions will be sent to the previous audience + the linux-watchdog list. For context, the full series (including previous versions) can be found on lore at https://lore.kernel.org/lkml/20180716122125.175792-1-maco@android.com/ and the cover letter for v3 has made it to linux-amlogic https://lore.kernel.org/linux-amlogic/20190821114955.12788-1-maennich@google.com/ >Anyway, WATCHDOG_CORE would be the default namespace (if it is what >I think it is) for watchdog drivers, even though not all watchdog drivers >use it. As such, I am missing an explanation why defining it in Makefile >is not trivial. "... as not only watchdog_core is defined in >drivers/watchdog/Makefile" does not mean anything to me and is not a real True, that is a bit out of context. Especially considering you did not receive any other messages of that series. Defining a namespace a symbol should be exported to can be done in different ways. All of them effectively change the EXPORT_SYMBOL* macro's behaviour. The method I am referring to is using ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=WATCHDOG_CORE directly in drivers/watchdog/Makefile. Since this would also apply the namespace to exports in non-core modules it would be incorrect. Thus I used the method of applying the namespace directly by changing the EXPORT_SYMBOL macro expansion. >explanation. Also, it is not immediately obvious to me why "select >WATCHDOG_CORE" in Kconfig would not automatically imply that WATCHDOG_CORE >is used by a given driver, and why it is impossible to use that >information to avoid the per-driver changes. > One intention of this patch series is to make exporting and using of namespaces explicit. As such, the subsystem exporting symbols is defining the namespace it exports to and the module using a namespace is supposed to explicitly declare its usage via import. In case of watchdog (and probably other cases) it might make sense to find a way to implicitly import the namespace for in-tree drivers in the same area. >I am also missing an explanation why WATCHDOG_CORE is going to be a >separate namespace to start with. Maybe that discussion has happened, >but I don't recall being advised or asked or told about it. Are we also >going to have a new HWMON_CORE namespace ? And the same for each other >subsystem in the kernel ? > This very patch is an RFC to demonstrate how Symbol Namespaces would be used based on the current implementation (the other RFC as part of this series is for the introduction of the namespace USB_STORAGE). WATCHDOG_CORE serves as one of two examples. I do not think the two RFC patches should be merged along with this series. >Since this is being added to the watchdog API, it will have to be >documented accordingly. Watchdog driver writers, both inside and outside >the watchdog subsystem, will need to know that they now have to add an >additional boilerplate declaration into their drivers. > Completely agree. This is just an RFC that omits these details as it purely focuses on the introduction and consequences of such a namespace to demonstrate how the feature works. >Last but not least, combining patches affecting multiple subsystems in a >single patch will make it difficult to apply and will likely result in >conflicts. Personally I would prefer a split into one patch per affected >subsystem. Also, please keep in mind that new pending watchdog drivers >won't have the new boilerplate. I understand the point. Especially as I am already now affected by the long list of recipients when sending this patch. The problem with single patches here is, that once a symbol is exported into a namespace, all modules using it have to declare that import to avoid a warning at compile time and module load time. Hence the all-in-one approach. Luckily, the patch series also provides a way to address such a warning (via `make nsdeps`) that creates the necessary source code fix as a single line per module and namespace right after MODULE_LICENSE(). That is how this patch was created in the first place. Cheers, Matthias