[PATCH RFC 1/2] ubi: add flags1 field to the EC header

[PATCH RFC 1/2] ubi: add flags1 field to the EC header

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

It allows storing extra information about header or a whole PEB.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/ubi/ubi-media.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h
index 22ed3f6..58065ef 100644
--- a/drivers/mtd/ubi/ubi-media.h
+++ b/drivers/mtd/ubi/ubi-media.h
@@ -125,6 +125,7 @@ enum {
  * @magic: erase counter header magic number (%UBI_EC_HDR_MAGIC)
  * @version: version of UBI implementation which is supposed to accept this
  *           UBI image
+ * @flags1: flags describing header/block
  * @padding1: reserved for future, zeroes
  * @ec: the erase counter
  * @vid_hdr_offset: where the VID header starts
@@ -157,7 +158,8 @@ enum {
 struct ubi_ec_hdr {
 	__be32  magic;
 	__u8    version;
-	__u8    padding1[3];
+	__u8	flags1;
+	__u8    padding1[2];
 	__be64  ec; /* Warning: the current limit is 31-bit anyway! */
 	__be32  vid_hdr_offset;
 	__be32  data_offset;
-- 
2.10.1

[PATCH RFC 2/2] ubi: add support for UBI_EC_FLAG_ERASE_FROM_HERE

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This flag can be used to mark block that should start erasing process
(including that block itself).
This can be useful for devices that don't support UBI natively (in a
bootloader or original firmware). In such cases we need to flash
ubinized image which is usually created with autoresize flag. In such
cases only few blocks of the whole ubi partition are programmed and the
rest of them should be formatted when attaching.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
Hi,

This patch is my attempt of getting upstream solution of problem we were
solving locally in OpenWrt/LEDE projects.

There are a lot of home routers with original firmware and bootloaders
not supporting UBI in any way. This is where we use ubinize tool to
prepare images with UBI. Unfortunately flashing such an image to the
firmware partition writes only few blocks (as many as were created by
ubinize). We need some way of formatting remainig blocks of the firmware
(ubi) partition.

This is RFC and I'd really like to hear your comments.

First of all using a global erase_all_next_pebs is a pretty bad idea. I
mean to store this information in struct ubi_attach_info but there goes
my question:
1) Should I pass "struct ubi_attach_info" to the ubi_io_read_ec_hdr to
   let this function set some interfnal flah that all next blocks should
   be erased?
OR
2) Should I introduce UBI_IO_ERASE (or similar) and add support for this
   new "error" in the scan_peb?

Second question: I wasn't sure what's the better solution:
1) Create empty block with UBI_EC_FLAG_ERASE_FROM_HERE
OR
2) Set UBI_EC_FLAG_ERASE_BEYOND in the last block

Third question: what impact this change can have? I'm pretty sure I'll
need to adjust ubiformat tool. We don't want to format ubi with writing
image at the same time, just to trigger erasing blocks on the next
attach attempt. I guess ubiformat shouldn't copy/write this flag when
writing an image (--flash-image=<file> option). What else should I
consider?
---
 drivers/mtd/ubi/attach.c    |  4 ++++
 drivers/mtd/ubi/io.c        | 16 ++++++++++++++++
 drivers/mtd/ubi/ubi-media.h |  3 +++
 drivers/mtd/ubi/ubi.h       |  3 +++
 4 files changed, 26 insertions(+)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 93ceea4..b01e932 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -95,6 +95,8 @@ static int self_check_ai(struct ubi_device *ubi, struct ubi_attach_info *ai);
 #define AV_ADD		BIT(1)
 #define AV_FIND_OR_ADD	(AV_FIND | AV_ADD)
 
+bool erase_all_next_pebs;
+
 /**
  * find_or_add_av - internal function to find a volume, add a volume or do
  *		    both (find and add if missing).
@@ -1573,6 +1575,8 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 	if (!ai)
 		return -ENOMEM;
 
+	erase_all_next_pebs = false;
+
 #ifdef CONFIG_MTD_UBI_FASTMAP
 	/* On small flash devices we disable fastmap in any case. */
 	if ((int)mtd_div_by_eb(ubi->mtd->size, ubi->mtd) <= UBI_FM_MAX_START) {
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index b6fb8f9..c2a8ce6 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -758,6 +758,13 @@ int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum,
 		 */
 	}
 
