| 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
| ||
|
Message-ID: <20151117093836.052c86b5@bbrezillon>
Date: Tue, 17 Nov 2015 09:38:36 +0100
From: Boris Brezillon <boris.brezillon@...e-electrons.com>
To: Brian Norris <computersforpeace@...il.com>,
Julia Lawall <Julia.Lawall@...6.fr>
Cc: David Woodhouse <dwmw2@...radead.org>,
linux-mtd@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
Hartley Sweeten <hsweeten@...ionengravers.com>,
Ryan Mallon <rmallon@...il.com>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <kernel@...gutronix.de>,
Imre Kaloz <kaloz@...nwrt.org>,
Krzysztof Halasa <khalasa@...p.pl>,
Tony Lindgren <tony@...mide.com>, linux-omap@...r.kernel.org,
Alexander Clouter <alex@...riz.org.uk>,
Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
Gregory CLEMENT <gregory.clement@...e-electrons.com>,
Jason Cooper <jason@...edaemon.net>,
Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
Andrew Lunn <andrew@...n.ch>, Daniel Mack <daniel@...que.org>,
Haojian Zhuang <haojian.zhuang@...il.com>,
Robert Jarzmik <robert.jarzmik@...e.fr>,
Marek Vasut <marek.vasut@...il.com>,
Steven Miao <realmz6@...il.com>,
adi-buildroot-devel@...ts.sourceforge.net,
Mikael Starvik <starvik@...s.com>,
Jesper Nilsson <jesper.nilsson@...s.com>,
linux-cris-kernel@...s.com, Josh Wu <josh.wu@...el.com>,
Wan ZongShun <mcuos.com@...il.com>,
Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
Maxim Levitsky <maximlevitsky@...il.com>,
Kukjin Kim <kgene@...nel.org>,
Krzysztof Kozlowski <k.kozlowski@...sung.com>,
linux-samsung-soc@...r.kernel.org,
Maxime Ripard <maxime.ripard@...e-electrons.com>,
Chen-Yu Tsai <wens@...e.org>, linux-sunxi@...glegroups.com,
Stefan Agner <stefan@...er.ch>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
devel@...verdev.osuosl.org
Subject: Re: [PATCH 14/27] mtd: nand: use the mtd instance embedded in
struct nand_chip
Hi Brian,
On Mon, 16 Nov 2015 19:00:19 -0800
Brian Norris <computersforpeace@...il.com> wrote:
> Hi Boris,
>
> On Mon, Nov 16, 2015 at 02:37:47PM +0100, Boris Brezillon wrote:
> > struct nand_chip now embeds an mtd device. Patch all drivers to make use
> > of this mtd instance instead of using the instance embedded in their
> > private struct or dynamically allocated.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@...e-electrons.com>
> > Cc: Julia Lawall <Julia.Lawall@...6.fr>
> > ---
> > Most of those changes were generate with this coccinelle script:
> > http://code.bulix.org/5vxuih-89429
>
> I appreciate that this patch is mostly autogenerated (a good thing for
> preventing errors!), but there are some issues that I don't think play
> out very well stylistically. Hopefully the cocci script can be improved
> to handle some of this?
>
> I'll try to point out a few snippets below.
>
> Also, in case others are interested in reviewing your cocci script
> directly, it might be better to paste it inline than to link to it.
> Given the size of the patch, I don't think people would mind a few dozen
> extra lines to show how it wsa generated. Or maybe stick some in the
> cover letter too, if you end up reusing them in several patches.
Sure, I'll paste the script directly in the commit message next time.
>
> > ---
> > drivers/mtd/nand/ams-delta.c | 13 ++--
> > drivers/mtd/nand/atmel_nand.c | 11 ++-
> > drivers/mtd/nand/au1550nd.c | 18 ++---
> > drivers/mtd/nand/bcm47xxnflash/bcm47xxnflash.h | 1 -
> > drivers/mtd/nand/bcm47xxnflash/main.c | 7 +-
> > drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 2 +-
> > drivers/mtd/nand/bf5xx_nand.c | 14 ++--
> > drivers/mtd/nand/brcmnand/brcmnand.c | 11 ++-
> > drivers/mtd/nand/cafe_nand.c | 10 +--
> > drivers/mtd/nand/cmx270_nand.c | 11 ++-
> > drivers/mtd/nand/cs553x_nand.c | 13 ++--
> > drivers/mtd/nand/davinci_nand.c | 25 +++----
> > drivers/mtd/nand/denali.c | 61 +++++++++--------
> > drivers/mtd/nand/denali.h | 1 -
> > drivers/mtd/nand/diskonchip.c | 11 ++-
> > drivers/mtd/nand/docg4.c | 18 +++--
> > drivers/mtd/nand/fsl_elbc_nand.c | 22 +++---
> > drivers/mtd/nand/fsl_ifc_nand.c | 23 +++----
> > drivers/mtd/nand/fsl_upm.c | 26 +++----
> > drivers/mtd/nand/fsmc_nand.c | 59 +++++++++-------
> > drivers/mtd/nand/gpio.c | 16 ++---
> > drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 2 +-
> > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 20 +++---
> > drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 1 -
> > drivers/mtd/nand/hisi504_nand.c | 11 ++-
> > drivers/mtd/nand/jz4740_nand.c | 9 ++-
> > drivers/mtd/nand/lpc32xx_mlc.c | 7 +-
> > drivers/mtd/nand/lpc32xx_slc.c | 7 +-
> > drivers/mtd/nand/mpc5121_nfc.c | 3 +-
> > drivers/mtd/nand/mxc_nand.c | 5 +-
> > drivers/mtd/nand/nandsim.c | 12 ++--
> > drivers/mtd/nand/ndfc.c | 22 +++---
> > drivers/mtd/nand/nuc900_nand.c | 21 +++---
> > drivers/mtd/nand/omap2.c | 94 +++++++++++++++-----------
> > drivers/mtd/nand/orion_nand.c | 4 +-
> > drivers/mtd/nand/pasemi_nand.c | 14 ++--
> > drivers/mtd/nand/plat_nand.c | 14 ++--
> > drivers/mtd/nand/pxa3xx_nand.c | 33 ++++-----
>
> ^^ BTW, this file already has a few conflicts. Sorry :(
>
> I'll try to keep any eye out for things like this once we're close to
> being able to apply something like this, so I don't merge unnecessary
> churn. But for now, I hope we can review this series, and it won't be
> too much work to rebase/resend once the bigger things have been worked
> out.
No problem, resolving this conflict was pretty easy.
>
> > drivers/mtd/nand/r852.c | 34 ++++------
> > drivers/mtd/nand/r852.h | 1 -
> > drivers/mtd/nand/s3c2410.c | 19 +++---
> > drivers/mtd/nand/sh_flctl.c | 8 +--
> > drivers/mtd/nand/sharpsl.c | 18 ++---
> > drivers/mtd/nand/socrates_nand.c | 5 +-
> > drivers/mtd/nand/sunxi_nand.c | 13 ++--
> > drivers/mtd/nand/tmio_nand.c | 7 +-
> > drivers/mtd/nand/txx9ndfmc.c | 3 +-
> > drivers/mtd/nand/vf610_nfc.c | 5 +-
> > include/linux/mtd/sh_flctl.h | 3 +-
> > 49 files changed, 383 insertions(+), 385 deletions(-)
> >
>
> ...
>
> > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> > index f8aac0a..51748b4 100644
> > --- a/drivers/mtd/nand/atmel_nand.c
> > +++ b/drivers/mtd/nand/atmel_nand.c
>
> ...
>
> > @@ -318,7 +317,7 @@ static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int bank)
> >
> > if (bank) {
> > /* Only for a 2k-page or lower flash, NFC can handle 2 banks */
> > - if (host->mtd.writesize > 2048)
> > + if (nand_to_mtd(&host->nand_chip)->writesize > 2048)
>
> (This isn't the worst one, but it just happens to be one of the first.)
> There are many cases where the typical style would be to declare a new
> variable at the top of the function, where you perform the
> macro/function-call to convert from one abstraction to another. Like
>
> static int nfc_set_sram_bank(struct atmel_nand_host *host, unsigned int bank)
> {
> struct mtd_info *mtd = nand_to_mtd(&hot->nand_chip);
> ...
>
> and then use it later. Can that be done very easily?
>
> > return -EINVAL;
> > nfc_writel(host->nfc->hsmc_regs, BANK, ATMEL_HSMC_NFC_BANK1);
> > } else {
>
> ...
Honestly, I don't know how to do that with a coccinelle script, and it
will probably take me more time to find how to do it than addressing
those problems manually.
Julia, could you give us some hint?
>
> > diff --git a/drivers/mtd/nand/au1550nd.c b/drivers/mtd/nand/au1550nd.c
> > index 73fceb8..7e2a376 100644
> > --- a/drivers/mtd/nand/au1550nd.c
> > +++ b/drivers/mtd/nand/au1550nd.c
> > @@ -23,7 +23,6 @@
> >
> >
> > struct au1550nd_ctx {
> > - struct mtd_info info;
> > struct nand_chip chip;
> >
> > int cs;
> > @@ -197,7 +196,8 @@ static void au_read_buf16(struct mtd_info *mtd, u_char *buf, int len)
> >
> > static void au1550_hwcontrol(struct mtd_info *mtd, int cmd)
> > {
> > - struct au1550nd_ctx *ctx = container_of(mtd, struct au1550nd_ctx, info);
> > + struct au1550nd_ctx *ctx = container_of(mtd_to_nand(mtd),
> > + struct au1550nd_ctx, chip);
> > struct nand_chip *this = mtd_to_nand(mtd);
>
> This is another good example. It's a little awkward to do this at all
> (function call within a macro):
>
> container_of(mtd_to_nand(mtd), ...);
>
> but that's not unforgiveable. It's a bit worse, though, when followed by
> assigning the next field to the same thing:
>
> struct nand_chip *this = mtd_to_nand(mtd);
>
> i.e., this would be nicer to see as:
>
> struct nand_chip *this = mtd_to_nand(mtd);
> struct au1550nd_ctx *ctx = container_of(this,
> struct au1550nd_ctx, chip);
>
> Again, I'm not sure how best to automate this kind of transformation.
Yes, I noticed all those problems too, but as I said, I don't know how
to handle them using a coccinelle script.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists