[PATCH 0/4] Add support to modify mbr partition layout

[PATCH 0/4] Add support to modify mbr partition layout

[Michael Grzeschik]

This patchset adds the param dos_partitions to the mci device
node. By setting this param with the standard cmdlinepart notation,
it is possible to rewrite the MBR partition layout on runtime.

Michael Grzeschik (4):
  partitions: add DEVFS_PARTITION_IN_PT flag
  cmdlinepart: add option to set DEVFS_PARTITION_IN_PT flag
  partitions/dos: add function to write partition table
  mci: add MBR write and read function to block devices

 common/partitions.c     |   2 +-
 common/partitions/dos.c |  71 ++++++++++++++++++++++++++++
 drivers/mci/mci-core.c  | 122 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/cmdlinepart.h   |   1 +
 include/disks.h         |   1 +
 include/driver.h        |   1 +
 lib/cmdlinepart.c       |   3 ++
 7 files changed, 200 insertions(+), 1 deletion(-)

-- 
2.9.3


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

[PATCH 1/4] partitions: add DEVFS_PARTITION_IN_PT flag

[Michael Grzeschik]

This flag is used to represent the current status of the partition. When
it is set, the current partition layout is also available in the
partition table of the device. We use it after the partition table was
parsed.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 common/partitions.c | 2 +-
 include/driver.h    | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/partitions.c b/common/partitions.c
index 69a2b1f..62ffaef 100644
--- a/common/partitions.c
+++ b/common/partitions.c
@@ -57,7 +57,7 @@ static int register_one_partition(struct block_device *blk,
 	dev_dbg(blk->dev, "Registering partition %s on drive %s\n",
 				partition_name, blk->cdev.name);
 	cdev = devfs_add_partition(blk->cdev.name,
-				start, size, 0, partition_name);
+				start, size, DEVFS_PARTITION_IN_PT, partition_name);
 	if (IS_ERR(cdev)) {
 		ret = PTR_ERR(cdev);
 		goto out;
diff --git a/include/driver.h b/include/driver.h
index 80aa8d8..4ff2f77 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -482,6 +482,7 @@ int cdev_erase(struct cdev *cdev, loff_t count, loff_t offset);
 #define DEVFS_PARTITION_READONLY	(1U << 1)
 #define DEVFS_IS_PARTITION		(1 << 2)
 #define DEVFS_IS_CHARACTER_DEV		(1 << 3)
+#define DEVFS_PARTITION_IN_PT		(1 << 4)
 
 struct cdev *devfs_add_partition(const char *devname, loff_t offset,
 		loff_t size, unsigned int flags, const char *name);
-- 
2.9.3


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

[PATCH 2/4] cmdlinepart: add option to set DEVFS_PARTITION_IN_PT flag

[Michael Grzeschik]

With this option it is possible to flag cdev partitions, which will also
be written to the partition table of its backing device.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 include/cmdlinepart.h | 1 +
 lib/cmdlinepart.c     | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/cmdlinepart.h b/include/cmdlinepart.h
index bf8cdfa..3363a74 100644
--- a/include/cmdlinepart.h
+++ b/include/cmdlinepart.h
@@ -2,6 +2,7 @@
 #define __CMD_LINE_PART_H
 
 #define CMDLINEPART_ADD_DEVNAME (1 << 0)
+#define CMDLINEPART_ADD_TO_PT (2 << 0)
 
 int cmdlinepart_do_parse_one(const char *devname, const char *partstr,
 				 const char **endp, loff_t *offset,
diff --git a/lib/cmdlinepart.c b/lib/cmdlinepart.c
index d7d4441..8633e82 100644
--- a/lib/cmdlinepart.c
+++ b/lib/cmdlinepart.c
@@ -81,6 +81,9 @@ int cmdlinepart_do_parse_one(const char *devname, const char *partstr,
 		end = (char *)(partstr + 2);
 	}
 
+	if (partition_flags & CMDLINEPART_ADD_TO_PT)
+		flags |= DEVFS_PARTITION_IN_PT;
+
 	if (endp)
 		*endp = end;
 
-- 
2.9.3


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

[PATCH 3/4] partitions/dos: add function to write partition table

[Michael Grzeschik]

The function can be used to write an partition layout to the block device
based on its cdev layout. Only cdevs with flag DEVFS_PARTITION_IN_PT set
get written. The function also adds an static offset of 0x200000 to
ensure the mbr and bootloader will not be overwritten.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 common/partitions/dos.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/disks.h         |  1 +
 2 files changed, 72 insertions(+)

diff --git a/common/partitions/dos.c b/common/partitions/dos.c
index 5f08e25..d7fa538 100644
--- a/common/partitions/dos.c
+++ b/common/partitions/dos.c
@@ -256,6 +256,77 @@ static void dos_partition(void *buf, struct block_device *blk,
 			&dsp->signature, "%08x", dsp);
 }
 
+static inline void hdimage_setup_chs(unsigned int lba, unsigned char *chs)
+{
+	const unsigned int hpc = 255;
+	const unsigned int spt = 63;
+	unsigned int s, c;
+
+	chs[0] = (lba/spt)%hpc;
+	c = (lba/(spt * hpc));
+	s = (lba > 0) ?(lba%spt + 1) : 0;
+	chs[1] = ((c & 0x300) >> 2) | (s & 0xff);
+	chs[2] = (c & 0xff);
+}
+
+int write_dos_partition_table(struct block_device *blk, struct list_head *cdevs)
+{
+	char part_table[6+4*sizeof(struct partition_entry)+2];
+	struct cdev *cdev, *ct;
+	void *buf;
+	int ret;
+	int n = 0;
+	char *ptr;
+
+	/* prepare partition table entry */
+	ptr = part_table;
+	memset(ptr, 0, sizeof(part_table));
+
+	/* skip disk signature */
+	ptr += 6;
+	list_for_each_entry_safe(cdev, ct, cdevs, devices_list) {
+		if ((cdev->flags & DEVFS_IS_PARTITION) &&
+			(cdev->flags & DEVFS_PARTITION_IN_PT)) {
+			struct partition_entry *entry;
+			entry = (struct partition_entry *)
+				(ptr + n * sizeof(struct partition_entry));
+
+			/* add static offset to skip the mbr and bootloader */
+			cdev->offset += 4096 * SECTOR_SIZE;
+
+			entry->type = 0x83;
+			entry->partition_start = cdev->offset / SECTOR_SIZE;
+			entry->partition_size = cdev->size / SECTOR_SIZE;
+
+			hdimage_setup_chs(entry->partition_start,
+						entry->chs_begin);
+			hdimage_setup_chs(entry->partition_start +
+						entry->partition_size - 1,
+						entry->chs_end);
+			n++;
+		}
+		if (n == 4)
+			break;
+	}
+
+	ptr += 4 * sizeof(struct partition_entry);
+	ptr[0] = 0x55;
+	ptr[1] = 0xaa;
+
+	buf = read_mbr(blk);
+	if (!buf)
+		return -EIO;
+
+	/* manipulate block with prepared partition table */
+	memcpy(buf + 440, part_table, sizeof(part_table));
+
+	ret = write_mbr(blk, buf);
+
+	free(buf);
+
+	return ret;
+}
+
 static struct partition_parser dos = {
 	.parse = dos_partition,
 	.type = filetype_mbr,
diff --git a/include/disks.h b/include/disks.h
index 9932750..432fb23 100644
--- a/include/disks.h
+++ b/include/disks.h
@@ -37,5 +37,6 @@ struct partition_entry {
 } __attribute__ ((packed));
 
 extern int parse_partition_table(struct block_device*);
+extern int write_dos_partition_table(struct block_device *blk, struct list_head *cdevs);
 
 #endif /* DISKS_H */
-- 
2.9.3


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

[PATCH 4/4] mci: add MBR write and read function to block devices

[Michael Grzeschik]

With this patch it is possible to write an mbr partition table to the
mci block device. By setting the device property "dos_partitions" of the
mmc device node, it is possible to write back the new partition layout
in the common cmdlinepart notation. The property can also be read back.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/mci/mci-core.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 4e176f7..c0013a1 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -33,9 +33,11 @@
 #include <asm-generic/div64.h>
 #include <asm/byteorder.h>
 #include <block.h>
+#include <fcntl.h>
 #include <disks.h>
 #include <of.h>
 #include <linux/err.h>
+#include <cmdlinepart.h>
 
 #define MAX_BUFFER_NUMBER 0xffffffff
 
@@ -1527,6 +1529,122 @@ static void mci_info(struct device_d *dev)
 		extract_mtd_year(mci));
 }
 