+	if (erase_all_next_pebs) {
+		if (!read_err)
+			return UBI_IO_FF;
+		else
+			return UBI_IO_FF_BITFLIPS;
+	}
+
 	magic = be32_to_cpu(ec_hdr->magic);
 	if (magic != UBI_EC_HDR_MAGIC) {
 		if (mtd_is_eccerr(read_err))
@@ -813,6 +820,15 @@ int ubi_io_read_ec_hdr(struct ubi_device *ubi, int pnum,
 			return UBI_IO_BAD_HDR_EBADMSG;
 	}
 
+	if (ec_hdr->flags1 & UBI_EC_FLAG_ERASE_FROM_HERE) {
+		ubi_msg(ubi, "erasing all PEBs starting from %d", pnum);
+		erase_all_next_pebs = true;
+		if (!read_err)
+			return UBI_IO_FF;
+		else
+			return UBI_IO_FF_BITFLIPS;
+	}
+
 	/* And of course validate what has just been read from the media */
 	err = validate_ec_hdr(ubi, ec_hdr);
 	if (err) {
diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h
index 58065ef..0e8cace 100644
--- a/drivers/mtd/ubi/ubi-media.h
+++ b/drivers/mtd/ubi/ubi-media.h
@@ -35,6 +35,9 @@
 /* The version of UBI images supported by this implementation */
 #define UBI_VERSION 1
 
+/* The version of UBI images supported by this implementation */
+#define UBI_EC_FLAG_ERASE_FROM_HERE	(1 << 0)
+
 /* The highest erase counter value supported by this implementation */
 #define UBI_MAX_ERASECOUNTER 0x7FFFFFFF
 
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 697dbcb..45a2cf3 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -779,6 +779,7 @@ struct ubi_attach_info {
 	int mean_ec;
 	uint64_t ec_sum;
 	int ec_count;
+	int erase_all_next_pebs;
 	struct kmem_cache *aeb_slab_cache;
 	struct ubi_ec_hdr *ech;
 	struct ubi_vid_io_buf *vidb;
@@ -821,6 +822,8 @@ extern struct class ubi_class;
 extern struct mutex ubi_devices_mutex;
 extern struct blocking_notifier_head ubi_notifiers;
 
+extern bool erase_all_next_pebs;
+
 /* attach.c */
 struct ubi_ainf_peb *ubi_alloc_aeb(struct ubi_attach_info *ai, int pnum,
 				   int ec);
-- 
2.10.1

Re: [PATCH RFC 2/2] ubi: add support for UBI_EC_FLAG_ERASE_FROM_HERE

[Richard Weinberger]

Rafał,

On 30.12.2016 18:11, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This flag can be used to mark block that should start erasing process
> (including that block itself).
> This can be useful for devices that don't support UBI natively (in a
> bootloader or original firmware). In such cases we need to flash
> ubinized image which is usually created with autoresize flag. In such
> cases only few blocks of the whole ubi partition are programmed and the
> rest of them should be formatted when attaching.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> Hi,
> 
> This patch is my attempt of getting upstream solution of problem we were
> solving locally in OpenWrt/LEDE projects.
> 
> There are a lot of home routers with original firmware and bootloaders
> not supporting UBI in any way. This is where we use ubinize tool to
> prepare images with UBI. Unfortunately flashing such an image to the
> firmware partition writes only few blocks (as many as were created by
> ubinize). We need some way of formatting remainig blocks of the firmware
> (ubi) partition.
> 
> This is RFC and I'd really like to hear your comments.
> 
> First of all using a global erase_all_next_pebs is a pretty bad idea. I
> mean to store this information in struct ubi_attach_info but there goes
> my question:
> 1) Should I pass "struct ubi_attach_info" to the ubi_io_read_ec_hdr to
>    let this function set some interfnal flah that all next blocks should
>    be erased?
> OR
> 2) Should I introduce UBI_IO_ERASE (or similar) and add support for this
>    new "error" in the scan_peb?
> 
> Second question: I wasn't sure what's the better solution:
> 1) Create empty block with UBI_EC_FLAG_ERASE_FROM_HERE
> OR
> 2) Set UBI_EC_FLAG_ERASE_BEYOND in the last block
> 
> Third question: what impact this change can have? I'm pretty sure I'll
> need to adjust ubiformat tool. We don't want to format ubi with writing
> image at the same time, just to trigger erasing blocks on the next
> attach attempt. I guess ubiformat shouldn't copy/write this flag when
> writing an image (--flash-image=<file> option). What else should I
> consider?

As discussed on IRC, I'm not fond of this patches.

To summarize, your target devices have "bad" bootloaders pre-installed.
You cannot change nor fix them. If you ask such a bootloader to flash
the UBI image to the MTD partition it will erase and program only as
much blocks as the image allokates and the remaining blocks on the MTD
partition stay untouched.
Later an ubiattach will fail since UBI wants to see either valid headers
or all 0xff bytes but it finds old data since the bootloader did not erase
all blocks.
This patches implement an "EOF marker" to teach UBI about the last valid
block and let UBI do the erase.

