[PATCH 1/8] mtd: Simplify partitions

[PATCH 1/8] mtd: Simplify partitions

[Sascha Hauer]

Embed the partition information in struct mtd_info. This makes the
mtd partition code simpler.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/partition.c | 75 ++++++++++++++++++++-----------------------------
 include/linux/mtd/mtd.h |  5 +++-
 2 files changed, 34 insertions(+), 46 deletions(-)

diff --git a/drivers/mtd/partition.c b/drivers/mtd/partition.c
index 85f486d..7431fc1 100644
--- a/drivers/mtd/partition.c
+++ b/drivers/mtd/partition.c
@@ -4,29 +4,19 @@
 #include <linux/err.h>
 #include <linux/mtd/mtd.h>
 
-struct mtd_part {
-	struct mtd_info mtd;
-	struct mtd_info *master;
-	uint64_t offset;
-	struct list_head list;
-};
-
-#define PART(x)  ((struct mtd_part *)(x))
-
 static int mtd_part_read(struct mtd_info *mtd, loff_t from, size_t len,
                 size_t *retlen, u_char *buf)
 {
-	struct mtd_part *part = PART(mtd);
 	struct mtd_ecc_stats stats;
 	int res;
 
-	stats = part->master->ecc_stats;
+	stats = mtd->master->ecc_stats;
 
 	if (from >= mtd->size)
 		len = 0;
 	else if (from + len > mtd->size)
 		len = mtd->size - from;
-	res = part->master->read(part->master, from + part->offset,
+	res = mtd->master->read(mtd->master, from + mtd->master_offset,
 				len, retlen, buf);
 	return res;
 }
@@ -34,57 +24,52 @@ static int mtd_part_read(struct mtd_info *mtd, loff_t from, size_t len,
 static int mtd_part_write(struct mtd_info *mtd, loff_t to, size_t len,
                 size_t *retlen, const u_char *buf)
 {
-	struct mtd_part *part = PART(mtd);
-
 	if (!(mtd->flags & MTD_WRITEABLE))
 		return -EROFS;
 	if (to >= mtd->size)
 		len = 0;
 	else if (to + len > mtd->size)
 		len = mtd->size - to;
-	return part->master->write(part->master, to + part->offset,
+	return mtd->master->write(mtd->master, to + mtd->master_offset,
 					len, retlen, buf);
 }
 
 static int mtd_part_erase(struct mtd_info *mtd, struct erase_info *instr)
 {
-	struct mtd_part *part = PART(mtd);
 	int ret;
 
 	if (!(mtd->flags & MTD_WRITEABLE))
 		return -EROFS;
 	if (instr->addr >= mtd->size)
 		return -EINVAL;
-	instr->addr += part->offset;
-	ret = part->master->erase(part->master, instr);
+	instr->addr += mtd->master_offset;
+	ret = mtd->master->erase(mtd->master, instr);
 	if (ret) {
 		if (instr->fail_addr != 0xffffffff)
-			instr->fail_addr -= part->offset;
-		instr->addr -= part->offset;
+			instr->fail_addr -= mtd->master_offset;
+		instr->addr -= mtd->master_offset;
 	}
 	return ret;
 }
 
 static int mtd_part_block_isbad(struct mtd_info *mtd, loff_t ofs)
 {
-	struct mtd_part *part = PART(mtd);
 	if (ofs >= mtd->size)
 		return -EINVAL;
-	ofs += part->offset;
-	return mtd_block_isbad(part->master, ofs);
+	ofs += mtd->master_offset;
+	return mtd_block_isbad(mtd->master, ofs);
 }
 
 static int mtd_part_block_markbad(struct mtd_info *mtd, loff_t ofs)
 {
-	struct mtd_part *part = PART(mtd);
 	int res;
 
 	if (!(mtd->flags & MTD_WRITEABLE))
 		return -EROFS;
 	if (ofs >= mtd->size)
 		return -EINVAL;
-	ofs += part->offset;
-	res = part->master->block_markbad(part->master, ofs);
+	ofs += mtd->master_offset;
+	res = mtd->master->block_markbad(mtd->master, ofs);
 	if (!res)
 		mtd->ecc_stats.badblocks++;
 	return res;
@@ -93,14 +78,12 @@ static int mtd_part_block_markbad(struct mtd_info *mtd, loff_t ofs)
 struct mtd_info *mtd_add_partition(struct mtd_info *mtd, off_t offset, size_t size,
 		unsigned long flags, const char *name)
 {
-	struct mtd_part *slave;
-	struct mtd_info *slave_mtd;
+	struct mtd_info *part;
 	int start = 0, end = 0, i;
 
-	slave = xzalloc(sizeof(*slave));
-	slave_mtd = &slave->mtd;
+	part = xzalloc(sizeof(*part));
 
-	memcpy(slave_mtd, mtd, sizeof(*slave));
+	memcpy(part, mtd, sizeof(*part));
 
 	/*
 	 * find the number of eraseregions the partition includes.
@@ -118,26 +101,28 @@ struct mtd_info *mtd_add_partition(struct mtd_info *mtd, off_t offset, size_t si
 			end = i;
 	}
 
-	slave_mtd->numeraseregions = end - start;
+	part->numeraseregions = end - start;
 
-	slave_mtd->read = mtd_part_read;
-	slave_mtd->write = mtd_part_write;
-	slave_mtd->erase = mtd_part_erase;
-	slave_mtd->block_isbad = mtd->block_isbad ? mtd_part_block_isbad : NULL;
-	slave_mtd->block_markbad = mtd->block_markbad ? mtd_part_block_markbad : NULL;
-	slave_mtd->size = size;
-	slave_mtd->name = strdup(name);
+	part->read = mtd_part_read;
+	part->write = mtd_part_write;
+	part->erase = mtd_part_erase;
+	part->block_isbad = mtd->block_isbad ? mtd_part_block_isbad : NULL;
+	part->block_markbad = mtd->block_markbad ? mtd_part_block_markbad : NULL;
+	part->size = size;
+	part->name = strdup(name);
 
-	slave->offset = offset;
-	slave->master = mtd;
+	part->master_offset = offset;
+	part->master = mtd;
 
-	return slave_mtd;
+	return part;
 }
 
-void mtd_del_partition(struct mtd_info *mtd)
+int mtd_del_partition(struct mtd_info *part)
 {
-	struct mtd_part *part = PART(mtd);
+	if (!part->master)
+		return -EINVAL;
 
-	free(mtd->name);
 	free(part);
+
+	return 0;
 }
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 402e497..347443b 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -217,6 +217,9 @@ struct mtd_info {
 	/* If true erasing bad blocks is allowed, this is set via a device parameter */
 	bool allow_erasebad;
 	int p_allow_erasebad;
+
+	struct mtd_info *master;
+	uint64_t master_offset;
 };
 
 int mtd_erase(struct mtd_info *mtd, struct erase_info *instr);
@@ -272,7 +275,7 @@ struct mtd_notifier {
 
 struct mtd_info *mtd_add_partition(struct mtd_info *mtd, off_t offset, size_t size,
 		unsigned long flags, const char *name);
-void mtd_del_partition(struct mtd_info *mtd);
+int mtd_del_partition(struct mtd_info *mtd);
 
 extern void register_mtd_user (struct mtd_notifier *new);
 extern int unregister_mtd_user (struct mtd_notifier *old);
-- 
1.8.5.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/8] device: init bus list

[Sascha Hauer]

bus_list is only initialized when the device has a bus, but it
needs to be initialized in unregister_device, so initialize the
list unconditionally.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/base/driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 4250fb0..b5fe346 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -154,6 +154,7 @@ int register_device(struct device_d *new_device)
 	INIT_LIST_HEAD(&new_device->cdevs);
 	INIT_LIST_HEAD(&new_device->parameters);
 	INIT_LIST_HEAD(&new_device->active);
+	INIT_LIST_HEAD(&new_device->bus_list);
 
 	if (new_device->bus) {
 		if (!new_device->parent)
-- 
1.8.5.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/8] device: remove parameters when unregistering a device

[Sascha Hauer]

Otherwise we loose memory on each device_unregister. The ethernet
code used to do this before calling unregister_device. This can
now be removed.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/base/driver.c | 2 ++
 net/eth.c             | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index b5fe346..37560fd 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -182,6 +182,8 @@ int unregister_device(struct device_d *old_dev)
 
 	dev_dbg(old_dev, "unregister\n");
 
+	dev_remove_parameters(old_dev);
+
 	if (old_dev->driver)
 		old_dev->bus->remove(old_dev);
 
diff --git a/net/eth.c b/net/eth.c
index 37dd9e0..8c4798c 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -380,8 +380,6 @@ void eth_unregister(struct eth_device *edev)
 	if (edev == eth_current)
 		eth_current = NULL;
 
-	dev_remove_parameters(&edev->dev);
-
 	if (IS_ENABLED(CONFIG_OFDEVICE) && edev->nodepath)
 		free(edev->nodepath);
 
-- 
1.8.5.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/8] mtd: erase_info may be modified in mtd_erase

[Sascha Hauer]

erase_info may be modified by the mtd partition code, so
set the address each time we call mtd_erase.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/core.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/core.c b/drivers/mtd/core.c
index 6db1c6d..e4def2a 100644
--- a/drivers/mtd/core.c
+++ b/drivers/mtd/core.c
@@ -146,6 +146,7 @@ static int mtd_op_erase(struct cdev *cdev, size_t count, loff_t offset)
 {
 	struct mtd_info *mtd = cdev->priv;
 	struct erase_info erase;
+	uint32_t addr;
 	int ret;
 
 	ret = mtd_erase_align(mtd, &count, &offset);
@@ -154,7 +155,7 @@ static int mtd_op_erase(struct cdev *cdev, size_t count, loff_t offset)
 
 	memset(&erase, 0, sizeof(erase));
 	erase.mtd = mtd;
-	erase.addr = offset;
+	addr = offset;
 
 	if (!mtd->block_isbad) {
 		erase.len = count;
@@ -164,22 +165,24 @@ static int mtd_op_erase(struct cdev *cdev, size_t count, loff_t offset)
 	erase.len = mtd->erasesize;
 
 	while (count > 0) {
-		dev_dbg(cdev->dev, "erase %d %d\n", erase.addr, erase.len);
+		dev_dbg(cdev->dev, "erase %d %d\n", addr, erase.len);
 
 		if (!mtd->allow_erasebad)
-			ret = mtd_block_isbad(mtd, erase.addr);
+			ret = mtd_block_isbad(mtd, addr);
 		else
 			ret = 0;
 
+		erase.addr = addr;
+
 		if (ret > 0) {
-			printf("Skipping bad block at 0x%08x\n", erase.addr);
+			printf("Skipping bad block at 0x%08x\n", addr);
 		} else {
 			ret = mtd_erase(mtd, &erase);
 			if (ret)
 				return ret;
 		}
 
-		erase.addr += mtd->erasesize;
+		addr += mtd->erasesize;
 		count -= count > mtd->erasesize ? mtd->erasesize : count;
 	}
 
-- 
1.8.5.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/8] mtd: Only call of_parse_partitions when the mtd has a parent

[Sascha Hauer]

mtd->parent is optional so we can't derefence mtd->parent->device_node
without checking mtd->parent.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/core.c b/drivers/mtd/core.c
index e4def2a..ecdd6c7 100644
--- a/drivers/mtd/core.c
+++ b/drivers/mtd/core.c
@@ -407,7 +407,9 @@ int add_mtd_device(struct mtd_info *mtd, char *devname, int device_id)
 	}
 
 	devfs_create(&mtd->cdev);
-	of_parse_partitions(&mtd->cdev, mtd->parent->device_node);
+
+	if (mtd->parent)
+		of_parse_partitions(&mtd->cdev, mtd->parent->device_node);
 
 	list_for_each_entry(hook, &mtd_register_hooks, hook)
 		if (hook->add_mtd_device)
-- 
1.8.5.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/8] mtd: partition: only copy selected fields to partition

[Sascha Hauer]

struct mtd_info contains members which should not be copied
to the new partition, like for example the class_dev, so
only copy selected members of struct mtd_info to the partition.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/partition.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/partition.c b/drivers/mtd/partition.c
index 7431fc1..450f281 100644
--- a/drivers/mtd/partition.c
+++ b/drivers/mtd/partition.c
@@ -83,7 +83,18 @@ struct mtd_info *mtd_add_partition(struct mtd_info *mtd, off_t offset, size_t si
 
 	part = xzalloc(sizeof(*part));
 
-	memcpy(part, mtd, sizeof(*part));
+	part->type = mtd->type;
+	part->flags = mtd->flags;
+	part->parent = mtd->parent;
+	part->erasesize = mtd->erasesize;
+	part->writesize = mtd->writesize;
+	part->writebufsize = mtd->writebufsize;
+	part->oobsize = mtd->oobsize;
+	part->oobavail = mtd->oobavail;
+	part->bitflip_threshold = mtd->bitflip_threshold;
+	part->ecclayout = mtd->ecclayout;
+	part->ecc_strength = mtd->ecc_strength;
+	part->subpage_sft = mtd->subpage_sft;
 
 	/*
 	 * find the number of eraseregions the partition includes.
-- 
1.8.5.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 7/8] mtd: register mtd partitions as real mtd devices

[Sascha Hauer]

So far mtd partitions were mtd devices, but these were not registered.
This patch changes this. mtd partitions are now registered like real
mtd devices. This makes them part of the device hierarchy.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/partition.c |  9 +++++++++
 fs/devfs-core.c         | 30 ++++++++++++++----------------
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/partition.c b/drivers/mtd/partition.c
index 450f281..ed3dfa0 100644
--- a/drivers/mtd/partition.c
+++ b/drivers/mtd/partition.c
@@ -125,6 +125,11 @@ struct mtd_info *mtd_add_partition(struct mtd_info *mtd, off_t offset, size_t si
 	part->master_offset = offset;
 	part->master = mtd;
 
+	if (!strncmp(mtd->cdev.name, name, strlen(mtd->cdev.name)))
+		part->cdev.partname = xstrdup(name + strlen(mtd->cdev.name) + 1);
+
+	add_mtd_device(part, part->name, DEVICE_ID_SINGLE);
+
 	return part;
 }
 
@@ -133,6 +138,10 @@ int mtd_del_partition(struct mtd_info *part)
 	if (!part->master)
 		return -EINVAL;
 
+	del_mtd_device(part);
+
+	free(part->cdev.partname);
+	free(part->name);
 	free(part);
 
 	return 0;
diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index 44f0169..bd6d482 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -276,6 +276,15 @@ struct cdev *devfs_add_partition(const char *devname, loff_t offset, loff_t size
 	if (offset + size > cdev->size)
 		return ERR_PTR(-EINVAL);
 
+	if (IS_ENABLED(CONFIG_PARTITION_NEED_MTD) && cdev->mtd) {
+		struct mtd_info *mtd;
+
+		mtd = mtd_add_partition(cdev->mtd, offset, size, flags, name);
+		if (IS_ERR(mtd))
+			return (void *)mtd;
+		return 0;
+	}
+
 	new = xzalloc(sizeof (*new));
 	new->name = strdup(name);
 	if (!strncmp(devname, name, strlen(devname)))
@@ -287,17 +296,6 @@ struct cdev *devfs_add_partition(const char *devname, loff_t offset, loff_t size
 	new->dev = cdev->dev;
 	new->flags = flags | DEVFS_IS_PARTITION;
 
-#ifdef CONFIG_PARTITION_NEED_MTD
-	if (cdev->mtd) {
-		new->mtd = mtd_add_partition(cdev->mtd, offset, size, flags, name);
-		if (IS_ERR(new->mtd)) {
-			int ret = PTR_ERR(new->mtd);
-			free(new);
-			return ERR_PTR(ret);
-		}
-	}
-#endif
-
 	devfs_create(new);
 
 	return new;
@@ -312,16 +310,16 @@ int devfs_del_partition(const char *name)
 	if (!cdev)
 		return -ENOENT;
 
+	if (IS_ENABLED(CONFIG_PARTITION_NEED_MTD) && cdev->mtd) {
+		ret = mtd_del_partition(cdev->mtd);
+		return ret;
+	}
+
 	if (!(cdev->flags & DEVFS_IS_PARTITION))
 		return -EINVAL;
 	if (cdev->flags & DEVFS_PARTITION_FIXED)
 		return -EPERM;
 
-#ifdef CONFIG_PARTITION_NEED_MTD
-	if (cdev->mtd)
-		mtd_del_partition(cdev->mtd);
-#endif
-
 	ret = devfs_remove(cdev);
 	if (ret)
 		return ret;
-- 
1.8.5.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 8/8] ubi: register ubi devices and volumes as devices

[Sascha Hauer]

struct ubi_device and struct ubi_volume have devices attached
to them. Register them to make them part of the barebox device
hierarchy.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/ubi/build.c | 13 ++++++++++++-
 drivers/mtd/ubi/cdev.c  |  2 ++
 drivers/mtd/ubi/vmt.c   | 12 ++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 202b046..9c75442 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -157,10 +157,18 @@ static int uif_init(struct ubi_device *ubi, int *ref)
 	*ref = 0;
 	sprintf(ubi->ubi_name, UBI_NAME_STR "%d", ubi->ubi_num);
 
+	sprintf(ubi->dev.name, "ubi");
+	ubi->dev.id = DEVICE_ID_DYNAMIC;
+	ubi->dev.parent = &ubi->mtd->class_dev;
+
+	err = register_device(&ubi->dev);
+	if (err)
+		goto out_unreg;
+
 	err = ubi_cdev_add(ubi);
 	if (err) {
 		ubi_err("cannot add character device");
-		goto out_unreg;
+		goto out_dev;
 	}
 
 	for (i = 0; i < ubi->vtbl_slots; i++)
@@ -177,6 +185,8 @@ static int uif_init(struct ubi_device *ubi, int *ref)
 out_volumes:
 	kill_volumes(ubi);
 	devfs_remove(&ubi->cdev);
+out_dev:
+	unregister_device(&ubi->dev);
 out_unreg:
 	ubi_err("cannot initialize UBI %s, error %d", ubi->ubi_name, err);
 	return err;
@@ -193,6 +203,7 @@ out_unreg:
 static void uif_close(struct ubi_device *ubi)
 {
 	kill_volumes(ubi);
+	unregister_device(&ubi->dev);
 	ubi_cdev_remove(ubi);
 }
 
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 129f2e2..e8beaa5 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -179,6 +179,7 @@ int ubi_volume_cdev_add(struct ubi_device *ubi, struct ubi_volume *vol)
 	cdev->name = asprintf("ubi%d.%s", ubi->ubi_num, vol->name);
 	cdev->priv = priv;
 	cdev->size = vol->used_bytes;
+	cdev->dev = &vol->dev;
 	printf("registering %s as /dev/%s\n", vol->name, cdev->name);
 	ret = devfs_create(cdev);
 	if (ret) {
@@ -199,6 +200,7 @@ void ubi_volume_cdev_remove(struct ubi_volume *vol)
 	list_del(&vol->list);
 
 	devfs_remove(cdev);
+	unregister_device(&vol->dev);
 	kfree(cdev->name);
 	kfree(priv);
 }
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 65339ef..e7a5d9a 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -147,6 +147,11 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req)
 			vol->last_eb_bytes = vol->usable_leb_size;
 	}
 
+	sprintf(vol->dev.name, "%s.%s", dev_name(&ubi->dev), vol->name);
+	vol->dev.id = DEVICE_ID_SINGLE;
+	vol->dev.parent = &ubi->dev;
+	register_device(&vol->dev);
+
 	/* Register character device for the volume */
 	err = ubi_volume_cdev_add(ubi, vol);
 	if (err) {
@@ -190,6 +195,7 @@ out_sysfs:
 out_mapping:
 	if (do_free)
 		kfree(vol->eba_tbl);
+	unregister_device(&vol->dev);
 out_acc:
 	ubi->rsvd_pebs -= vol->reserved_pebs;
 	ubi->avail_pebs += vol->reserved_pebs;
@@ -427,6 +433,11 @@ int ubi_add_volume(struct ubi_device *ubi, struct ubi_volume *vol)
 
 	dbg_gen("add volume");
 
+	sprintf(vol->dev.name, "%s.%s", dev_name(&ubi->dev), vol->name);
+	vol->dev.id = DEVICE_ID_SINGLE;
+	vol->dev.parent = &ubi->dev;
+	register_device(&vol->dev);
+
 	/* Register character device for the volume */
 	err = ubi_volume_cdev_add(ubi, vol);
 	if (err) {
@@ -454,6 +465,7 @@ void ubi_free_volume(struct ubi_device *ubi, struct ubi_volume *vol)
 	dbg_gen("free volume %d", vol->vol_id);
 
 	ubi->volumes[vol->vol_id] = NULL;
+	unregister_device(&vol->dev);
 	devfs_remove(&vol->cdev);
 }
 
-- 
1.8.5.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/8] mtd: Simplify partitions

[Alexander Aring]

Hi Sascha,

only some small nitpicks.

On Thu, Feb 13, 2014 at 11:25:28AM +0100, Sascha Hauer wrote:
> Embed the partition information in struct mtd_info. This makes the
> mtd partition code simpler.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/partition.c | 75 ++++++++++++++++++++-----------------------------
>  include/linux/mtd/mtd.h |  5 +++-
>  2 files changed, 34 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/mtd/partition.c b/drivers/mtd/partition.c
> index 85f486d..7431fc1 100644
> --- a/drivers/mtd/partition.c
> +++ b/drivers/mtd/partition.c
> @@ -4,29 +4,19 @@
>  #include <linux/err.h>
>  #include <linux/mtd/mtd.h>
>  
...
> +	struct mtd_info *part;
>  	int start = 0, end = 0, i;
>  
> -	slave = xzalloc(sizeof(*slave));
> -	slave_mtd = &slave->mtd;
> +	part = xzalloc(sizeof(*part));
>  
> -	memcpy(slave_mtd, mtd, sizeof(*slave));
> +	memcpy(part, mtd, sizeof(*part));

Maybe we should fixup this commit with patch:

[PATCH 6/8] mtd: partition: only copy selected fields to partition
>  
>  	/*
>  	 * find the number of eraseregions the partition includes.
> @@ -118,26 +101,28 @@ struct mtd_info *mtd_add_partition(struct mtd_info *mtd, off_t offset, size_t si
>  			end = i;
>  	}
>  
> -	slave_mtd->numeraseregions = end - start;
> +	part->numeraseregions = end - start;
>  
> -	slave_mtd->read = mtd_part_read;
> -	slave_mtd->write = mtd_part_write;
> -	slave_mtd->erase = mtd_part_erase;
> -	slave_mtd->block_isbad = mtd->block_isbad ? mtd_part_block_isbad : NULL;
> -	slave_mtd->block_markbad = mtd->block_markbad ? mtd_part_block_markbad : NULL;
> -	slave_mtd->size = size;
> -	slave_mtd->name = strdup(name);
> +	part->read = mtd_part_read;
> +	part->write = mtd_part_write;
> +	part->erase = mtd_part_erase;
> +	part->block_isbad = mtd->block_isbad ? mtd_part_block_isbad : NULL;
> +	part->block_markbad = mtd->block_markbad ? mtd_part_block_markbad : NULL;
> +	part->size = size;
> +	part->name = strdup(name);
>  
> -	slave->offset = offset;
> -	slave->master = mtd;
> +	part->master_offset = offset;
> +	part->master = mtd;
>  
> -	return slave_mtd;
> +	return part;
>  }
>  
> -void mtd_del_partition(struct mtd_info *mtd)
> +int mtd_del_partition(struct mtd_info *part)
>  {
> -	struct mtd_part *part = PART(mtd);
> +	if (!part->master)
> +		return -EINVAL;
>  
> -	free(mtd->name);
I think this should be free(part->name);

...
Yea I know it's only a bootloader but I can't stop to say something to
this. :-(

>  	free(part);
> +
> +	return 0;
>  }
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 402e497..347443b 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -217,6 +217,9 @@ struct mtd_info {
>  	/* If true erasing bad blocks is allowed, this is set via a device parameter */
>  	bool allow_erasebad;
>  	int p_allow_erasebad;
> +
> +	struct mtd_info *master;
> +	uint64_t master_offset;
Currently we have only u_int32_t like for total mtd size. Do you
plan a support for mtd devices which a greater than 4GB? :-)

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/8] mtd: Simplify partitions

[Sascha Hauer]

On Thu, Feb 13, 2014 at 12:08:58PM +0100, Alexander Aring wrote:
> Hi Sascha,
> 
> only some small nitpicks.
> 
> On Thu, Feb 13, 2014 at 11:25:28AM +0100, Sascha Hauer wrote:
> > Embed the partition information in struct mtd_info. This makes the
> > mtd partition code simpler.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/mtd/partition.c | 75 ++++++++++++++++++++-----------------------------
> >  include/linux/mtd/mtd.h |  5 +++-
> >  2 files changed, 34 insertions(+), 46 deletions(-)
> > 
> > diff --git a/drivers/mtd/partition.c b/drivers/mtd/partition.c
> > index 85f486d..7431fc1 100644
> > --- a/drivers/mtd/partition.c
> > +++ b/drivers/mtd/partition.c
> > @@ -4,29 +4,19 @@
> >  #include <linux/err.h>
> >  #include <linux/mtd/mtd.h>
> >  
> ...
> > +	struct mtd_info *part;
> >  	int start = 0, end = 0, i;
> >  
> > -	slave = xzalloc(sizeof(*slave));
> > -	slave_mtd = &slave->mtd;
> > +	part = xzalloc(sizeof(*part));
> >  
> > -	memcpy(slave_mtd, mtd, sizeof(*slave));
> > +	memcpy(part, mtd, sizeof(*part));
> 
> Maybe we should fixup this commit with patch:
> 
> [PATCH 6/8] mtd: partition: only copy selected fields to partition

I don't think so. Both change the same line, but do quite different
things.

> > -	struct mtd_part *part = PART(mtd);
> > +	if (!part->master)
> > +		return -EINVAL;
> >  
> > -	free(mtd->name);
> I think this should be free(part->name);

What do you mean? The line you are referring to is removed in this
patch.

> 
> ...
> Yea I know it's only a bootloader but I can't stop to say something to
> this. :-(
> 
> >  	free(part);
> > +
> > +	return 0;
> >  }
> > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > index 402e497..347443b 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -217,6 +217,9 @@ struct mtd_info {
> >  	/* If true erasing bad blocks is allowed, this is set via a device parameter */
> >  	bool allow_erasebad;
> >  	int p_allow_erasebad;
> > +
> > +	struct mtd_info *master;
> > +	uint64_t master_offset;
> Currently we have only u_int32_t like for total mtd size. Do you
> plan a support for mtd devices which a greater than 4GB? :-)

Well the mtd layer is directly from Linux which supports devices > 4GB,
so sooner or later we'll do aswell. Anyway, since the master device
doesn't support 64bit there's no point in making this variable 64bit
now.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/8] mtd: Simplify partitions

