Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1287192

Re: [PATCH v3 bis 12/25] mtd: nand: use the mtd instance embedded in struct nand_chip

Show all headers | View raw

Hi Brian,

On Tue, 8 Dec 2015 16:17:41 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> 
> > diff --git a/drivers/mtd/nand/cmx270_nand.c b/drivers/mtd/nand/cmx270_nand.c
> > index 43bded6..84d027e 100644
> > --- a/drivers/mtd/nand/cmx270_nand.c
> > +++ b/drivers/mtd/nand/cmx270_nand.c
> > @@ -160,10 +160,8 @@ static int __init cmx270_init(void)
> >  	gpio_direction_input(GPIO_NAND_RB);
> >  
> >  	/* Allocate memory for MTD device structure and private data */
> > -	cmx270_nand_mtd = kzalloc(sizeof(struct mtd_info) +
> > -				  sizeof(struct nand_chip),
> > -				  GFP_KERNEL);
> > -	if (!cmx270_nand_mtd) {
> > +	this = kzalloc(sizeof(struct nand_chip), GFP_KERNEL);
> > +	if (!this) {
> >  		ret = -ENOMEM;
> >  		goto err_kzalloc;
> >  	}
> > @@ -175,8 +173,7 @@ static int __init cmx270_init(void)
> >  		goto err_ioremap;
> >  	}
> >  
> > -	/* Get pointer to private data */
> > -	this = (struct nand_chip *)(&cmx270_nand_mtd[1]);
> > +	cmx270_nand_mtd = nand_to_mtd(this);
> 
> So, you make cmx270_nand_mtd no longer kzalloc()'d, but I still see the
> cmx270_init() function end with:
> 
> err_scan:
>         iounmap(cmx270_nand_io);
> err_ioremap:
>         kfree(cmx270_nand_mtd);  <----- *** this! ***

Oh, crap.

> err_kzalloc:
>         gpio_free(GPIO_NAND_RB);
> err_gpio_request:
> 	gpio_free(GPIO_NAND_CS);
> 
> 	return ret;
> 
> }
> 
> I have a feeling there's a failing in your coccinelle script somewhere.

These changes are not automated, because it's kind of hard to address
all the different cases where the following pattern is employed;

var = kzalloc(sizeof(struct mtd_info) +
	      sizeof(struct nand_chip) + ..., ...);

Sometime var is an mtd_info pointer, sometime it's a nand_chip pointer
or directly a pointer to the private struct.

I'm pretty sure we could come up with a valid coccinelle script, but
given the number of drivers using this approach I'm not sure it is
worth it...

> 
> Given that I was only through 10 of 49 files changes, I think you might
> need to take a comb over your patch better.

I'll go over those changes one more time, but from my experience, these
kind of bugs are spotted more easily by people who didn't write the
code, so other reviews are more than welcome.

Also, as you suggested, I'll split the changes in several commits (one
per driver) so that you can pick them independently.

Thanks,

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2 00/25] mtd: nand: refactor the NAND subsystem (part 1) Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-01 12:20 +0100
  [PATCH v2 01/25] ARM: nand: make use of mtd_to_nand() where appropriate Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-01 12:20 +0100
  [PATCH v2 05/25] sh: nand: make use of mtd_to_nand() where appropriate Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-01 12:20 +0100
  [PATCH v2 06/25] mtd: nand: make use of mtd_to_nand() in NAND core code Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-01 12:20 +0100
  [PATCH v2 02/25] blackfin: nand: make use of mtd_to_nand() where appropriate Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-01 12:20 +0100
  [PATCH v2 08/25] staging: mt29f_spinand: make use of mtd_to_nand() Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-01 12:20 +0100
  [PATCH v2 11/25] coccinelle: nand: detect and correct drivers embedding an mtd_info object Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-01 12:20 +0100
    Re: [PATCH v2 11/25] coccinelle: nand: detect and correct drivers  embedding an mtd_info object Julia Lawall <julia.lawall@lip6.fr> - 2015-12-01 12:20 +0100
      Re: [PATCH v2 11/25] coccinelle: nand: detect and correct drivers  embedding an mtd_info object Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-01 13:30 +0100
  [PATCH v2 03/25] cris: nand: make use of mtd_to_nand() where appropriate Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-01 12:20 +0100
  Re: [PATCH v2 12/25] mtd: nand: use the mtd instance embedded in  struct nand_chip Brian Norris <computersforpeace@gmail.com> - 2015-12-01 23:20 +0100
  Re: [PATCH v2 12/25] mtd: nand: use the mtd instance embedded in  struct nand_chip Brian Norris <computersforpeace@gmail.com> - 2015-12-01 23:30 +0100
  [PATCH v3 17/25] mtd: nand: remove useless mtd->priv = chip assignments Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-02 10:00 +0100
  Re: [PATCH v3 bis 12/25] mtd: nand: use the mtd instance embedded in  struct nand_chip Brian Norris <computersforpeace@gmail.com> - 2015-12-09 01:20 +0100
    Re: [PATCH v3 bis 12/25] mtd: nand: use the mtd instance embedded  in struct nand_chip Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-09 09:40 +0100
  Re: [PATCH v2 00/25] mtd: nand: refactor the NAND subsystem (part 1) Brian Norris <computersforpeace@gmail.com> - 2015-12-09 01:40 +0100
    Re: [PATCH v2 00/25] mtd: nand: refactor the NAND subsystem (part  1) Boris Brezillon <boris.brezillon@free-electrons.com> - 2015-12-09 09:20 +0100

csiph-web