Do these bootloaders really don't offer a command to erase the whole MTD
partition? If they offer a flash command I'd expect an erase command too.

I recommend using an initramfs. In the initramfs you can do all the clean-up
your bootloader misses. Using flash_erase you can do the erase yourself,
load the ubi module, run ubiattach, mount ubifs and hand-over to the regular
userspace.

Brian, Boris, Artem, what do you think?
I'd like to keep UBI free from such workarounds if it can be addressed
in another way.

Thanks,
//richard

Re: [PATCH RFC 2/2] ubi: add support for UBI_EC_FLAG_ERASE_FROM_HERE

[Rafał Miłecki]

On 30 December 2016 at 21:47, Richard Weinberger <richard@nod.at> wrote:
> On 30.12.2016 18:11, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This flag can be used to mark block that should start erasing process
>> (including that block itself).
>> This can be useful for devices that don't support UBI natively (in a
>> bootloader or original firmware). In such cases we need to flash
>> ubinized image which is usually created with autoresize flag. In such
>> cases only few blocks of the whole ubi partition are programmed and the
>> rest of them should be formatted when attaching.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> Hi,
>>
>> This patch is my attempt of getting upstream solution of problem we were
>> solving locally in OpenWrt/LEDE projects.
>>
>> There are a lot of home routers with original firmware and bootloaders
>> not supporting UBI in any way. This is where we use ubinize tool to
>> prepare images with UBI. Unfortunately flashing such an image to the
>> firmware partition writes only few blocks (as many as were created by
>> ubinize). We need some way of formatting remainig blocks of the firmware
>> (ubi) partition.
>>
>> This is RFC and I'd really like to hear your comments.
>>
>> First of all using a global erase_all_next_pebs is a pretty bad idea. I
>> mean to store this information in struct ubi_attach_info but there goes
>> my question:
>> 1) Should I pass "struct ubi_attach_info" to the ubi_io_read_ec_hdr to
>>    let this function set some interfnal flah that all next blocks should
>>    be erased?
>> OR
>> 2) Should I introduce UBI_IO_ERASE (or similar) and add support for this
>>    new "error" in the scan_peb?
>>
>> Second question: I wasn't sure what's the better solution:
>> 1) Create empty block with UBI_EC_FLAG_ERASE_FROM_HERE
>> OR
>> 2) Set UBI_EC_FLAG_ERASE_BEYOND in the last block
>>
>> Third question: what impact this change can have? I'm pretty sure I'll
>> need to adjust ubiformat tool. We don't want to format ubi with writing
>> image at the same time, just to trigger erasing blocks on the next
>> attach attempt. I guess ubiformat shouldn't copy/write this flag when
>> writing an image (--flash-image=<file> option). What else should I
>> consider?
>
> As discussed on IRC, I'm not fond of this patches.

Thanks for the IRC discussion, it's sometimes hard to describe all
details of such problems. Glad you managed to get all extra needed
info from me ;)


> To summarize, your target devices have "bad" bootloaders pre-installed.
> You cannot change nor fix them. If you ask such a bootloader to flash
> the UBI image to the MTD partition it will erase and program only as
> much blocks as the image allokates and the remaining blocks on the MTD
> partition stay untouched.
> Later an ubiattach will fail since UBI wants to see either valid headers
> or all 0xff bytes but it finds old data since the bootloader did not erase
> all blocks.
> This patches implement an "EOF marker" to teach UBI about the last valid
> block and let UBI do the erase.

A very nice summary, thanks!


> Do these bootloaders really don't offer a command to erase the whole MTD
> partition? If they offer a flash command I'd expect an erase command too.

Some of them do, but that requires serial access to the device. Most
users use button-triggered tftp server/client or just minimal
bootloader web interface for flashing an image. In these cases you
don't have any control over what bootloader does.


> I recommend using an initramfs. In the initramfs you can do all the clean-up
> your bootloader misses. Using flash_erase you can do the erase yourself,
> load the ubi module, run ubiattach, mount ubifs and hand-over to the regular
> userspace.

I'll see if I can do that, thanks for this alternative solution idea.


> Brian, Boris, Artem, what do you think?
> I'd like to keep UBI free from such workarounds if it can be addressed
> in another way.

-- 
Rafał

Re: [PATCH RFC 2/2] ubi: add support for UBI_EC_FLAG_ERASE_FROM_HERE

[Richard Weinberger]

Rafał,

On 30.12.2016 22:44, Rafał Miłecki wrote:
> Some of them do, but that requires serial access to the device. Most
> users use button-triggered tftp server/client or just minimal
> bootloader web interface for flashing an image. In these cases you
> don't have any control over what bootloader does.