[Alexander Aring]

On Thu, Feb 13, 2014 at 02:34:30PM +0100, Sascha Hauer wrote:
> On Thu, Feb 13, 2014 at 12:08:58PM +0100, Alexander Aring wrote:
> > Hi Sascha,
> > 
> > only some small nitpicks.
> > 
> > On Thu, Feb 13, 2014 at 11:25:28AM +0100, Sascha Hauer wrote:
> > > Embed the partition information in struct mtd_info. This makes the
> > > mtd partition code simpler.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/mtd/partition.c | 75 ++++++++++++++++++++-----------------------------
> > >  include/linux/mtd/mtd.h |  5 +++-
> > >  2 files changed, 34 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/partition.c b/drivers/mtd/partition.c
> > > index 85f486d..7431fc1 100644
> > > --- a/drivers/mtd/partition.c
> > > +++ b/drivers/mtd/partition.c
> > > @@ -4,29 +4,19 @@
> > >  #include <linux/err.h>
> > >  #include <linux/mtd/mtd.h>
> > >  
> > ...
> > > +	struct mtd_info *part;
> > >  	int start = 0, end = 0, i;
> > >  
> > > -	slave = xzalloc(sizeof(*slave));
> > > -	slave_mtd = &slave->mtd;
> > > +	part = xzalloc(sizeof(*part));
> > >  
> > > -	memcpy(slave_mtd, mtd, sizeof(*slave));
> > > +	memcpy(part, mtd, sizeof(*part));
> > 
> > Maybe we should fixup this commit with patch:
> > 
> > [PATCH 6/8] mtd: partition: only copy selected fields to partition
> 
> I don't think so. Both change the same line, but do quite different
> things.
> 
ok.
> > > -	struct mtd_part *part = PART(mtd);
> > > +	if (!part->master)
> > > +		return -EINVAL;
> > >  
> > > -	free(mtd->name);
> > I think this should be free(part->name);
> 
> What do you mean? The line you are referring to is removed in this
> patch.
> 
I mean the part->name = strdup(mtd->name) in add partition function.

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/8] mtd: Simplify partitions

