diff mbox

mtd: fsl-quadspi: Fix module unbound

Message ID 20150106064931.GB13447@norris-Latitude-E6410
State RFC
Headers show

Commit Message

Brian Norris Jan. 6, 2015, 6:49 a.m. UTC 
Hi,

On Fri, Dec 05, 2014 at 07:14:46PM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> When removing the fsl-quadspi module and running 'cat /proc/mtd' afterwards,
> we see garbage data like:
> 
> $ rmmod  fsl-quadspi
> $ cat /proc/mtd
> dev:    size   erasesize  name
> mtd0: 00000000 00000000 "(null)"
> mtd0: 00000000 00000000 "(null)"
> mtd0: 00000000 00000000 "(null)"
> ...
> mtd0: a22296c6c756e28 00000000 "(null)"
> mtd0: a22296c6c756e28 3064746d "(null)"
> 
> The reason for this is due to the wrong mtd index used in
> mtd_device_unregister() in the remove function.
> 
> We need to keep the mtd index aligned with the one used in the probe function,
> which means we need to take into account the 'has_second_chip' property.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 39763b9..4b468a9 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -227,6 +227,7 @@ struct fsl_qspi {
>  	u32 nor_num;
>  	u32 clk_rate;
>  	unsigned int chip_base_addr; /* We may support two chips. */
> +	bool has_second_chip;
>  };
>  
>  static inline int is_vybrid_qspi(struct fsl_qspi *q)
> @@ -783,7 +784,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  	struct spi_nor *nor;
>  	struct mtd_info *mtd;
>  	int ret, i = 0;
> -	bool has_second_chip = false;
>  	const struct of_device_id *of_id =
>  			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
>  
> @@ -860,14 +860,14 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  		goto irq_failed;
>  
>  	if (of_get_property(np, "fsl,qspi-has-second-chip", NULL))

Huh? Why was this property even needed in the first place? It seems
oddly specific, without actually being very explanatory/descriptive.