+static char *print_size(uint64_t s)
+{
+	if (!(s & ((1 << 20) - 1)))
+		return basprintf("%lldM", s >> 20);
+	if (!(s & ((1 << 10) - 1)))
+		return basprintf("%lldk", s >> 10);
+	return basprintf("0x%lld", s);
+}
+
+static int print_part(char *buf, int bufsize, struct cdev *cdev, int is_last)
+{
+	char *size = print_size(cdev->size);
+	int ret;
+
+	if (!size) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = snprintf(buf, bufsize, "%s(%s)%s", size,
+			cdev->partname,
+			is_last ? "" : ",");
+out:
+	free(size);
+
+	return ret;
+}
+
+static int print_parts(char *buf, int bufsize, struct mci *mci)
+{
+	struct cdev *cdev, *ct;
+	int ret = 0;
+
+	list_for_each_entry_safe(cdev, ct, &mci->dev.cdevs, devices_list) {
+		if ((cdev->flags & DEVFS_IS_PARTITION) &&
+			(cdev->flags & DEVFS_PARTITION_IN_PT)) {
+			int now;
+			int is_last = 0;
+			struct list_head *nh = (cdev)->devices_list.next;
+			struct cdev *next = container_of(nh, typeof(*(cdev)), devices_list);
+
+			if (list_is_last(&cdev->devices_list, &mci->dev.cdevs) ||
+				!(next->flags & DEVFS_PARTITION_IN_PT))
+				is_last = 1;
+
+			now = print_part(buf, bufsize, cdev, is_last);
+			if (now < 0)
+				return now;
+
+			if (buf && bufsize) {
+				buf += now;
+				bufsize -= now;
+			}
+			ret += now;
+		}
+	}
+
+	return ret;
+}
+
+static const char *mci_partition_get(struct device_d *dev, struct param_d *p)
+{
+	struct mci *mci = container_of(dev, struct mci, dev);
+	int len = 0;
+
+	free(p->value);
+
+	len = print_parts(NULL, 0, mci);
+	p->value = xzalloc(len + 1);
+	print_parts(p->value, len + 1, mci);
+
+	return p->value;
+}
+
+#ifdef CONFIG_BLOCK_WRITE
+static int mci_partition_set(struct device_d *dev, struct param_d *p, const char *val)
+{
+	struct mci *mci = container_of(dev, struct mci, dev);
+	struct cdev *cdev, *ct;
+	int ret;
+
+	if (!val)
+		return -EINVAL;
+
+	/* remove all partition cdevs with DEVFS_IS_PARTITION set */
+	list_for_each_entry_safe(cdev, ct, &mci->dev.cdevs, devices_list) {
+		if ((cdev->flags & DEVFS_IS_PARTITION) &&
+			(cdev->flags & DEVFS_PARTITION_IN_PT))
+			ret = devfs_del_partition(cdev->name);
+			if (ret)
+				return ret;
+	}
+
+	/* read back the prepared partition layot from dos_partitions param */
+	ret = cmdlinepart_do_parse(mci->cdevname, val, mci->capacity,
+			CMDLINEPART_ADD_DEVNAME | CMDLINEPART_ADD_TO_PT);
+	if (ret)
+		return ret;
+
+	/* write the MBR partition layout based on cdevs with DEVFS_IS_PARTITION set  */
+	for (int i = 0; i < mci->nr_parts; i++) {
+		struct mci_part *part = &mci->part[i];
+		if (part->area_type == MMC_BLK_DATA_AREA_MAIN) {
+			ret = write_dos_partition_table(&part->blk,
+							&mci->dev.cdevs);
+			if (ret != 0) {
+				dev_warn(&mci->dev, "Could not write partition table\n");
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+#endif
+
 /**
  * Check if the MCI card is already probed
  * @param mci MCI device instance
@@ -1786,6 +1904,10 @@ int mci_register(struct mci_host *host)
 	mci->param_probe = dev_add_param_bool(&mci->dev, "probe",
 			mci_set_probe, NULL, &mci->probe, mci);
 
+#ifdef CONFIG_BLOCK_WRITE
+	dev_add_param(&mci->dev, "dos_partitions", mci_partition_set, mci_partition_get, 0);
+#endif
+
 	if (IS_ERR(mci->param_probe) && PTR_ERR(mci->param_probe) != -ENOSYS) {
 		ret = PTR_ERR(mci->param_probe);
 		dev_dbg(&mci->dev, "Failed to add 'probe' parameter to the MCI device\n");
-- 
2.9.3


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

Re: [PATCH 3/4] partitions/dos: add function to write partition table

[Sascha Hauer]

Hi Michael,

On Mon, Oct 17, 2016 at 03:29:22PM +0200, Michael Grzeschik wrote:
> The function can be used to write an partition layout to the block device
> based on its cdev layout. Only cdevs with flag DEVFS_PARTITION_IN_PT set
> get written. The function also adds an static offset of 0x200000 to
> ensure the mbr and bootloader will not be overwritten.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  common/partitions/dos.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/disks.h         |  1 +
>  2 files changed, 72 insertions(+)
> 
> diff --git a/common/partitions/dos.c b/common/partitions/dos.c
> index 5f08e25..d7fa538 100644
> --- a/common/partitions/dos.c
> +++ b/common/partitions/dos.c
> @@ -256,6 +256,77 @@ static void dos_partition(void *buf, struct block_device *blk,
>  			&dsp->signature, "%08x", dsp);
>  }
>  
> +static inline void hdimage_setup_chs(unsigned int lba, unsigned char *chs)
> +{
> +	const unsigned int hpc = 255;
> +	const unsigned int spt = 63;
> +	unsigned int s, c;
> +
> +	chs[0] = (lba/spt)%hpc;

Please run checkpatch over this. There are some stylistic flaws like
missing whitespaces left and right of operators.

> +	c = (lba/(spt * hpc));
> +	s = (lba > 0) ?(lba%spt + 1) : 0;
> +	chs[1] = ((c & 0x300) >> 2) | (s & 0xff);
> +	chs[2] = (c & 0xff);
> +}
> +
> +int write_dos_partition_table(struct block_device *blk, struct list_head *cdevs)
> +{
> +	char part_table[6+4*sizeof(struct partition_entry)+2];
> +	struct cdev *cdev, *ct;
> +	void *buf;
> +	int ret;
> +	int n = 0;
> +	char *ptr;
> +
> +	/* prepare partition table entry */
> +	ptr = part_table;
> +	memset(ptr, 0, sizeof(part_table));
> +
> +	/* skip disk signature */
> +	ptr += 6;

It's even more important to skip this in the output buffer. This code
should not change the disk signature.

> +	list_for_each_entry_safe(cdev, ct, cdevs, devices_list) {

Why _safe? You do not remove entries, do you?

> +		if ((cdev->flags & DEVFS_IS_PARTITION) &&
> +			(cdev->flags & DEVFS_PARTITION_IN_PT)) {

In a multiline if clause the second line should either start directly
under the opening brace or indented with at least two more tabs than the
opening if(), but using the same indention level as the conditional code
makes it hard to read.

> +			struct partition_entry *entry;

Instead of the silent test below, do a:

			if (n == 4) {
				pr_warn("Only 4 partitions written to MBR\n");
				break;
			}

> +			entry = (struct partition_entry *)
> +				(ptr + n * sizeof(struct partition_entry));
> +
> +			/* add static offset to skip the mbr and bootloader */
> +			cdev->offset += 4096 * SECTOR_SIZE;
> +
> +			entry->type = 0x83;
> +			entry->partition_start = cdev->offset / SECTOR_SIZE;
> +			entry->partition_size = cdev->size / SECTOR_SIZE;

We should have a test if offset and/or size exceed the 32bit limit.

> +
> +			hdimage_setup_chs(entry->partition_start,
> +						entry->chs_begin);
> +			hdimage_setup_chs(entry->partition_start +
> +						entry->partition_size - 1,
> +						entry->chs_end);
> +			n++;
> +		}
> +		if (n == 4)
> +			break;
> +	}
> +
> +	ptr += 4 * sizeof(struct partition_entry);
> +	ptr[0] = 0x55;
> +	ptr[1] = 0xaa;
> +
> +	buf = read_mbr(blk);
> +	if (!buf)
> +		return -EIO;

You could move this to the top of the function and directly manipulate
the input buffer.

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 4/4] mci: add MBR write and read function to block devices

[Sascha Hauer]

On Mon, Oct 17, 2016 at 03:29:23PM +0200, Michael Grzeschik wrote:
> With this patch it is possible to write an mbr partition table to the
> mci block device. By setting the device property "dos_partitions" of the
> mmc device node, it is possible to write back the new partition layout
> in the common cmdlinepart notation. The property can also be read back.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/mci/mci-core.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
> 
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index 4e176f7..c0013a1 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -33,9 +33,11 @@
>  #include <asm-generic/div64.h>
>  #include <asm/byteorder.h>
>  #include <block.h>
> +#include <fcntl.h>
>  #include <disks.h>
>  #include <of.h>
>  #include <linux/err.h>
> +#include <cmdlinepart.h>
>  
>  #define MAX_BUFFER_NUMBER 0xffffffff
>  
> @@ -1527,6 +1529,122 @@ static void mci_info(struct device_d *dev)
>  		extract_mtd_year(mci));
>  }
>  
> +static char *print_size(uint64_t s)
> +{
> +	if (!(s & ((1 << 20) - 1)))
> +		return basprintf("%lldM", s >> 20);
> +	if (!(s & ((1 << 10) - 1)))
> +		return basprintf("%lldk", s >> 10);
> +	return basprintf("0x%lld", s);

s/lld/llx/

> +}
> +
> +static int print_part(char *buf, int bufsize, struct cdev *cdev, int is_last)
> +{
> +	char *size = print_size(cdev->size);
> +	int ret;
> +
> +	if (!size) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = snprintf(buf, bufsize, "%s(%s)%s", size,
> +			cdev->partname,
> +			is_last ? "" : ",");
> +out:
> +	free(size);
> +
> +	return ret;
> +}
> +
> +static int print_parts(char *buf, int bufsize, struct mci *mci)
> +{
> +	struct cdev *cdev, *ct;
> +	int ret = 0;
> +
> +	list_for_each_entry_safe(cdev, ct, &mci->dev.cdevs, devices_list) {

safe_?

> +		if ((cdev->flags & DEVFS_IS_PARTITION) &&
> +			(cdev->flags & DEVFS_PARTITION_IN_PT)) {
> +			int now;
> +			int is_last = 0;
> +			struct list_head *nh = (cdev)->devices_list.next;
> +			struct cdev *next = container_of(nh, typeof(*(cdev)), devices_list);
> +
> +			if (list_is_last(&cdev->devices_list, &mci->dev.cdevs) ||
> +				!(next->flags & DEVFS_PARTITION_IN_PT))
> +				is_last = 1;

Is this test safe? What if the next partition does not have the
DEVFS_PARTITION_IN_PT flag set, but the one after that has? Maybe you
have to count the number of partitions in a first pass.

> +
> +			now = print_part(buf, bufsize, cdev, is_last);
> +			if (now < 0)
> +				return now;
> +
> +			if (buf && bufsize) {
> +				buf += now;
> +				bufsize -= now;
> +			}
> +			ret += now;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static const char *mci_partition_get(struct device_d *dev, struct param_d *p)
> +{
> +	struct mci *mci = container_of(dev, struct mci, dev);
> +	int len = 0;
> +
> +	free(p->value);
> +
> +	len = print_parts(NULL, 0, mci);
> +	p->value = xzalloc(len + 1);
> +	print_parts(p->value, len + 1, mci);
> +
> +	return p->value;
> +}
> +
> +#ifdef CONFIG_BLOCK_WRITE
> +static int mci_partition_set(struct device_d *dev, struct param_d *p, const char *val)
> +{
> +	struct mci *mci = container_of(dev, struct mci, dev);
> +	struct cdev *cdev, *ct;
> +	int ret;
> +
> +	if (!val)
> +		return -EINVAL;
> +
> +	/* remove all partition cdevs with DEVFS_IS_PARTITION set */
> +	list_for_each_entry_safe(cdev, ct, &mci->dev.cdevs, devices_list) {
> +		if ((cdev->flags & DEVFS_IS_PARTITION) &&
> +			(cdev->flags & DEVFS_PARTITION_IN_PT))
> +			ret = devfs_del_partition(cdev->name);
> +			if (ret)
> +				return ret;
> +	}
> +
> +	/* read back the prepared partition layot from dos_partitions param */

s/layot/layout/

> +	ret = cmdlinepart_do_parse(mci->cdevname, val, mci->capacity,
> +			CMDLINEPART_ADD_DEVNAME | CMDLINEPART_ADD_TO_PT);
> +	if (ret)
> +		return ret;
> +
> +	/* write the MBR partition layout based on cdevs with DEVFS_IS_PARTITION set  */
> +	for (int i = 0; i < mci->nr_parts; i++) {
> +		struct mci_part *part = &mci->part[i];
> +		if (part->area_type == MMC_BLK_DATA_AREA_MAIN) {
> +			ret = write_dos_partition_table(&part->blk,
> +							&mci->dev.cdevs);
> +			if (ret != 0) {
> +				dev_warn(&mci->dev, "Could not write partition table\n");
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +#endif
> +
>  /**
>   * Check if the MCI card is already probed
>   * @param mci MCI device instance
> @@ -1786,6 +1904,10 @@ int mci_register(struct mci_host *host)
>  	mci->param_probe = dev_add_param_bool(&mci->dev, "probe",
>  			mci_set_probe, NULL, &mci->probe, mci);
>  
> +#ifdef CONFIG_BLOCK_WRITE
> +	dev_add_param(&mci->dev, "dos_partitions", mci_partition_set, mci_partition_get, 0);
> +#endif

Use IS_ENABLED()

Other than that this code should be attached to parse_partition_table()
rather than making this mci specific.
We probably can safely write a dos partition table to an unpartitioned
device, but should refuse to create/manipulate a dos partition table
when a EFI partition table exists.

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 4/4] mci: add MBR write and read function to block devices

[Michael Grzeschik]

On Tue, Oct 18, 2016 at 08:23:22AM +0200, Sascha Hauer wrote:
> On Mon, Oct 17, 2016 at 03:29:23PM +0200, Michael Grzeschik wrote:
> > With this patch it is possible to write an mbr partition table to the
> > mci block device. By setting the device property "dos_partitions" of the
> > mmc device node, it is possible to write back the new partition layout
> > in the common cmdlinepart notation. The property can also be read back.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  drivers/mci/mci-core.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 122 insertions(+)
> > 
> > diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> > index 4e176f7..c0013a1 100644
> > --- a/drivers/mci/mci-core.c
> > +++ b/drivers/mci/mci-core.c
> > @@ -33,9 +33,11 @@
> >  #include <asm-generic/div64.h>
> >  #include <asm/byteorder.h>
> >  #include <block.h>
> > +#include <fcntl.h>
> >  #include <disks.h>
> >  #include <of.h>
> >  #include <linux/err.h>
> > +#include <cmdlinepart.h>
> >  
> >  #define MAX_BUFFER_NUMBER 0xffffffff
> >  
> > @@ -1527,6 +1529,122 @@ static void mci_info(struct device_d *dev)
> >  		extract_mtd_year(mci));
> >  }
> >  
> > +static char *print_size(uint64_t s)
> > +{
> > +	if (!(s & ((1 << 20) - 1)))
> > +		return basprintf("%lldM", s >> 20);
> > +	if (!(s & ((1 << 10) - 1)))
> > +		return basprintf("%lldk", s >> 10);
> > +	return basprintf("0x%lld", s);
> 
> s/lld/llx/

Why that? This will break the typical layout compared
to all other users of the kernelcmdline syntax.

> > +}
> > +
> > +static int print_part(char *buf, int bufsize, struct cdev *cdev, int is_last)
> > +{
> > +	char *size = print_size(cdev->size);
> > +	int ret;
> > +
> > +	if (!size) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	ret = snprintf(buf, bufsize, "%s(%s)%s", size,
> > +			cdev->partname,
> > +			is_last ? "" : ",");
> > +out:
> > +	free(size);
> > +
> > +	return ret;
> > +}
> > +
> > +static int print_parts(char *buf, int bufsize, struct mci *mci)
> > +{
> > +	struct cdev *cdev, *ct;
> > +	int ret = 0;
> > +
> > +	list_for_each_entry_safe(cdev, ct, &mci->dev.cdevs, devices_list) {
> 
> safe_?

Sure. We don't change the list.

> > +		if ((cdev->flags & DEVFS_IS_PARTITION) &&
> > +			(cdev->flags & DEVFS_PARTITION_IN_PT)) {
> > +			int now;
> > +			int is_last = 0;
> > +			struct list_head *nh = (cdev)->devices_list.next;
> > +			struct cdev *next = container_of(nh, typeof(*(cdev)), devices_list);
> > +
> > +			if (list_is_last(&cdev->devices_list, &mci->dev.cdevs) ||
> > +				!(next->flags & DEVFS_PARTITION_IN_PT))
> > +				is_last = 1;
> 
> Is this test safe? What if the next partition does not have the
> DEVFS_PARTITION_IN_PT flag set, but the one after that has? Maybe you
> have to count the number of partitions in a first pass.


Yes. That's a good point. Will fix that.

> > +
> > +			now = print_part(buf, bufsize, cdev, is_last);
> > +			if (now < 0)
> > +				return now;
> > +
> > +			if (buf && bufsize) {
> > +				buf += now;
> > +				bufsize -= now;
> > +			}
> > +			ret += now;
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const char *mci_partition_get(struct device_d *dev, struct param_d *p)
> > +{
> > +	struct mci *mci = container_of(dev, struct mci, dev);
> > +	int len = 0;
> > +
> > +	free(p->value);
> > +
> > +	len = print_parts(NULL, 0, mci);
> > +	p->value = xzalloc(len + 1);
> > +	print_parts(p->value, len + 1, mci);
> > +
> > +	return p->value;
> > +}
> > +
> > +#ifdef CONFIG_BLOCK_WRITE
> > +static int mci_partition_set(struct device_d *dev, struct param_d *p, const char *val)
> > +{
> > +	struct mci *mci = container_of(dev, struct mci, dev);
> > +	struct cdev *cdev, *ct;
> > +	int ret;
> > +
> > +	if (!val)
> > +		return -EINVAL;
> > +
> > +	/* remove all partition cdevs with DEVFS_IS_PARTITION set */
> > +	list_for_each_entry_safe(cdev, ct, &mci->dev.cdevs, devices_list) {
> > +		if ((cdev->flags & DEVFS_IS_PARTITION) &&
> > +			(cdev->flags & DEVFS_PARTITION_IN_PT))
> > +			ret = devfs_del_partition(cdev->name);
> > +			if (ret)
> > +				return ret;
> > +	}
> > +
> > +	/* read back the prepared partition layot from dos_partitions param */
> 
> s/layot/layout/
> 

Jupp.

> > +	ret = cmdlinepart_do_parse(mci->cdevname, val, mci->capacity,
> > +			CMDLINEPART_ADD_DEVNAME | CMDLINEPART_ADD_TO_PT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* write the MBR partition layout based on cdevs with DEVFS_IS_PARTITION set  */
> > +	for (int i = 0; i < mci->nr_parts; i++) {
> > +		struct mci_part *part = &mci->part[i];
> > +		if (part->area_type == MMC_BLK_DATA_AREA_MAIN) {
> > +			ret = write_dos_partition_table(&part->blk,
> > +							&mci->dev.cdevs);
> > +			if (ret != 0) {
> > +				dev_warn(&mci->dev, "Could not write partition table\n");
> > +				return ret;
> > +			}
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +#endif
> > +
> >  /**
> >   * Check if the MCI card is already probed
> >   * @param mci MCI device instance
> > @@ -1786,6 +1904,10 @@ int mci_register(struct mci_host *host)
> >  	mci->param_probe = dev_add_param_bool(&mci->dev, "probe",
> >  			mci_set_probe, NULL, &mci->probe, mci);
> >  
> > +#ifdef CONFIG_BLOCK_WRITE
> > +	dev_add_param(&mci->dev, "dos_partitions", mci_partition_set, mci_partition_get, 0);
> > +#endif
> 
> Use IS_ENABLED()
> 
> Other than that this code should be attached to parse_partition_table()
> rather than making this mci specific.
> We probably can safely write a dos partition table to an unpartitioned
> device, but should refuse to create/manipulate a dos partition table
> when a EFI partition table exists.

I will have that fixed in v2, until I figured out how to integrate that properly.

Thanks,
Michael


-- 
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 3/4] partitions/dos: add function to write partition table

[Michael Grzeschik]

On Tue, Oct 18, 2016 at 08:07:12AM +0200, Sascha Hauer wrote:
> Hi Michael,
> 
> On Mon, Oct 17, 2016 at 03:29:22PM +0200, Michael Grzeschik wrote:
> > The function can be used to write an partition layout to the block device
> > based on its cdev layout. Only cdevs with flag DEVFS_PARTITION_IN_PT set
> > get written. The function also adds an static offset of 0x200000 to
> > ensure the mbr and bootloader will not be overwritten.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> >  common/partitions/dos.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/disks.h         |  1 +
> >  2 files changed, 72 insertions(+)
> > 
> > diff --git a/common/partitions/dos.c b/common/partitions/dos.c
> > index 5f08e25..d7fa538 100644
> > --- a/common/partitions/dos.c
> > +++ b/common/partitions/dos.c
> > @@ -256,6 +256,77 @@ static void dos_partition(void *buf, struct block_device *blk,
> >  			&dsp->signature, "%08x", dsp);
> >  }
> >  
> > +static inline void hdimage_setup_chs(unsigned int lba, unsigned char *chs)
> > +{
> > +	const unsigned int hpc = 255;
> > +	const unsigned int spt = 63;
> > +	unsigned int s, c;
> > +
> > +	chs[0] = (lba/spt)%hpc;
> 
> Please run checkpatch over this. There are some stylistic flaws like
> missing whitespaces left and right of operators.

Thanks. I forgot to do that. It was an badly formatet template I used
here for reference. Will fix it.

> > +	c = (lba/(spt * hpc));
> > +	s = (lba > 0) ?(lba%spt + 1) : 0;
> > +	chs[1] = ((c & 0x300) >> 2) | (s & 0xff);
> > +	chs[2] = (c & 0xff);
> > +}
> > +
> > +int write_dos_partition_table(struct block_device *blk, struct list_head *cdevs)
> > +{
> > +	char part_table[6+4*sizeof(struct partition_entry)+2];
> > +	struct cdev *cdev, *ct;
> > +	void *buf;
> > +	int ret;
> > +	int n = 0;
> > +	char *ptr;
> > +
> > +	/* prepare partition table entry */
> > +	ptr = part_table;
> > +	memset(ptr, 0, sizeof(part_table));
> > +
> > +	/* skip disk signature */
> > +	ptr += 6;
> 
> It's even more important to skip this in the output buffer. This code
> should not change the disk signature.
> 
> > +	list_for_each_entry_safe(cdev, ct, cdevs, devices_list) {
> 
> Why _safe? You do not remove entries, do you?

No elements get changed in the iteration. I will change it.

> > +		if ((cdev->flags & DEVFS_IS_PARTITION) &&
> > +			(cdev->flags & DEVFS_PARTITION_IN_PT)) {
> 
> In a multiline if clause the second line should either start directly
> under the opening brace or indented with at least two more tabs than the
> opening if(), but using the same indention level as the conditional code
> makes it hard to read.

Will be changed.

> 
> > +			struct partition_entry *entry;
> 
> Instead of the silent test below, do a:
> 
> 			if (n == 4) {
> 				pr_warn("Only 4 partitions written to MBR\n");
> 				break;
> 			}
> 

Good thought. Will change.


> > +			entry = (struct partition_entry *)
> > +				(ptr + n * sizeof(struct partition_entry));
> > +
> > +			/* add static offset to skip the mbr and bootloader */
> > +			cdev->offset += 4096 * SECTOR_SIZE;
> > +
> > +			entry->type = 0x83;
> > +			entry->partition_start = cdev->offset / SECTOR_SIZE;
> > +			entry->partition_size = cdev->size / SECTOR_SIZE;
> 
> We should have a test if offset and/or size exceed the 32bit limit.
> 

Good point. Will add in v2.

> > +
> > +			hdimage_setup_chs(entry->partition_start,
> > +						entry->chs_begin);
> > +			hdimage_setup_chs(entry->partition_start +
> > +						entry->partition_size - 1,
> > +						entry->chs_end);
> > +			n++;
> > +		}
> > +		if (n == 4)
> > +			break;
> > +	}
> > +
> > +	ptr += 4 * sizeof(struct partition_entry);
> > +	ptr[0] = 0x55;
> > +	ptr[1] = 0xaa;
> > +
> > +	buf = read_mbr(blk);
> > +	if (!buf)
> > +		return -EIO;
> 
> You could move this to the top of the function and directly manipulate
> the input buffer.

Already prepared for v2.

Thanks,
Michael

-- 
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 4/4] mci: add MBR write and read function to block devices

[Michael Grzeschik]

On Wed, Oct 26, 2016 at 11:09:33AM +0200, Michael Grzeschik wrote:
> On Tue, Oct 18, 2016 at 08:23:22AM +0200, Sascha Hauer wrote:
> > On Mon, Oct 17, 2016 at 03:29:23PM +0200, Michael Grzeschik wrote:
> > > With this patch it is possible to write an mbr partition table to the
> > > mci block device. By setting the device property "dos_partitions" of the
> > > mmc device node, it is possible to write back the new partition layout
> > > in the common cmdlinepart notation. The property can also be read back.
> > > 
> > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > > ---
> > >  drivers/mci/mci-core.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 122 insertions(+)
> > > 
> > > diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> > > index 4e176f7..c0013a1 100644
> > > --- a/drivers/mci/mci-core.c
> > > +++ b/drivers/mci/mci-core.c
> > > @@ -33,9 +33,11 @@
> > >  #include <asm-generic/div64.h>
> > >  #include <asm/byteorder.h>
> > >  #include <block.h>
> > > +#include <fcntl.h>
> > >  #include <disks.h>
> > >  #include <of.h>
> > >  #include <linux/err.h>
> > > +#include <cmdlinepart.h>
> > >  
> > >  #define MAX_BUFFER_NUMBER 0xffffffff
> > >  
> > > @@ -1527,6 +1529,122 @@ static void mci_info(struct device_d *dev)
> > >  		extract_mtd_year(mci));
> > >  }
> > >  
> > > +static char *print_size(uint64_t s)
> > > +{
> > > +	if (!(s & ((1 << 20) - 1)))
> > > +		return basprintf("%lldM", s >> 20);
> > > +	if (!(s & ((1 << 10) - 1)))
> > > +		return basprintf("%lldk", s >> 10);
> > > +	return basprintf("0x%lld", s);
> > 
> > s/lld/llx/
> 
> Why that? This will break the typical layout compared
> to all other users of the kernelcmdline syntax.

As I now realize you only ment the last line, this sure makes
sense. I will fix it. But the mtd layer I copied this from
has the same issue. Will fix that aswell.

Thanks,
Michael

-- 
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