[Sascha Hauer]

On Thu, Feb 13, 2014 at 03:01:55PM +0100, Alexander Aring wrote:
> On Thu, Feb 13, 2014 at 02:34:30PM +0100, Sascha Hauer wrote:
> > On Thu, Feb 13, 2014 at 12:08:58PM +0100, Alexander Aring wrote:
> > > Hi Sascha,
> > > 
> > > only some small nitpicks.
> > > 
> > > On Thu, Feb 13, 2014 at 11:25:28AM +0100, Sascha Hauer wrote:
> > > > Embed the partition information in struct mtd_info. This makes the
> > > > mtd partition code simpler.
> > > > 
> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > ---
> > > >  drivers/mtd/partition.c | 75 ++++++++++++++++++++-----------------------------
> > > >  include/linux/mtd/mtd.h |  5 +++-
> > > >  2 files changed, 34 insertions(+), 46 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/partition.c b/drivers/mtd/partition.c
> > > > index 85f486d..7431fc1 100644
> > > > --- a/drivers/mtd/partition.c
> > > > +++ b/drivers/mtd/partition.c
> > > > @@ -4,29 +4,19 @@
> > > >  #include <linux/err.h>
> > > >  #include <linux/mtd/mtd.h>
> > > >  
> > > ...
> > > > +	struct mtd_info *part;
> > > >  	int start = 0, end = 0, i;
> > > >  
> > > > -	slave = xzalloc(sizeof(*slave));
> > > > -	slave_mtd = &slave->mtd;
> > > > +	part = xzalloc(sizeof(*part));
> > > >  
> > > > -	memcpy(slave_mtd, mtd, sizeof(*slave));
> > > > +	memcpy(part, mtd, sizeof(*part));
> > > 
> > > Maybe we should fixup this commit with patch:
> > > 
> > > [PATCH 6/8] mtd: partition: only copy selected fields to partition
> > 
> > I don't think so. Both change the same line, but do quite different
> > things.
> > 
> ok.
> > > > -	struct mtd_part *part = PART(mtd);
> > > > +	if (!part->master)
> > > > +		return -EINVAL;
> > > >  
> > > > -	free(mtd->name);
> > > I think this should be free(part->name);
> > 
> > What do you mean? The line you are referring to is removed in this
> > patch.
> > 
> I mean the part->name = strdup(mtd->name) in add partition function.