> -		has_second_chip = true;
> +		q->has_second_chip = true;
>  
>  	/* iterate the subnodes. */
>  	for_each_available_child_of_node(dev->of_node, np) {
>  		char modalias[40];
>  
>  		/* skip the holes */
> -		if (!has_second_chip)
> +		if (!q->has_second_chip)
>  			i *= 2;

Why do you need to "skip" anything here? It doesn't really look like
you're skpping anything functionally, as this indexing is purely
artificial. So you're just jumping through hoops for no reason.

Can this just be more straightforward by dropping 'has_second_chip' and
indexing straightforwardly? e.g. this patch:


>  
>  		nor = &q->nor[i];
> @@ -943,9 +943,12 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>  	return 0;
>  
>  last_init_failed:
> -	for (i = 0; i < q->nor_num; i++)
> +	for (i = 0; i < q->nor_num; i++) {
> +		/* skip the holes */
> +		if (!q->has_second_chip)
> +			i *= 2;
>  		mtd_device_unregister(&q->mtd[i]);

This error path was already broken, and it still is after your patch.
It's a little less broken after my patch, but you might want to look at
how you unwind from the for_each_available_child_of_node() loop. Also,
note the different between children and available children.

> -
> +	}
>  irq_failed:
>  	clk_disable_unprepare(q->clk);
>  clk_failed:
> @@ -960,8 +963,12 @@ static int fsl_qspi_remove(struct platform_device *pdev)
>  	struct fsl_qspi *q = platform_get_drvdata(pdev);
>  	int i;
>  
> -	for (i = 0; i < q->nor_num; i++)
> +	for (i = 0; i < q->nor_num; i++) {
> +		/* skip the holes */
> +		if (!q->has_second_chip)
> +			i *= 2;
>  		mtd_device_unregister(&q->mtd[i]);
> +	}
>  
>  	/* disable the hardware */
>  	writel(QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);

Brian

Comments

Fabio Estevam Jan. 6, 2015, 12:52 p.m. UTC | #1 
On Tue, Jan 6, 2015 at 4:49 AM, Brian Norris
<computersforpeace@gmail.com> wrote:

> Huh? Why was this property even needed in the first place? It seems
> oddly specific, without actually being very explanatory/descriptive.

Huang, care to comment as you were the one that added it?

>
>> -             has_second_chip = true;
>> +             q->has_second_chip = true;
>>
>>       /* iterate the subnodes. */
>>       for_each_available_child_of_node(dev->of_node, np) {
>>               char modalias[40];
>>
>>               /* skip the holes */
>> -             if (!has_second_chip)
>> +             if (!q->has_second_chip)
>>                       i *= 2;
>
> Why do you need to "skip" anything here? It doesn't really look like
> you're skpping anything functionally, as this indexing is purely
> artificial. So you're just jumping through hoops for no reason.
>
> Can this just be more straightforward by dropping 'has_second_chip' and
> indexing straightforwardly? e.g. this patch:

With your proposed patch I get probe failure:

root@freescale /$ dmesg | grep qspi
[    1.688344] fsl-quadspi 21e4000.qspi: s25fl128s (16384 Kbytes)
[    1.698146] fsl-quadspi 21e4000.qspi: unrecognized JEDEC id bytes: ff, ff, ff
[    1.705350] fsl-quadspi 21e4000.qspi: Freescale QuadSPI probe failed
Brian Norris Jan. 7, 2015, 12:43 a.m. UTC | #2 
On Tue, Jan 06, 2015 at 10:52:00AM -0200, Fabio Estevam wrote:
> On Tue, Jan 6, 2015 at 4:49 AM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> 
> > Huh? Why was this property even needed in the first place? It seems
> > oddly specific, without actually being very explanatory/descriptive.
> 
> Huang, care to comment as you were the one that added it?
> 
> >
> >> -             has_second_chip = true;
> >> +             q->has_second_chip = true;
> >>
> >>       /* iterate the subnodes. */
> >>       for_each_available_child_of_node(dev->of_node, np) {
> >>               char modalias[40];
> >>
> >>               /* skip the holes */
> >> -             if (!has_second_chip)
> >> +             if (!q->has_second_chip)
> >>                       i *= 2;
> >
> > Why do you need to "skip" anything here? It doesn't really look like
> > you're skpping anything functionally, as this indexing is purely
> > artificial. So you're just jumping through hoops for no reason.
> >
> > Can this just be more straightforward by dropping 'has_second_chip' and
> > indexing straightforwardly? e.g. this patch:
> 
> With your proposed patch I get probe failure:
> 
> root@freescale /$ dmesg | grep qspi
> [    1.688344] fsl-quadspi 21e4000.qspi: s25fl128s (16384 Kbytes)
> [    1.698146] fsl-quadspi 21e4000.qspi: unrecognized JEDEC id bytes: ff, ff, ff
> [    1.705350] fsl-quadspi 21e4000.qspi: Freescale QuadSPI probe failed

I think there was one more subtle piece to this driver; it relies on the
implicit layout of the nor[] array for determining a MMIO offset in
fsl_qspi_set_base_addr(). It seems like it would be clearer to avoid
this pointer subtraction.

Brian
Huang Shijie Jan. 7, 2015, 1:12 a.m. UTC | #3 
On Mon, Jan 05, 2015 at 10:49:31PM -0800, Brian Norris wrote:
> Hi,
> 
> On Fri, Dec 05, 2014 at 07:14:46PM -0200, Fabio Estevam wrote:
> > From: Fabio Estevam <fabio.estevam@freescale.com>
> > 
> > When removing the fsl-quadspi module and running 'cat /proc/mtd' afterwards,
> > we see garbage data like:
> > 
> > $ rmmod  fsl-quadspi
> > $ cat /proc/mtd
> > dev:    size   erasesize  name
> > mtd0: 00000000 00000000 "(null)"
> > mtd0: 00000000 00000000 "(null)"
> > mtd0: 00000000 00000000 "(null)"
> > ...
> > mtd0: a22296c6c756e28 00000000 "(null)"
> > mtd0: a22296c6c756e28 3064746d "(null)"
> > 
> > The reason for this is due to the wrong mtd index used in
> > mtd_device_unregister() in the remove function.
> > 
> > We need to keep the mtd index aligned with the one used in the probe function,
> > which means we need to take into account the 'has_second_chip' property.
> > 
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> >  drivers/mtd/spi-nor/fsl-quadspi.c | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> > index 39763b9..4b468a9 100644
> > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > @@ -227,6 +227,7 @@ struct fsl_qspi {
> >  	u32 nor_num;
> >  	u32 clk_rate;
> >  	unsigned int chip_base_addr; /* We may support two chips. */
> > +	bool has_second_chip;
> >  };
> >  
> >  static inline int is_vybrid_qspi(struct fsl_qspi *q)
> > @@ -783,7 +784,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> >  	struct spi_nor *nor;
> >  	struct mtd_info *mtd;
> >  	int ret, i = 0;
> > -	bool has_second_chip = false;
> >  	const struct of_device_id *of_id =
> >  			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
> >  
> > @@ -860,14 +860,14 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> >  		goto irq_failed;
> >  
> >  	if (of_get_property(np, "fsl,qspi-has-second-chip", NULL))
> 
> Huh? Why was this property even needed in the first place? It seems
> oddly specific, without actually being very explanatory/descriptive.
The qspi controller can connect with two SPI flashes at the same time.
Most of the time, we only connect one flash to it.

Use this property can makes the DTS file simple. If we remove this
propery, we have to add a long same child node for the qspi. 

But i think it is ok to remove it, if Brian thinks it is odd.

Hi Fabio, could you please create a correct patch based on Brian's sample patch?

If you do not have time, please tell me. :)

thanks
Huang Shijie
Fabio Estevam Jan. 7, 2015, 1:41 a.m. UTC | #4 
Hi Huang,

On Tue, Jan 6, 2015 at 11:12 PM, Huang Shijie <shijie.huang@intel.com> wrote:

> The qspi controller can connect with two SPI flashes at the same time.
> Most of the time, we only connect one flash to it.
>
> Use this property can makes the DTS file simple. If we remove this
> propery, we have to add a long same child node for the qspi.
>
> But i think it is ok to remove it, if Brian thinks it is odd.
>
> Hi Fabio, could you please create a correct patch based on Brian's sample patch?
>
> If you do not have time, please tell me. :)

It would be nice if you could generate such patch, if possible. Thanks
Huang Shijie Jan. 7, 2015, 1:52 a.m. UTC | #5 
On Tue, Jan 06, 2015 at 11:41:42PM -0200, Fabio Estevam wrote:
> Hi Huang,
> 
> On Tue, Jan 6, 2015 at 11:12 PM, Huang Shijie <shijie.huang@intel.com> wrote:
> 
> > The qspi controller can connect with two SPI flashes at the same time.
> > Most of the time, we only connect one flash to it.
> >
> > Use this property can makes the DTS file simple. If we remove this
> > propery, we have to add a long same child node for the qspi.
> >
> > But i think it is ok to remove it, if Brian thinks it is odd.
> >
> > Hi Fabio, could you please create a correct patch based on Brian's sample patch?
> >
> > If you do not have time, please tell me. :)
> 
> It would be nice if you could generate such patch, if possible. Thanks
okay. Thank you all the same.