So the use case is the initial installation of OpenWRT.
What about using an installer?
i.e. create Kernel with an in-kernel initramfs. This "installer kernel"
will get flashed using that button-triggered mechanism.
All the pre-installed bootloader has to do is flashing and booting the kernel.
This kernel will then boot and provide a fancy webinterface to the user
where she can install the OpenWRT rootfs. That way you can use all the Linux
tools for partitioning, flashing, verification(!), etc.
The installer kernel will be used only once can be get removed after the
installation on OpenWRT.

Thanks,
//richard

Re: [PATCH RFC 2/2] ubi: add support for UBI_EC_FLAG_ERASE_FROM_HERE

[Boris Brezillon]

On Fri, 30 Dec 2016 21:47:06 +0100
Richard Weinberger <richard@nod.at> wrote:

> Rafał,
> 
> On 30.12.2016 18:11, Rafał Miłecki wrote:
> > From: Rafał Miłecki <rafal@milecki.pl>
> > 
> > This flag can be used to mark block that should start erasing process
> > (including that block itself).
> > This can be useful for devices that don't support UBI natively (in a
> > bootloader or original firmware). In such cases we need to flash
> > ubinized image which is usually created with autoresize flag. In such
> > cases only few blocks of the whole ubi partition are programmed and the
> > rest of them should be formatted when attaching.
> > 
> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> > ---
> > Hi,
> > 
> > This patch is my attempt of getting upstream solution of problem we were
> > solving locally in OpenWrt/LEDE projects.
> > 
> > There are a lot of home routers with original firmware and bootloaders
> > not supporting UBI in any way. This is where we use ubinize tool to
> > prepare images with UBI. Unfortunately flashing such an image to the
> > firmware partition writes only few blocks (as many as were created by
> > ubinize). We need some way of formatting remainig blocks of the firmware
> > (ubi) partition.
> > 
> > This is RFC and I'd really like to hear your comments.
> > 
> > First of all using a global erase_all_next_pebs is a pretty bad idea. I
> > mean to store this information in struct ubi_attach_info but there goes
> > my question:
> > 1) Should I pass "struct ubi_attach_info" to the ubi_io_read_ec_hdr to
> >    let this function set some interfnal flah that all next blocks should
> >    be erased?
> > OR
> > 2) Should I introduce UBI_IO_ERASE (or similar) and add support for this
> >    new "error" in the scan_peb?
> > 
> > Second question: I wasn't sure what's the better solution:
> > 1) Create empty block with UBI_EC_FLAG_ERASE_FROM_HERE
> > OR
> > 2) Set UBI_EC_FLAG_ERASE_BEYOND in the last block
> > 
> > Third question: what impact this change can have? I'm pretty sure I'll
> > need to adjust ubiformat tool. We don't want to format ubi with writing
> > image at the same time, just to trigger erasing blocks on the next
> > attach attempt. I guess ubiformat shouldn't copy/write this flag when
> > writing an image (--flash-image=<file> option). What else should I
> > consider?  
> 
> As discussed on IRC, I'm not fond of this patches.
> 
> To summarize, your target devices have "bad" bootloaders pre-installed.
> You cannot change nor fix them. If you ask such a bootloader to flash
> the UBI image to the MTD partition it will erase and program only as
> much blocks as the image allokates and the remaining blocks on the MTD
> partition stay untouched.
> Later an ubiattach will fail since UBI wants to see either valid headers
> or all 0xff bytes but it finds old data since the bootloader did not erase
> all blocks.
> This patches implement an "EOF marker" to teach UBI about the last valid
> block and let UBI do the erase.
> 
> Do these bootloaders really don't offer a command to erase the whole MTD
> partition? If they offer a flash command I'd expect an erase command too.
> 
> I recommend using an initramfs. In the initramfs you can do all the clean-up
> your bootloader misses. Using flash_erase you can do the erase yourself,
> load the ubi module, run ubiattach, mount ubifs and hand-over to the regular
> userspace.
> 
> Brian, Boris, Artem, what do you think?
> I'd like to keep UBI free from such workarounds if it can be addressed
> in another way.

I haven't been through the IRC discussion yet, but I agree with what
you say here: we should avoid adding new hacks in UBI if it's not
proven to be necessary. This is not the first time we see OpenWRT/LEDE
developers trying to address things directly in the kernel while it
could be properly addressed from userspace using existing tools or
developing custom ones.
I hear your argument that some platforms have old/buggy bootloaders
which don't support (or badly support) some features, but putting ugly
workarounds in the kernel code does not sound like the right solution
to me.

Richard's suggestion sounds good. You can also add a second
stage bootloader (kexec based?) to make sure that you have the
necessary tools to format the ubi image, but that's probably more
invasive and requires extra space.

Regards,

Boris