[PATCH 1/2] mtd: fsl_ifc_nand: Use devm_* throughout driver

[PATCH 1/2] mtd: fsl_ifc_nand: Use devm_* throughout driver

[Aaron Sierra]

For consistency, use managed resources for allocations and remaps
throughout the driver.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/mtd/nand/fsl_ifc_nand.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 2338124..7861909 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -997,9 +997,6 @@ static int fsl_ifc_chip_remove(struct fsl_ifc_mtd *priv)
 
 	kfree(priv->mtd.name);
 
-	if (priv->vbase)
-		iounmap(priv->vbase);
-
 	ifc_nand_ctrl->chips[priv->bank] = NULL;
 
 	return 0;
@@ -1062,7 +1059,8 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
 
 	mutex_lock(&fsl_ifc_nand_mutex);
 	if (!fsl_ifc_ctrl_dev->nand) {
-		ifc_nand_ctrl = kzalloc(sizeof(*ifc_nand_ctrl), GFP_KERNEL);
+		ifc_nand_ctrl = devm_kzalloc(&dev->dev,
+					sizeof(*ifc_nand_ctrl), GFP_KERNEL);
 		if (!ifc_nand_ctrl) {
 			mutex_unlock(&fsl_ifc_nand_mutex);
 			return -ENOMEM;
@@ -1085,7 +1083,7 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
 	priv->ctrl = fsl_ifc_ctrl_dev;
 	priv->dev = &dev->dev;
 
-	priv->vbase = ioremap(res.start, resource_size(&res));
+	priv->vbase = devm_ioremap(priv->dev, res.start, resource_size(&res));
 	if (!priv->vbase) {
 		dev_err(priv->dev, "%s: failed to map chip region\n", __func__);
 		ret = -ENOMEM;
@@ -1148,10 +1146,8 @@ static int fsl_ifc_nand_remove(struct platform_device *dev)
 
 	mutex_lock(&fsl_ifc_nand_mutex);
 	ifc_nand_ctrl->counter--;
-	if (!ifc_nand_ctrl->counter) {
+	if (!ifc_nand_ctrl->counter)
 		fsl_ifc_ctrl_dev->nand = NULL;
-		kfree(ifc_nand_ctrl);
-	}
 	mutex_unlock(&fsl_ifc_nand_mutex);
 
 	return 0;
-- 
1.9.1

[PATCH 2/2] mtd: fsl_ifc_nand: Probe partitions OF node

[Aaron Sierra]

Previously, the OF node defining the IFC NAND controller was being
passed to mtd_device_parse_register(), not the node defining the
partitions. This resulted in no OF-defined partitions being created.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/mtd/nand/fsl_ifc_nand.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 7861909..9aeb146 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -1029,7 +1029,6 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
 	struct device_node *node = dev->dev.of_node;
 	struct mtd_part_parser_data ppdata;
 
-	ppdata.of_node = dev->dev.of_node;
 	if (!fsl_ifc_ctrl_dev || !fsl_ifc_ctrl_dev->regs)
 		return -ENODEV;
 	ifc = fsl_ifc_ctrl_dev->regs;
@@ -1126,9 +1125,13 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
 
 	/* First look for RedBoot table or partitions on the command
 	 * line, these take precedence over device tree information */
+	ppdata.of_node = of_get_next_child(dev->dev.of_node, NULL);
 	mtd_device_parse_register(&priv->mtd, part_probe_types, &ppdata,
 						NULL, 0);
 
+	if (ppdata.of_node)
+		of_node_put(ppdata.of_node);
+
 	dev_info(priv->dev, "IFC NAND device at 0x%llx, bank %d\n",
 		 (unsigned long long)res.start, priv->bank);
 	return 0;
-- 
1.9.1

Re: [PATCH 2/2] mtd: fsl_ifc_nand: Probe partitions OF node

[Scott Wood]

On Fri, 2014-08-15 at 19:47 -0500, Aaron Sierra wrote:
> Previously, the OF node defining the IFC NAND controller was being
> passed to mtd_device_parse_register(), not the node defining the
> partitions. This resulted in no OF-defined partitions being created.

This driver probes on "fsl,ifc-nand", not "fsl,ifc".  So how is it
getting the controller node?

What does the device tree look like in which you're seeing this happen?

-Scott

Re: [PATCH 2/2] mtd: fsl_ifc_nand: Probe partitions OF node

[Aaron Sierra]

----- Original Message -----
> From: "Scott Wood" <scottwood@freescale.com>
> Sent: Wednesday, August 20, 2014 6:26:25 PM
> 
> On Fri, 2014-08-15 at 19:47 -0500, Aaron Sierra wrote:
> > Previously, the OF node defining the IFC NAND controller was being
> > passed to mtd_device_parse_register(), not the node defining the
> > partitions. This resulted in no OF-defined partitions being created.
> 
> This driver probes on "fsl,ifc-nand", not "fsl,ifc".  So how is it
> getting the controller node?
> 
> What does the device tree look like in which you're seeing this happen?
> 
> -Scott
> 

This is the node that is defined in my T1042 device tree:

		nand0@4,0 {
			#address-cells = <0>;
			#size-cells = <0>;
			compatible = "fsl,ifc-nand";
                        reg = <4 0x0 0x040000>;

			nand@0 {
				#address-cells = <1>;
				#size-cells = <2>;
				compatible = "micron,mt29f32g08";

				partition@0 {
					label = "NAND Filesystem";
					reg = <0 0x1 0x00000000>;
				};
			};
		};

It is based on a node used previously with the fsl_upm NAND driver on
a P5020:

		upm@4,0 {
			#address-cells = <0>;
			#size-cells = <0>;
			compatible = "fsl,upm-nand";
                        reg = <4 0x0 0x080000>;
                        fsl,upm-addr-offset = <0x10>;
                        fsl,upm-cmd-offset = <0x08>;
                        fsl,upm-wait-flags = <0x1>;
                        chip-delay = <50>;

			nand@0 {
				#address-cells = <1>;
				#size-cells = <2>;
				compatible = "micron,mt29f32g08";

				partition@0 {
					label = "NAND Filesystem";
					reg = <0 0x1 0x00000000>;
				};
			};
		};

The patch that I submitted is based on code from the fsl_upm driver:

	flash_np = of_get_next_child(upm_np, NULL);
	if (!flash_np)
		return -ENODEV;

[snip]

	ppdata.of_node = flash_np;
	ret = mtd_device_parse_register(&fun->mtd, NULL, &ppdata, NULL, 0);
err:
	of_node_put(flash_np);

-Aaron

Re: [PATCH 2/2] mtd: fsl_ifc_nand: Probe partitions OF node

[Scott Wood]

On Wed, 2014-08-20 at 18:45 -0500, Aaron Sierra wrote:
> ----- Original Message -----
> > From: "Scott Wood" <scottwood@freescale.com>
> > Sent: Wednesday, August 20, 2014 6:26:25 PM
> > 
> > On Fri, 2014-08-15 at 19:47 -0500, Aaron Sierra wrote:
> > > Previously, the OF node defining the IFC NAND controller was being
> > > passed to mtd_device_parse_register(), not the node defining the
> > > partitions. This resulted in no OF-defined partitions being created.
> > 
> > This driver probes on "fsl,ifc-nand", not "fsl,ifc".  So how is it
> > getting the controller node?
> > 
> > What does the device tree look like in which you're seeing this happen?
> > 
> > -Scott
> > 
> 
> This is the node that is defined in my T1042 device tree:
> 
> 		nand0@4,0 {
> 			#address-cells = <0>;
> 			#size-cells = <0>;
> 			compatible = "fsl,ifc-nand";
>                         reg = <4 0x0 0x040000>;
> 
> 			nand@0 {
> 				#address-cells = <1>;
> 				#size-cells = <2>;
> 				compatible = "micron,mt29f32g08";
> 
> 				partition@0 {
> 					label = "NAND Filesystem";
> 					reg = <0 0x1 0x00000000>;
> 				};
> 			};
> 		};

This is wrong.  You shouldn't have a separate nand@0 node.

Your patch will break partition detection on all of the existing IFC
device trees.

> It is based on a node used previously with the fsl_upm NAND driver on
> a P5020:

Why would you base it on a upm node rather than on an ifc node, or on
the IFC binding document
(Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt)?

-Scott

Re: [PATCH 2/2] mtd: fsl_ifc_nand: Probe partitions OF node

[Aaron Sierra]

----- Original Message -----
> From: "Scott Wood" <scottwood@freescale.com>
> Sent: Wednesday, August 20, 2014 6:59:56 PM
> 
> On Wed, 2014-08-20 at 18:45 -0500, Aaron Sierra wrote:
> > ----- Original Message -----
> > > From: "Scott Wood" <scottwood@freescale.com>
> > > Sent: Wednesday, August 20, 2014 6:26:25 PM
> > > 
> > > On Fri, 2014-08-15 at 19:47 -0500, Aaron Sierra wrote:
> > > > Previously, the OF node defining the IFC NAND controller was being
> > > > passed to mtd_device_parse_register(), not the node defining the
> > > > partitions. This resulted in no OF-defined partitions being created.
> > > 
> > > This driver probes on "fsl,ifc-nand", not "fsl,ifc".  So how is it
> > > getting the controller node?
> > > 
> > > What does the device tree look like in which you're seeing this happen?
> > > 
> > > -Scott
> > > 
> > 
> > This is the node that is defined in my T1042 device tree:
> > 
> > 		nand0@4,0 {
> > 			#address-cells = <0>;
> > 			#size-cells = <0>;
> > 			compatible = "fsl,ifc-nand";
> >                         reg = <4 0x0 0x040000>;
> > 
> > 			nand@0 {
> > 				#address-cells = <1>;
> > 				#size-cells = <2>;
> > 				compatible = "micron,mt29f32g08";
> > 
> > 				partition@0 {
> > 					label = "NAND Filesystem";
> > 					reg = <0 0x1 0x00000000>;
> > 				};
> > 			};
> > 		};
> 
> This is wrong.  You shouldn't have a separate nand@0 node.
> 
> Your patch will break partition detection on all of the existing IFC
> device trees.
> 
> > It is based on a node used previously with the fsl_upm NAND driver on
> > a P5020:
> 
> Why would you base it on a upm node rather than on an ifc node, or on
> the IFC binding document
> (Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt)?
> 
> -Scott
> 

Ah, I was under the wrong impression that there was a common MTD
partitioning format. I didn't realize it was defined by each
driver. Thanks for clearing that up.

-Aaron

Re: [PATCH 2/2] mtd: fsl_ifc_nand: Probe partitions OF node

[Scott Wood]

On Wed, 2014-08-20 at 19:15 -0500, Aaron Sierra wrote:
> Ah, I was under the wrong impression that there was a common MTD
> partitioning format. I didn't realize it was defined by each
> driver. Thanks for clearing that up.

The partition nodes should take the same form regardless of controller
type, but not the node that contains the partition nodes.

-Scott

Re: [PATCH 1/2] mtd: fsl_ifc_nand: Use devm_* throughout driver

[Brian Norris]

Hi Aaron,

Hmm, you're not the only one to send a patch like this. But I'm not sure
if it's correct; this driver is written awkwardly.

On Fri, Aug 15, 2014 at 07:46:44PM -0500, Aaron Sierra wrote:
> For consistency, use managed resources for allocations and remaps
> throughout the driver.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/mtd/nand/fsl_ifc_nand.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index 2338124..7861909 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -997,9 +997,6 @@ static int fsl_ifc_chip_remove(struct fsl_ifc_mtd *priv)
>  
>  	kfree(priv->mtd.name);
>  
> -	if (priv->vbase)
> -		iounmap(priv->vbase);
> -
>  	ifc_nand_ctrl->chips[priv->bank] = NULL;
>  
>  	return 0;
> @@ -1062,7 +1059,8 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
>  
>  	mutex_lock(&fsl_ifc_nand_mutex);
>  	if (!fsl_ifc_ctrl_dev->nand) {
> -		ifc_nand_ctrl = kzalloc(sizeof(*ifc_nand_ctrl), GFP_KERNEL);
> +		ifc_nand_ctrl = devm_kzalloc(&dev->dev,
> +					sizeof(*ifc_nand_ctrl), GFP_KERNEL);

This structure is static, and it looks like it's written with an attempt
of sharing the same controller structure across potentially many
instances of the device (multiple banks / chip selects?). And it has a
refcount for freeing the struct, but that refcount is wrong (see below).

>  		if (!ifc_nand_ctrl) {
>  			mutex_unlock(&fsl_ifc_nand_mutex);
>  			return -ENOMEM;
> @@ -1085,7 +1083,7 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
>  	priv->ctrl = fsl_ifc_ctrl_dev;
>  	priv->dev = &dev->dev;
>  
> -	priv->vbase = ioremap(res.start, resource_size(&res));
> +	priv->vbase = devm_ioremap(priv->dev, res.start, resource_size(&res));
>  	if (!priv->vbase) {
>  		dev_err(priv->dev, "%s: failed to map chip region\n", __func__);
>  		ret = -ENOMEM;
> @@ -1148,10 +1146,8 @@ static int fsl_ifc_nand_remove(struct platform_device *dev)
>  
>  	mutex_lock(&fsl_ifc_nand_mutex);
>  	ifc_nand_ctrl->counter--;
> -	if (!ifc_nand_ctrl->counter) {
> +	if (!ifc_nand_ctrl->counter)

Notice here, that there is an attempt at refcounting the ->nand
structure. It is wrong (see how 'counter' is never incremented, only
decremented). I don't know if anyone uses this driver in a "removed"
context (e.g., rmmod or device unbinding), but this patch is potentially
circumventing the (incorrect) refcounting, and instead will free the
memory when it's possibly still used by another device.

IMO, it's better to have a potential memory leak than a potential
use-after-free, so I will not apply this patch. Feel free to provide an
actual bugfix for this driver to fix the refcounting instead, though! Or
work with the original authors/users to see if this driver actually
needs all the global/static info that's being used here. Perhaps the
driver could be refactored.

>  		fsl_ifc_ctrl_dev->nand = NULL;
> -		kfree(ifc_nand_ctrl);
> -	}
>  	mutex_unlock(&fsl_ifc_nand_mutex);
>  
>  	return 0;

BTW, I see that an earlier incarnation of this type of patch says it was
based on an automated semantic patch. I'd suggest taking a hard look at
the automated tools you're using, so you don't inadverently add more
bugs while you're making "cleanups."

Regards,
Brian