I will generate this patch if Allan Xu is busy too.

thanks
Huang Shijie
Fabio Estevam Jan. 7, 2015, 2:54 a.m. UTC | #6 
On Tue, Jan 6, 2015 at 11:52 PM, Huang Shijie <shijie.huang@intel.com> wrote:

>> It would be nice if you could generate such patch, if possible. Thanks
> okay. Thank you all the same.
>
> I will generate this patch if Allan Xu is busy too.

Ok, I think I managed to fix it. Will send a new version shortly.

Thanks
Huang Shijie Jan. 7, 2015, 3:54 a.m. UTC | #7 
On Wed, Jan 07, 2015 at 09:12:42AM +0800, Huang Shijie wrote:
> On Mon, Jan 05, 2015 at 10:49:31PM -0800, Brian Norris wrote:
> > Hi,
> > 
> > On Fri, Dec 05, 2014 at 07:14:46PM -0200, Fabio Estevam wrote:
> > > From: Fabio Estevam <fabio.estevam@freescale.com>
> > > 
> > > When removing the fsl-quadspi module and running 'cat /proc/mtd' afterwards,
> > > we see garbage data like:
> > > 
> > > $ rmmod  fsl-quadspi
> > > $ cat /proc/mtd
> > > dev:    size   erasesize  name
> > > mtd0: 00000000 00000000 "(null)"
> > > mtd0: 00000000 00000000 "(null)"
> > > mtd0: 00000000 00000000 "(null)"
> > > ...
> > > mtd0: a22296c6c756e28 00000000 "(null)"
> > > mtd0: a22296c6c756e28 3064746d "(null)"
> > > 
> > > The reason for this is due to the wrong mtd index used in
> > > mtd_device_unregister() in the remove function.
> > > 
> > > We need to keep the mtd index aligned with the one used in the probe function,
> > > which means we need to take into account the 'has_second_chip' property.
> > > 
> > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > > ---
> > >  drivers/mtd/spi-nor/fsl-quadspi.c | 19 +++++++++++++------
> > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> > > index 39763b9..4b468a9 100644
> > > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > > @@ -227,6 +227,7 @@ struct fsl_qspi {
> > >  	u32 nor_num;
> > >  	u32 clk_rate;
> > >  	unsigned int chip_base_addr; /* We may support two chips. */
> > > +	bool has_second_chip;
> > >  };
> > >  
> > >  static inline int is_vybrid_qspi(struct fsl_qspi *q)
> > > @@ -783,7 +784,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> > >  	struct spi_nor *nor;
> > >  	struct mtd_info *mtd;
> > >  	int ret, i = 0;
> > > -	bool has_second_chip = false;
> > >  	const struct of_device_id *of_id =
> > >  			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
> > >  
> > > @@ -860,14 +860,14 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> > >  		goto irq_failed;
> > >  
> > >  	if (of_get_property(np, "fsl,qspi-has-second-chip", NULL))
> > 
> > Huh? Why was this property even needed in the first place? It seems
> > oddly specific, without actually being very explanatory/descriptive.
> The qspi controller can connect with two SPI flashes at the same time.
> Most of the time, we only connect one flash to it.
> 
sorry, I have forgotten some information. The above comment is wrong.
The qspi controller can connect 4 SPI flashes at the same time.

thanks
Huang Shijie
diff mbox

Patch

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 39763b94f67d..9b11c92ce927 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -782,8 +782,7 @@  static int fsl_qspi_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct spi_nor *nor;
 	struct mtd_info *mtd;
-	int ret, i = 0;
-	bool has_second_chip = false;
+	int ret, i;
 	const struct of_device_id *of_id =
 			of_match_device(fsl_qspi_dt_ids, &pdev->dev);
 
@@ -791,8 +790,8 @@  static int fsl_qspi_probe(struct platform_device *pdev)
 	if (!q)
 		return -ENOMEM;
 
-	q->nor_num = of_get_child_count(dev->of_node);
-	if (!q->nor_num || q->nor_num > FSL_QSPI_MAX_CHIP)
+	ret = of_get_child_count(dev->of_node);
+	if (!ret || ret > FSL_QSPI_MAX_CHIP)
 		return -ENODEV;
 
 	/* find the resources */
@@ -859,19 +858,12 @@  static int fsl_qspi_probe(struct platform_device *pdev)
 	if (ret)
 		goto irq_failed;
 
-	if (of_get_property(np, "fsl,qspi-has-second-chip", NULL))
-		has_second_chip = true;
-
 	/* iterate the subnodes. */
 	for_each_available_child_of_node(dev->of_node, np) {
 		char modalias[40];
 
-		/* skip the holes */
-		if (!has_second_chip)
-			i *= 2;
-
-		nor = &q->nor[i];
-		mtd = &q->mtd[i];
+		nor = &q->nor[q->nor_num];
+		mtd = &q->mtd[q->nor_num];
 
 		nor->mtd = mtd;
 		nor->dev = dev;
@@ -929,7 +921,7 @@  static int fsl_qspi_probe(struct platform_device *pdev)
 		if (nor->page_size > q->devtype_data->txfifo)
 			nor->page_size = q->devtype_data->txfifo;
 
-		i++;
+		q->nor_num++;
 	}
 
 	/* finish the rest init. */