Some fixes for MTD drivers

Some fixes for MTD drivers

[Richard Weinberger]

This series fixes some minor issues I've found while bowsing
in various MTD drivers.

[PATCH 1/6] mtd: r852: Fix device_create_file() usage
[PATCH 2/6] mtd: nandsim: Fix kasprintf() usage
[PATCH 3/6] mtd: cs553x_nand: Fix kasprintf() usage
[PATCH 4/6] mtd: docg3: Don't leak docg3->bbt in error path
[PATCH 5/6] mtd: docg3: Fix kasprintf() usage
[PATCH 6/6] mtd: docg3: Don't do ERR_PTR(0)

Thanks,
//richard

[PATCH 1/6] mtd: r852: Fix device_create_file() usage

[Richard Weinberger]

device_create_file() can fail, therefore we have to
handle this case and abort.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/nand/r852.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/r852.c b/drivers/mtd/nand/r852.c
index baea83f..77e96d2 100644
--- a/drivers/mtd/nand/r852.c
+++ b/drivers/mtd/nand/r852.c
@@ -653,11 +653,15 @@ static int r852_register_nand_device(struct r852_device *dev)
 	if (sm_register_device(dev->mtd, dev->sm))
 		goto error2;
 
-	if (device_create_file(&dev->mtd->dev, &dev_attr_media_type))
+	if (device_create_file(&dev->mtd->dev, &dev_attr_media_type)) {
 		message("can't create media type sysfs attribute");
+		goto error3;
+	}
 
 	dev->card_registred = 1;
 	return 0;
+error3:
+	nand_release(dev->mtd);
 error2:
 	kfree(dev->mtd);
 error1:
-- 
1.8.4.5

Re: [PATCH 1/6] mtd: r852: Fix device_create_file() usage

[Brian Norris]

On Mon, Jun 01, 2015 at 11:10:49PM +0200, Richard Weinberger wrote:
> device_create_file() can fail, therefore we have to
> handle this case and abort.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>

Pushed the first 5. Still looking at the 6th.

[PATCH 2/6] mtd: nandsim: Fix kasprintf() usage

[Richard Weinberger]

kasprintf() used in get_partition_name() does a dynamic
memory allocation and can fail. We have to handle that case.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/nand/nandsim.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index f232427..52c0c1a 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -743,6 +743,11 @@ static int init_nandsim(struct mtd_info *mtd)
 			goto error;
 		}
 		ns->partitions[i].name   = get_partition_name(i);
+		if (!ns->partitions[i].name) {
+			NS_ERR("unable to allocate memory.\n");
+			ret = -ENOMEM;
+			goto error;
+		}
 		ns->partitions[i].offset = next_offset;
 		ns->partitions[i].size   = part_sz;
 		next_offset += ns->partitions[i].size;
@@ -756,6 +761,11 @@ static int init_nandsim(struct mtd_info *mtd)
 			goto error;
 		}
 		ns->partitions[i].name   = get_partition_name(i);
+		if (!ns->partitions[i].name) {
+			NS_ERR("unable to allocate memory.\n");
+			ret = -ENOMEM;
+			goto error;
+		}
 		ns->partitions[i].offset = next_offset;
 		ns->partitions[i].size   = remains;
 		ns->nbparts += 1;
-- 
1.8.4.5

Re: [PATCH 2/6] mtd: nandsim: Fix kasprintf() usage

[Brian Norris]

On Mon, Jun 01, 2015 at 11:10:50PM +0200, Richard Weinberger wrote:
> kasprintf() used in get_partition_name() does a dynamic
> memory allocation and can fail. We have to handle that case.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/nand/nandsim.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
> index f232427..52c0c1a 100644
> --- a/drivers/mtd/nand/nandsim.c
> +++ b/drivers/mtd/nand/nandsim.c
> @@ -743,6 +743,11 @@ static int init_nandsim(struct mtd_info *mtd)
>  			goto error;
>  		}
>  		ns->partitions[i].name   = get_partition_name(i);
> +		if (!ns->partitions[i].name) {
> +			NS_ERR("unable to allocate memory.\n");

Probably don't really need the allocation failure messages. But this
matches the current style, so we can just rip the messages out at
another time.

> +			ret = -ENOMEM;
> +			goto error;
> +		}
>  		ns->partitions[i].offset = next_offset;
>  		ns->partitions[i].size   = part_sz;
>  		next_offset += ns->partitions[i].size;
> @@ -756,6 +761,11 @@ static int init_nandsim(struct mtd_info *mtd)
>  			goto error;
>  		}
>  		ns->partitions[i].name   = get_partition_name(i);
> +		if (!ns->partitions[i].name) {
> +			NS_ERR("unable to allocate memory.\n");

Same here.

> +			ret = -ENOMEM;
> +			goto error;
> +		}
>  		ns->partitions[i].offset = next_offset;
>  		ns->partitions[i].size   = remains;
>  		ns->nbparts += 1;

