linux-kernel - RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM6PR04MB4966AE313DE13AD65B2D47B280280@AM6PR04MB4966.eurprd04.prod.outlook.com>
Date:   Mon, 7 Sep 2020 08:17:55 +0000
From:   Aisheng Dong <aisheng.dong@....com>
To:     Anson Huang <anson.huang@....com>, Arnd Bergmann <arnd@...db.de>
CC:     Daniel Baluta <daniel.baluta@....com>,
        "festevam@...il.com" <festevam@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "stefan@...er.ch" <stefan@...er.ch>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl driver as
 module

> From: Anson Huang <anson.huang@....com>
> Sent: Friday, July 17, 2020 5:53 PM
> 
> Hi, Arnd
> 
> > Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU pinctrl
> > driver as module
> >
> > On Fri, Jul 17, 2020 at 11:24 AM Anson Huang <anson.huang@....com>
> > wrote:
> > > > Subject: Re: [PATCH 1/2] pinctrl: imx: Support building SCU
> > > > pinctrl driver as module
> > > > +MODULE_AUTHOR("Dong Aisheng<aisheng.dong@....com>");
> > > > +MODULE_DESCRIPTION("NXP i.MX SCU common pinctrl driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > >
> > > >
> > > > This can be in a separate patch.
> > >
> > > I don't understand, without adding module license, changing the SCU
> > > pinctrl driver to "tristate", when building with =M, the build will
> > > have warning as below, so I think it does NOT make sense to split it
> > > to 2
> > patches.
> > >
> > >   CC [M]  drivers/pinctrl/freescale/pinctrl-scu.o
> > >   MODPOST Module.symvers
> > > WARNING: modpost: missing MODULE_LICENSE() in
> > drivers/pinctrl/freescale/pinctrl-scu.o
> > >   LD [M]  drivers/pinctrl/freescale/pinctrl-scu.ko
> >
> > I agree it would be clearer to do it as separate patches, but you then
> > have to be careful about the order to avoid the problem you mention.
> >
> > A clear indication that it may be sensible to split up the patch is
> > that your changelog has a list of five items in it, which are mostly doing
> different things.
> > The ideal way to split up a patch series is to have each patch with a
> > changelog that has to explain exactly one thing, and makes it obvious
> > how each changed line corresponds to the description, but never
> > explain the same thing in more than one patch (i.e. you combine
> > patches that do the same thing in multiple files).
> >
> > In this case, a good split may be:
> >
> > patch 1:
> >    - Use function callbacks for SCU related functions in pinctrl-imx.c
> >       in order to support the scenario of PINCTRL_IMX is built in
> >       while PINCTRL_IMX_SCU is built as module;
> >     - All drivers using SCU pinctrl driver need to initialize the
> >       SCU related function callback;
> >
> > patch 2:
> >     - Export SCU related functions and use "IS_ENABLED" instead of
> >       "ifdef" to support SCU pinctrl driver user and itself to be
> >       built as module;
> >     - Change PINCTR_IMX_SCU to tristate;
> >     - Add module author, description and license.
> >
> > and then rewrite the description to not have a bulleted list.
> >
> > That said, I don't think it is critical here, and I would not have
> > complained about the version you sent.
> >
> > If you end up changing the patch, I think you can actually drop the
> > "#if IS_ENABLED()" check entirely, as the functions are now always
> > assumed to be available, and we don't #ifdef declarations when there
> > is no #else path otherwise.
> >
> 
> Thanks for the good suggestion, if there is other comment need a V2, or
> maintainer thinks it is better to split it following your guide, I will send V2
> following your guide.
> 

Pls do as Arnd suggested.
Besides that, I have a few minor comments in separate replies.

Regards
Aisheng

> Thanks,
> Anson