mtd: remove driver-core BUS_ID_SIZE

PATCH Kay Sievers 4 participants 8 messages Apr 16, 2009 – Jun 2, 2009

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Kay Sievers

author

2009-04-16 16:26 UTC #0 lore raw

From: Kay Sievers <[email protected]>
Subject: mtd: remove driver-core BUS_ID_SIZE

The name size limit is gone from the driver-core, the BUS_ID_SIZE value will be removed.

Cc [email protected]

Acked-by Greg Kroah-Hartman <[email protected]>

Signed-off-by Kay Sievers <[email protected]>

---
 drivers/mtd/nand/txx9ndfmc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mtd/nand/txx9ndfmc.c +1 -1

@@ -64,7 +64,7 @@ struct txx9ndfmc_priv {
 	struct nand_chip chip;
 	struct mtd_info mtd;
 	int cs;
-	char mtdname[BUS_ID_SIZE + 2];
+	char mtdname[20 + 2];
 };
 
 #define MAX_TXX9NDFMC_DEV	4

Artem Bityutskiy

2009-04-17 07:25 UTC #1 lore raw

Quoted 17 lines · Kay Sievers

From: Kay Sievers <[email protected]>
Subject: mtd: remove driver-core BUS_ID_SIZE
The name size limit is gone from the driver-core, the BUS_ID_SIZE value will be removed.
Cc [email protected]
Cc [email protected]
Acked-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Kay Sievers <[email protected]>
---
 drivers/mtd/nand/txx9ndfmc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mtd/nand/txx9ndfmc.c struct txx9ndfmc_priv { · line 70

Quoted 8 lines

drivers/mtd/nand/txx9ndfmc.c +1 -1

@@ -64,7 +64,7 @@ struct txx9ndfmc_priv {
 	struct nand_chip chip;
 	struct mtd_info mtd;
 	int cs;
-	char mtdname[BUS_ID_SIZE + 2];
+	char mtdname[20 + 2];
 };
 
 #define MAX_TXX9NDFMC_DEV	4

Pushed, to l2-mtd-2.6.git. This does not guarantee anything, it just mean I'll ping dwmw2 to pull from that tree.

Signature · 2 lines

Best regards,
Artem Bityutskiy (Битюцкий Артём)

David Woodhouse

2009-05-29 13:11 UTC #2 lore raw

In reply to Kay Sievers

Quoted 17 lines · Kay Sievers

From: Kay Sievers <[email protected]>
Subject: mtd: remove driver-core BUS_ID_SIZE
The name size limit is gone from the driver-core, the BUS_ID_SIZE value will be removed.
Cc [email protected]
Cc [email protected]
Acked-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Kay Sievers <[email protected]>
---
 drivers/mtd/nand/txx9ndfmc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mtd/nand/txx9ndfmc.c struct txx9ndfmc_priv { · line 70

Quoted 8 lines

drivers/mtd/nand/txx9ndfmc.c +1 -1

@@ -64,7 +64,7 @@ struct txx9ndfmc_priv {
 	struct nand_chip chip;
 	struct mtd_info mtd;
 	int cs;
-	char mtdname[BUS_ID_SIZE + 2];
+	char mtdname[20 + 2];
 };
 
 #define MAX_TXX9NDFMC_DEV	4

Not so cunning. You're explicitly noting that the limit on the input string size is gone, and yet you do nothing about the code which uses it...

		if (plat->ch_mask != 1) {
			txx9_priv->cs = i;
			sprintf(txx9_priv->mtdname, "%s.%u",
				dev_name(&dev->dev), i);
		} else {
			txx9_priv->cs = -1;
			strcpy(txx9_priv->mtdname, dev_name(&dev->dev));
		}

Signature · 2 lines

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

David Woodhouse

2009-05-29 13:21 UTC #3 lore raw

Quoted 31 lines · David Woodhouse

Quoted 17 lines · Kay Sievers
From: Kay Sievers <[email protected]>
Subject: mtd: remove driver-core BUS_ID_SIZE
The name size limit is gone from the driver-core, the BUS_ID_SIZE value will be removed.
Cc [email protected]
Cc [email protected]
Acked-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Kay Sievers <[email protected]>
---
 drivers/mtd/nand/txx9ndfmc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/nand/txx9ndfmc.c struct txx9ndfmc_priv { · line 70
Quoted 8 lines
drivers/mtd/nand/txx9ndfmc.c +1 -1
@@ -64,7 +64,7 @@ struct txx9ndfmc_priv {
 	struct nand_chip chip;
 	struct mtd_info mtd;
 	int cs;
-	char mtdname[BUS_ID_SIZE + 2];
+	char mtdname[20 + 2];
 };
 
 #define MAX_TXX9NDFMC_DEV	4
Not so cunning. You're explicitly noting that the limit on the input string size is gone, and yet you do nothing about the code which uses it...

Quoted 8 lines

		if (plat->ch_mask != 1) {
			txx9_priv->cs = i;
			sprintf(txx9_priv->mtdname, "%s.%u",
				dev_name(&dev->dev), i);
		} else {
			txx9_priv->cs = -1;
			strcpy(txx9_priv->mtdname, dev_name(&dev->dev));
		}

This might work better. Nemoto-san, can you test and confirm?

drivers/mtd/nand/txx9ndfmc.c +14 -3

index 8124792..6329382 100644
@@ -64,7 +64,7 @@ struct txx9ndfmc_priv {
 	struct nand_chip chip;
 	struct mtd_info mtd;
 	int cs;
-	char mtdname[BUS_ID_SIZE + 2];
+	char *mtdname;
 };
 
 #define MAX_TXX9NDFMC_DEV	4

@@ -333,12 +333,21 @@ static int __init txx9ndfmc_probe(struct platform_device *dev)
 		txx9_priv->dev = dev;
 
 		if (plat->ch_mask != 1) {
+			char *devname = dev_name(&dev->dev);
 			txx9_priv->cs = i;
+			txx9_priv->mtdname = kmalloc(strlen(devname) + 3,
+						     GFP_KERNEL);
+			if (!txx9_mtdname) {
+				kfree(txx9_priv);
+				dev_err(&dev->dev,
+					"Unable to allocate TXx9 NDFMC MTD device name.\n");
+				continue;
+			}
 			sprintf(txx9_priv->mtdname, "%s.%u",
-				dev_name(&dev->dev), i);
+				devname, i);
 		} else {
 			txx9_priv->cs = -1;
-			strcpy(txx9_priv->mtdname, dev_name(&dev->dev));
+			txx9_priv->mtdname = dev_name(&dev->dev);
 		}
 		if (plat->wide_mask & (1 << i))
 			chip->options |= NAND_BUSWIDTH_16;

@@ -385,6 +394,8 @@ static int __exit txx9ndfmc_remove(struct platform_device *dev)
 		kfree(drvdata->parts[i]);
 #endif
 		del_mtd_device(mtd);
+		if (txx9_priv->mtdname != dev_name(&dev->dev))
+			kfree(txx9_priv->mtdname);
 		kfree(txx9_priv);
 	}
 	return 0;

Signature · 1 line

dwmw2

Kay Sievers

author

2009-05-29 13:26 UTC #4 lore raw

Quoted 1 line · David Woodhouse

This might work better. Nemoto-san, can you test and confirm?

Looks good to me.

Quoted 7 lines

drivers/mtd/nand/txx9ndfmc.c +0 -0
if (plat->ch_mask != 1) {
+ char *devname = dev_name(&dev->dev); txx9_priv->cs = i;
+ txx9_priv->mtdname = kmalloc(strlen(devname) + 3,
+ GFP_KERNEL);

kasprintf() might be nicer here?

Thanks,
Kay

David Woodhouse

2009-05-29 13:33 UTC #5 lore raw

Quoted 7 lines · Kay Sievers

Quoted 1 line · David Woodhouse
This might work better. Nemoto-san, can you test and confirm?
Looks good to me.
Quoted 1 line
drivers/mtd/nand/txx9ndfmc.c +0 -0

Quoted 7 lines

drivers/mtd/nand/txx9ndfmc.c

Quoted 5 lines

               if (plat->ch_mask != 1) {

drivers/mtd/nand/txx9ndfmc.c +3 -0

+                       char *devname = dev_name(&dev->dev);
                       txx9_priv->cs = i;
+                       txx9_priv->mtdname = kmalloc(strlen(devname) + 3,
+                                                    GFP_KERNEL);

kasprintf() might be nicer here?

True; thanks. (Er, how have I managed not to know that asprintf() exists, for the last 20-odd years?)

drivers/mtd/nand/txx9ndfmc.c +12 -4

index 8124792..5f919e6 100644
@@ -64,7 +64,7 @@ struct txx9ndfmc_priv {
 	struct nand_chip chip;
 	struct mtd_info mtd;
 	int cs;
-	char mtdname[BUS_ID_SIZE + 2];
+	const char *mtdname;
 };
 
 #define MAX_TXX9NDFMC_DEV	4

@@ -334,11 +334,17 @@ static int __init txx9ndfmc_probe(struct platform_device *dev)
 
 		if (plat->ch_mask != 1) {
 			txx9_priv->cs = i;
-			sprintf(txx9_priv->mtdname, "%s.%u",
-				dev_name(&dev->dev), i);
+			txx9_priv->mtdname = kasprintf(GFP_KERNEL, "%s.%u",
+						       dev_name(&dev->dev), i);
+			if (!txx9_priv->mtdname) {
+				kfree(txx9_priv);
+				dev_err(&dev->dev,
+					"Unable to allocate TXx9 NDFMC MTD device name.\n");
+				continue;
+			}
 		} else {
 			txx9_priv->cs = -1;
-			strcpy(txx9_priv->mtdname, dev_name(&dev->dev));
+			txx9_priv->mtdname = dev_name(&dev->dev);
 		}
 		if (plat->wide_mask & (1 << i))
 			chip->options |= NAND_BUSWIDTH_16;

@@ -385,6 +391,8 @@ static int __exit txx9ndfmc_remove(struct platform_device *dev)
 		kfree(drvdata->parts[i]);
 #endif
 		del_mtd_device(mtd);
+		if (txx9_priv->mtdname != dev_name(&dev->dev))
+			kfree(txx9_priv->mtdname);
 		kfree(txx9_priv);
 	}
 	return 0;

Signature · 2 lines

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation

Atsushi Nemoto

2009-05-29 15:52 UTC #6 lore raw

Quoted 1 line · David Woodhouse

Quoted 1 line
Quoted 1 line
This might work better. Nemoto-san, can you test and confirm?

Sure, I will test it on next week.

Quoted 14 lines

Quoted 9 lines
drivers/mtd/nand/txx9ndfmc.c static int __init txx9ndfmc_probe(struct platform_device *dev)
Quoted 7 lines
drivers/mtd/nand/txx9ndfmc.c +3 -0
 
               if (plat->ch_mask != 1) {
+                       char *devname = dev_name(&dev->dev);
                       txx9_priv->cs = i;
+                       txx9_priv->mtdname = kmalloc(strlen(devname) + 3,
+                                                    GFP_KERNEL);
kasprintf() might be nicer here?
True; thanks. (Er, how have I managed not to know that asprintf() exists, for the last 20-odd years?)
drivers/mtd/nand/txx9ndfmc.c +0 -0

...

drivers/mtd/nand/txx9ndfmc.c static int __init txx9ndfmc_probe(struct platform_device *dev) · line 347

Quoted 4 lines

drivers/mtd/nand/txx9ndfmc.c +1 -1

 		} else {
 			txx9_priv->cs = -1;
-			strcpy(txx9_priv->mtdname, dev_name(&dev->dev));
+			txx9_priv->mtdname = dev_name(&dev->dev);

Use kstrdup() here, otherwise nand_scan() failure will cause memory leak.

Or you can add conditional kfree into failure path of nand_scan(), but I think saving few bytes is not worth to complicate an error (and removal) path.

drivers/mtd/nand/txx9ndfmc.c static int __exit txx9ndfmc_remove(struct platform_device *dev) · line 395

Quoted 9 lines

drivers/mtd/nand/txx9ndfmc.c +2 -0

 		}
 		if (plat->wide_mask & (1 << i))
 			chip->options |= NAND_BUSWIDTH_16;

@@ -385,6 +391,8 @@ static int __exit txx9ndfmc_remove(struct platform_device *dev)
 		kfree(drvdata->parts[i]);
 #endif
 		del_mtd_device(mtd);
+		if (txx9_priv->mtdname != dev_name(&dev->dev))
+			kfree(txx9_priv->mtdname);

If you did kstrdup() as above, you can just call kfree unconditionally.

---
Atsushi Nemoto

Atsushi Nemoto

2009-06-02 15:09 UTC #7 lore raw

In reply to David Woodhouse

Quoted 1 line · David Woodhouse

Quoted 1 line
Quoted 1 line
This might work better. Nemoto-san, can you test and confirm?

...

drivers/mtd/nand/txx9ndfmc.c

Quoted 7 lines

Quoted 1 line
kasprintf() might be nicer here?
True; thanks. (Er, how have I managed not to know that asprintf() exists, for the last 20-odd years?)
drivers/mtd/nand/txx9ndfmc.c +0 -0
index 8124792..5f919e6 100644

Your latest patch with slight fix works fine.

Here is diff against your patch. Please fold into it. Thanks.

drivers/mtd/nand/txx9ndfmc.c +3 -3

index 5f919e6..fa0458e 100644
@@ -344,12 +344,13 @@ static int __init txx9ndfmc_probe(struct platform_device *dev)
 			}
 		} else {
 			txx9_priv->cs = -1;
-			txx9_priv->mtdname = dev_name(&dev->dev);
+			txx9_priv->mtdname = kstrdup(dev_name(&dev->dev));
 		}
 		if (plat->wide_mask & (1 << i))
 			chip->options |= NAND_BUSWIDTH_16;
 
 		if (nand_scan(mtd, 1)) {
+			kfree(txx9_priv->mtdname)
 			kfree(txx9_priv);
 			continue;
 		}

@@ -391,8 +392,7 @@ static int __exit txx9ndfmc_remove(struct platform_device *dev)
 		kfree(drvdata->parts[i]);
 #endif
 		del_mtd_device(mtd);
-		if (txx9_priv->mtdname != dev_name(&dev->dev))
-			kfree(txx9_priv->mtdname);
+		kfree(txx9_priv->mtdname);
 		kfree(txx9_priv);
 	}
 	return 0;

---
Atsushi Nemoto