Brian

Re: [PATCH 2/6] mtd: nandsim: Fix kasprintf() usage

[Joe Perches]

On Tue, 2015-06-16 at 19:07 -0700, Brian Norris wrote:
> On Mon, Jun 01, 2015 at 11:10:50PM +0200, Richard Weinberger wrote:
> > kasprintf() used in get_partition_name() does a dynamic
> > memory allocation and can fail. We have to handle that case.
[]
> > diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
[]
> > @@ -743,6 +743,11 @@ static int init_nandsim(struct mtd_info *mtd)
> >  			goto error;
> >  		}
> >  		ns->partitions[i].name   = get_partition_name(i);
> > +		if (!ns->partitions[i].name) {
> > +			NS_ERR("unable to allocate memory.\n");
> 
> Probably don't really need the allocation failure messages. But this
> matches the current style, so we can just rip the messages out at
> another time.

Maybe that other time can use the more typical
pr_<level> mechanisms instead of NS_<LEVEL> too.

As far as I can tell, the only thing that the
NS_<LEVEL> macros do is prefix "error: " and
"warning: " to the output.

"[nandsim] " could be added via pr_fmt and it
could be changed to "nandsim: " for commonality
with the majority of the kernel logging.

[PATCH 3/6] mtd: cs553x_nand: Fix kasprintf() usage

[Richard Weinberger]

kasprintf() does a dynamic memory allocation and can fail.
We have to handle that case.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/nand/cs553x_nand.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/cs553x_nand.c b/drivers/mtd/nand/cs553x_nand.c
index 88109d3..aec6045 100644
--- a/drivers/mtd/nand/cs553x_nand.c
+++ b/drivers/mtd/nand/cs553x_nand.c
@@ -237,17 +237,23 @@ static int __init cs553x_init_one(int cs, int mmio, unsigned long adr)
 	/* Enable the following for a flash based bad block table */
 	this->bbt_options = NAND_BBT_USE_FLASH;
 
+	new_mtd->name = kasprintf(GFP_KERNEL, "cs553x_nand_cs%d", cs);
+	if (!new_mtd->name) {
+		err = -ENOMEM;
+		goto out_ior;
+	}
+
 	/* Scan to find existence of the device */
 	if (nand_scan(new_mtd, 1)) {
 		err = -ENXIO;
-		goto out_ior;
+		goto out_free;
 	}
 
-	new_mtd->name = kasprintf(GFP_KERNEL, "cs553x_nand_cs%d", cs);
-
 	cs553x_mtd[cs] = new_mtd;
 	goto out;
 
+out_free:
+	kfree(new_mtd->name);
 out_ior:
 	iounmap(this->IO_ADDR_R);
 out_mtd:
-- 
1.8.4.5

[PATCH 4/6] mtd: docg3: Don't leak docg3->bbt in error path

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/devices/docg3.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index be5fb2b..486936b 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -1900,7 +1900,7 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev)
 
 	ret = 0;
 	if (chip_id != (u16)(~chip_id_inv)) {
-		goto nomem3;
+		goto nomem4;
 	}
 
 	switch (chip_id) {
@@ -1910,7 +1910,7 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev)
 		break;
 	default:
 		doc_err("Chip id %04x is not a DiskOnChip G3 chip\n", chip_id);
-		goto nomem3;
+		goto nomem4;
 	}
 
 	doc_set_driver_info(chip_id, mtd);
@@ -1919,6 +1919,8 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev)
 	doc_reload_bbt(docg3);
 	return mtd;
 
+nomem4:
+	kfree(docg3->bbt);
 nomem3:
 	kfree(mtd);
 nomem2:
-- 
1.8.4.5

[PATCH 5/6] mtd: docg3: Fix kasprintf() usage

[Richard Weinberger]

kasprintf() does a dynamic memory allocation and can fail.
We have to handle that case.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/devices/docg3.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 486936b..5e67b4a 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -1815,7 +1815,7 @@ static void doc_dbg_unregister(struct docg3 *docg3)
  * @chip_id: The chip ID of the supported chip
  * @mtd: The structure to fill
  */
-static void __init doc_set_driver_info(int chip_id, struct mtd_info *mtd)
+static int __init doc_set_driver_info(int chip_id, struct mtd_info *mtd)
 {
 	struct docg3 *docg3 = mtd->priv;
 	int cfg;
@@ -1828,6 +1828,8 @@ static void __init doc_set_driver_info(int chip_id, struct mtd_info *mtd)
 	case DOC_CHIPID_G3:
 		mtd->name = kasprintf(GFP_KERNEL, "docg3.%d",
 				      docg3->device_id);
+		if (!mtd->name)
+			return -ENOMEM;
 		docg3->max_block = 2047;
 		break;
 	}
@@ -1850,6 +1852,8 @@ static void __init doc_set_driver_info(int chip_id, struct mtd_info *mtd)
 	mtd->_block_isbad = doc_block_isbad;
 	mtd->ecclayout = &docg3_oobinfo;
 	mtd->ecc_strength = DOC_ECC_BCH_T;
+
+	return 0;
 }
 
 /**
@@ -1913,7 +1917,9 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev)
 		goto nomem4;
 	}
 
-	doc_set_driver_info(chip_id, mtd);
+	ret = doc_set_driver_info(chip_id, mtd);
+	if (ret)
+		goto nomem4;
 
 	doc_hamming_ecc_init(docg3, DOC_LAYOUT_OOB_PAGEINFO_SZ);
 	doc_reload_bbt(docg3);
-- 
1.8.4.5

[PATCH 6/6] mtd: docg3: Don't do ERR_PTR(0)

[Richard Weinberger]

Don't return a obfuscated null pointer using ERR_PTR(0).
If the no device is found clearly return -ENODEV.
This makes the code more clear and matches the comment
of doc_probe_device().

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/devices/docg3.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 5e67b4a..630e29a 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -1902,7 +1902,7 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev)
 	chip_id = doc_register_readw(docg3, DOC_CHIPID);
 	chip_id_inv = doc_register_readw(docg3, DOC_CHIPID_INV);
 
-	ret = 0;
+	ret = -ENODEV;
 	if (chip_id != (u16)(~chip_id_inv)) {
 		goto nomem4;
 	}
@@ -2068,13 +2068,10 @@ static int __init docg3_probe(struct platform_device *pdev)
 		mtd = doc_probe_device(cascade, floor, dev);
 		if (IS_ERR(mtd)) {
 			ret = PTR_ERR(mtd);
-			goto err_probe;
-		}
-		if (!mtd) {
-			if (floor == 0)
-				goto notfound;
-			else
+			if (ret == -ENODEV && floor == 0)
 				continue;
+			else
+				goto err_probe;
 		}
 		cascade->floors[floor] = mtd;
 		ret = mtd_device_parse_register(mtd, part_probes, NULL, NULL,
@@ -2091,10 +2088,9 @@ static int __init docg3_probe(struct platform_device *pdev)
 	doc_dbg_register(cascade->floors[0]->priv);
 	return 0;
 
-notfound:
-	ret = -ENODEV;
-	dev_info(dev, "No supported DiskOnChip found\n");
 err_probe:
+	if (ret == -ENODEV)
+		dev_info(dev, "No supported DiskOnChip found\n");
 	free_bch(cascade->bch);
 	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
 		if (cascade->floors[floor])
-- 
1.8.4.5

Re: [PATCH 6/6] mtd: docg3: Don't do ERR_PTR(0)

[Brian Norris]

On Mon, Jun 01, 2015 at 11:10:54PM +0200, Richard Weinberger wrote:
> Don't return a obfuscated null pointer using ERR_PTR(0).
> If the no device is found clearly return -ENODEV.
> This makes the code more clear and matches the comment
> of doc_probe_device().
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---

Have you tested this patch?

>  drivers/mtd/devices/docg3.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index 5e67b4a..630e29a 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -1902,7 +1902,7 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev)
>  	chip_id = doc_register_readw(docg3, DOC_CHIPID);
>  	chip_id_inv = doc_register_readw(docg3, DOC_CHIPID_INV);
>  
> -	ret = 0;
> +	ret = -ENODEV;
>  	if (chip_id != (u16)(~chip_id_inv)) {
>  		goto nomem4;
>  	}
> @@ -2068,13 +2068,10 @@ static int __init docg3_probe(struct platform_device *pdev)
>  		mtd = doc_probe_device(cascade, floor, dev);
>  		if (IS_ERR(mtd)) {
>  			ret = PTR_ERR(mtd);
> -			goto err_probe;
> -		}
> -		if (!mtd) {
> -			if (floor == 0)
> -				goto notfound;
> -			else
> +			if (ret == -ENODEV && floor == 0)

I think you might have changed the logic when refactoring here. I think
the right refactoring would be the following, no?

			if (ret == -ENODEV && floor != 0)
				continue;
			else
				goto err_probe;

>  				continue;
> +			else
> +				goto err_probe;
>  		}
>  		cascade->floors[floor] = mtd;
>  		ret = mtd_device_parse_register(mtd, part_probes, NULL, NULL,
> @@ -2091,10 +2088,9 @@ static int __init docg3_probe(struct platform_device *pdev)
>  	doc_dbg_register(cascade->floors[0]->priv);
>  	return 0;
>  
> -notfound:
> -	ret = -ENODEV;
> -	dev_info(dev, "No supported DiskOnChip found\n");
>  err_probe:
> +	if (ret == -ENODEV)
> +		dev_info(dev, "No supported DiskOnChip found\n");
>  	free_bch(cascade->bch);
>  	for (floor = 0; floor < DOC_MAX_NBFLOORS; floor++)
>  		if (cascade->floors[floor])

Brian

Re: [PATCH 6/6] mtd: docg3: Don't do ERR_PTR(0)

[Richard Weinberger]

Am 17.06.2015 um 20:41 schrieb Brian Norris:
> On Mon, Jun 01, 2015 at 11:10:54PM +0200, Richard Weinberger wrote:
>> Don't return a obfuscated null pointer using ERR_PTR(0).
>> If the no device is found clearly return -ENODEV.
>> This makes the code more clear and matches the comment
>> of doc_probe_device().
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
> 
> Have you tested this patch?

nah, I don't own such a device.

>>  drivers/mtd/devices/docg3.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
>> index 5e67b4a..630e29a 100644
>> --- a/drivers/mtd/devices/docg3.c
>> +++ b/drivers/mtd/devices/docg3.c
>> @@ -1902,7 +1902,7 @@ doc_probe_device(struct docg3_cascade *cascade, int floor, struct device *dev)
>>  	chip_id = doc_register_readw(docg3, DOC_CHIPID);
>>  	chip_id_inv = doc_register_readw(docg3, DOC_CHIPID_INV);
>>  
>> -	ret = 0;
>> +	ret = -ENODEV;
>>  	if (chip_id != (u16)(~chip_id_inv)) {
>>  		goto nomem4;
>>  	}
>> @@ -2068,13 +2068,10 @@ static int __init docg3_probe(struct platform_device *pdev)
>>  		mtd = doc_probe_device(cascade, floor, dev);
>>  		if (IS_ERR(mtd)) {
>>  			ret = PTR_ERR(mtd);
>> -			goto err_probe;
>> -		}
>> -		if (!mtd) {
>> -			if (floor == 0)
>> -				goto notfound;
>> -			else
>> +			if (ret == -ENODEV && floor == 0)
> 
> I think you might have changed the logic when refactoring here. I think
> the right refactoring would be the following, no?
> 
> 			if (ret == -ENODEV && floor != 0)
> 				continue;
> 			else
> 				goto err_probe;
> 

You are right, the logic changed. ;-\
Please drop this patch, I'll resend it for 4.3.

Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] mtd: docg3: Don't do ERR_PTR(0)

[Robert Jarzmik]

Richard Weinberger <richard@nod.at> writes:

> Am 17.06.2015 um 20:41 schrieb Brian Norris:
>> On Mon, Jun 01, 2015 at 11:10:54PM +0200, Richard Weinberger wrote:
>>> Don't return a obfuscated null pointer using ERR_PTR(0).
>>> If the no device is found clearly return -ENODEV.
>>> This makes the code more clear and matches the comment
>>> of doc_probe_device().
>>>
>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>> ---
>> 
>> Have you tested this patch?
>
> nah, I don't own such a device.
But I do. If you resend a patch, please Cc me. You can even ask for a test from
time to time if you want a confirmation, I have a 2 floors docg3 device.

Cheers.

--
Robert

Re: [PATCH 6/6] mtd: docg3: Don't do ERR_PTR(0)

[Brian Norris]

Hi Robert,

On Tue, Jun 23, 2015 at 10:41:33PM +0200, Robert Jarzmik wrote:
> Richard Weinberger <richard@nod.at> writes:
> 
> > Am 17.06.2015 um 20:41 schrieb Brian Norris:
> >> Have you tested this patch?
> >
> > nah, I don't own such a device.
> But I do. If you resend a patch, please Cc me. You can even ask for a test from
> time to time if you want a confirmation, I have a 2 floors docg3 device.

Do you want to be on a MAINTAINERS entry for this driver?

Brian

Re: [PATCH 6/6] mtd: docg3: Don't do ERR_PTR(0)

[Robert Jarzmik]

Brian Norris <computersforpeace@gmail.com> writes:

> Hi Robert,
>
> On Tue, Jun 23, 2015 at 10:41:33PM +0200, Robert Jarzmik wrote:
>> Richard Weinberger <richard@nod.at> writes:
>> 
>> > Am 17.06.2015 um 20:41 schrieb Brian Norris:
>> >> Have you tested this patch?
>> >
>> > nah, I don't own such a device.
>> But I do. If you resend a patch, please Cc me. You can even ask for a test from
>> time to time if you want a confirmation, I have a 2 floors docg3 device.
>
> Do you want to be on a MAINTAINERS entry for this driver?
Sure.

Here is the patch, after the scissors.

Cheers.

-- 
Robert

---8<---
>From 20002639eac1bd7a81b0613c4bd15ae7522c269d Mon Sep 17 00:00:00 2001
From: Robert Jarzmik <robert.jarzmik@free.fr>
Date: Thu, 25 Jun 2015 19:07:48 +0200
Subject: [PATCH] MAINTAINERS: mtd: docg3: add docg3 maintainer

Add myself as maintainer of the NAND based MSystems DiskOnChip G3
driver.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aca2886..87d87c7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6687,6 +6687,12 @@ T:	git git://linuxtv.org/anttip/media_tree.git
 S:	Maintained
 F:	drivers/media/usb/msi2500/
 
+MSYSTEMS DISKONCHIP G3 MTD DRIVER
+M:	Robert Jarzmik <robert.jarzmik@free.fr>
+L:	linux-mtd@lists.infradead.org
+S:	Maintained
+F:	drivers/mtd/devices/docg3*
+
 MT9M032 APTINA SENSOR DRIVER
 M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
 L:	linux-media@vger.kernel.org
-- 
2.1.4

Re: [PATCH 6/6] mtd: docg3: Don't do ERR_PTR(0)

[Brian Norris]

On Thu, Jun 25, 2015 at 07:14:18PM +0200, Robert Jarzmik wrote:
> From 20002639eac1bd7a81b0613c4bd15ae7522c269d Mon Sep 17 00:00:00 2001
> From: Robert Jarzmik <robert.jarzmik@free.fr>
> Date: Thu, 25 Jun 2015 19:07:48 +0200
> Subject: [PATCH] MAINTAINERS: mtd: docg3: add docg3 maintainer
> 
> Add myself as maintainer of the NAND based MSystems DiskOnChip G3
> driver.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

Queued, thanks