Now I get it. It's actually there in the end result but slipped into the
wrong patch. Fixed this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/8] mtd: Simplify partitions

[Alexander Aring]

On Thu, Feb 13, 2014 at 07:45:33PM +0100, Sascha Hauer wrote:
> On Thu, Feb 13, 2014 at 03:01:55PM +0100, Alexander Aring wrote:
> > On Thu, Feb 13, 2014 at 02:34:30PM +0100, Sascha Hauer wrote:
> > > On Thu, Feb 13, 2014 at 12:08:58PM +0100, Alexander Aring wrote:
> > > > Hi Sascha,
> > > > 
> > > > only some small nitpicks.
> > > > 
> > > > On Thu, Feb 13, 2014 at 11:25:28AM +0100, Sascha Hauer wrote:
> > > > > Embed the partition information in struct mtd_info. This makes the
> > > > > mtd partition code simpler.
> > > > > 
> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > ---
> > > > >  drivers/mtd/partition.c | 75 ++++++++++++++++++++-----------------------------
> > > > >  include/linux/mtd/mtd.h |  5 +++-
> > > > >  2 files changed, 34 insertions(+), 46 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mtd/partition.c b/drivers/mtd/partition.c
> > > > > index 85f486d..7431fc1 100644
> > > > > --- a/drivers/mtd/partition.c
> > > > > +++ b/drivers/mtd/partition.c
> > > > > @@ -4,29 +4,19 @@
> > > > >  #include <linux/err.h>
> > > > >  #include <linux/mtd/mtd.h>
> > > > >  
> > > > ...
> > > > > +	struct mtd_info *part;
> > > > >  	int start = 0, end = 0, i;
> > > > >  
> > > > > -	slave = xzalloc(sizeof(*slave));
> > > > > -	slave_mtd = &slave->mtd;
> > > > > +	part = xzalloc(sizeof(*part));
> > > > >  
> > > > > -	memcpy(slave_mtd, mtd, sizeof(*slave));
> > > > > +	memcpy(part, mtd, sizeof(*part));
> > > > 
> > > > Maybe we should fixup this commit with patch:
> > > > 
> > > > [PATCH 6/8] mtd: partition: only copy selected fields to partition
> > > 
> > > I don't think so. Both change the same line, but do quite different
> > > things.
> > > 
> > ok.
> > > > > -	struct mtd_part *part = PART(mtd);
> > > > > +	if (!part->master)
> > > > > +		return -EINVAL;
> > > > >  
> > > > > -	free(mtd->name);
> > > > I think this should be free(part->name);
> > > 
> > > What do you mean? The line you are referring to is removed in this
> > > patch.
> > > 
> > I mean the part->name = strdup(mtd->name) in add partition function.
> 
> Now I get it. It's actually there in the end result but slipped into the
> wrong patch. Fixed this.
> 
sry Sascha, I am thinking too fast sometimes. Next time I will try to makes
a proper description.

Thanks

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox