[PATCH 2/2] mtd: map_rom: Support UBI on ROM

[PATCH 2/2] mtd: map_rom: Support UBI on ROM

[Aaron Sierra]

UBI needs to know the physical erase block size, even on read-only
devices, since it defines the on-device layout. Use a device-tree
provided value to support previously written UBI on read-only NOR.

UBI also needs a non-zero writebufsize, so we set it to one.

Note: This was implemented because hardware write-protected CFI
      NOR cannot be probed for the physical erase block size.

Signed-off-by: Joe Schultz <jschultz@xes-inc.ccom>
Signed-off-by: Aaron Sierra <asierra@xes-inc.ccom>
---
 Documentation/devicetree/bindings/mtd/mtd-physmap.txt |  5 +++++
 drivers/mtd/chips/map_rom.c                           | 13 ++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
index 6b9f680..8ab16df 100644
--- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
+++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
@@ -36,6 +36,11 @@ are defined:
  - vendor-id : Contains the flash chip's vendor id (1 byte).
  - device-id : Contains the flash chip's device id (1 byte).
 
+For ROM compatible devices (and ROM fallback from cfi-flash), the following
+additional property is defined:
+
+ - erase-size : The chip's physical erase block size in bytes.
+
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.
 
diff --git a/drivers/mtd/chips/map_rom.c b/drivers/mtd/chips/map_rom.c
index 47a43cf..a831265 100644
--- a/drivers/mtd/chips/map_rom.c
+++ b/drivers/mtd/chips/map_rom.c
@@ -11,6 +11,7 @@
 #include <linux/errno.h>
 #include <linux/slab.h>
 #include <linux/init.h>
+#include <linux/of.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/map.h>
 
@@ -28,6 +29,15 @@ static struct mtd_chip_driver maprom_chipdrv = {
 	.module	= THIS_MODULE
 };
 
+static unsigned int default_erasesize(struct map_info *map)
+{
+	const __be32 *erase_size = NULL;
+#ifdef CONFIG_OF
+	erase_size = of_get_property(map->device_node, "erase-size", NULL);
+#endif
+	return !erase_size ? map->size : be32_to_cpu(*erase_size);
+}
+
 static struct mtd_info *map_rom_probe(struct map_info *map)
 {
 	struct mtd_info *mtd;
@@ -47,8 +57,9 @@ static struct mtd_info *map_rom_probe(struct map_info *map)
 	mtd->_sync = maprom_nop;
 	mtd->_erase = maprom_erase;
 	mtd->flags = MTD_CAP_ROM;
-	mtd->erasesize = map->size;
+	mtd->erasesize = default_erasesize(map);
 	mtd->writesize = 1;
+	mtd->writebufsize = 1;
 
 	__module_get(THIS_MODULE);
 	return mtd;
-- 
1.9.1

Re: [PATCH 2/2] mtd: map_rom: Support UBI on ROM

[Brian Norris]

Hi Aaron,

I see this has been sitting around way too long...

On Thu, Sep 25, 2014 at 12:20:24PM -0500, Aaron Sierra wrote:
> UBI needs to know the physical erase block size, even on read-only
> devices, since it defines the on-device layout. Use a device-tree
> provided value to support previously written UBI on read-only NOR.
> 
> UBI also needs a non-zero writebufsize, so we set it to one.

Hmm, this one is a pretty big bug IIRC (don't you get an OOPS or
BUG()?). Do you think it might be worth splitting into a separate patch
and sending to stable?

But then I'm curious: how do you get anything useful out of using UBI on
a read-only device? You don't need to (and can't) handle anything like
read disturb, and there's no write wear that needs to be leveled.

> 
> Note: This was implemented because hardware write-protected CFI
>       NOR cannot be probed for the physical erase block size.
> 
> Signed-off-by: Joe Schultz <jschultz@xes-inc.ccom>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.ccom>
> ---
>  Documentation/devicetree/bindings/mtd/mtd-physmap.txt |  5 +++++
>  drivers/mtd/chips/map_rom.c                           | 13 ++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> index 6b9f680..8ab16df 100644
> --- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> +++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> @@ -36,6 +36,11 @@ are defined:
>   - vendor-id : Contains the flash chip's vendor id (1 byte).
>   - device-id : Contains the flash chip's device id (1 byte).
>  
> +For ROM compatible devices (and ROM fallback from cfi-flash), the following
> +additional property is defined:
> +
> + - erase-size : The chip's physical erase block size in bytes.
> +
>  The device tree may optionally contain sub-nodes describing partitions of the
>  address space. See partition.txt for more detail.
>  
> diff --git a/drivers/mtd/chips/map_rom.c b/drivers/mtd/chips/map_rom.c
> index 47a43cf..a831265 100644
> --- a/drivers/mtd/chips/map_rom.c
> +++ b/drivers/mtd/chips/map_rom.c
> @@ -11,6 +11,7 @@
>  #include <linux/errno.h>
>  #include <linux/slab.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/map.h>
>  
> @@ -28,6 +29,15 @@ static struct mtd_chip_driver maprom_chipdrv = {
>  	.module	= THIS_MODULE
>  };
>  
> +static unsigned int default_erasesize(struct map_info *map)
> +{
> +	const __be32 *erase_size = NULL;
> +#ifdef CONFIG_OF
> +	erase_size = of_get_property(map->device_node, "erase-size", NULL);
> +#endif

I think you can just drop the #ifdef and have the same effect, no?

> +	return !erase_size ? map->size : be32_to_cpu(*erase_size);
> +}
> +
>  static struct mtd_info *map_rom_probe(struct map_info *map)
>  {
>  	struct mtd_info *mtd;
> @@ -47,8 +57,9 @@ static struct mtd_info *map_rom_probe(struct map_info *map)
>  	mtd->_sync = maprom_nop;
>  	mtd->_erase = maprom_erase;
>  	mtd->flags = MTD_CAP_ROM;
> -	mtd->erasesize = map->size;
> +	mtd->erasesize = default_erasesize(map);
>  	mtd->writesize = 1;
> +	mtd->writebufsize = 1;
>  
>  	__module_get(THIS_MODULE);
>  	return mtd;

Brian

Re: [PATCH 2/2] mtd: map_rom: Support UBI on ROM

[Ricard Wanderlof]

On Wed, 17 Dec 2014, Brian Norris wrote:

> But then I'm curious: how do you get anything useful out of using UBI on
> a read-only device? You don't need to (and can't) handle anything like
> read disturb, and there's no write wear that needs to be leveled.

Don't know about this particular case, but in some cases it can be an 
advantage to be able to use the same file system image in both a read-only 
and a writable environment.

Say you have some form of ROM device with no bit flips, but occasionally 
(e.g. during development) you want to use the same file system image in a 
flash. While the same thing can be accomplished by using seperate file 
systems which share the same content, it can simplify the whole system if 
only one file system image is needed which can reside in multiple types of 
media.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 2/2] mtd: map_rom: Support UBI on ROM

[Aaron Sierra]

----- Original Message -----
> From: "Ricard Wanderlof" <ricard.wanderlof@axis.com>
> Sent: Wednesday, December 17, 2014 3:37:42 AM
> 
> On Wed, 17 Dec 2014, Brian Norris wrote:
> 
> > But then I'm curious: how do you get anything useful out of using UBI on
> > a read-only device? You don't need to (and can't) handle anything like
> > read disturb, and there's no write wear that needs to be leveled.
> 
> Don't know about this particular case, but in some cases it can be an
> advantage to be able to use the same file system image in both a read-only
> and a writable environment.
>
> Say you have some form of ROM device with no bit flips, but occasionally
> (e.g. during development) you want to use the same file system image in a
> flash. While the same thing can be accomplished by using seperate file
> systems which share the same content, it can simplify the whole system if
> only one file system image is needed which can reside in multiple types of
> media.

Thanks Ricard, you have touched on the scenario that I am concerned
about. The environment in our case is a single-board computer that can
become read-only based on a hardware jumper or signal from a backplane.
It would be tedious to _have_ to prepare a separate image in order to
install the SBC into a backplane that forces the NOR into read-only
operation. Instead, we find it advantageous to be able to simply freeze
the state of the UBI filesytem until the SBC is moved back to a
read-write environment for further development.

-Aaron

Re: [PATCH 2/2] mtd: map_rom: Support UBI on ROM

[Aaron Sierra]

Brian, I have responses to your questions inline below.

----- Original Message -----
> From: "Brian Norris" <computersforpeace@gmail.com>
> Sent: Tuesday, December 16, 2014 8:33:44 PM
> 
> Hi Aaron,
> 
> I see this has been sitting around way too long...
> 
> On Thu, Sep 25, 2014 at 12:20:24PM -0500, Aaron Sierra wrote:
> > UBI needs to know the physical erase block size, even on read-only
> > devices, since it defines the on-device layout. Use a device-tree
> > provided value to support previously written UBI on read-only NOR.
> > 
> > UBI also needs a non-zero writebufsize, so we set it to one.
> 
> Hmm, this one is a pretty big bug IIRC (don't you get an OOPS or
> BUG()?). Do you think it might be worth splitting into a separate patch
> and sending to stable?

The following assumes an initially unpatched map_rom.c.

Attempting to ubiattach a ROM partition (/dev/mtd5) with the default
map_rom writebufsize set to zero, I get the following error in the kernel
log:

UBI: attaching mtd5 to ubi0
UBI error: io_init: bad write buffer size 0 for 1 min. I/O unit
ubiattach: error!: cannot attach "/dev/mtd5"
           error 22 (Invalid argument)

After assigning writebufsize to one and recompiling, attempting to
ubiattach without specifying an erase-size in the device-tree results
in the following error in the kernel log:

UBI: attaching mtd5 to ubi0
UBI: MTD device 5 is write-protected, attach in read-only mode
UBI: scanning is finished
UBI: empty MTD device detected
UBI error: ubi_early_get_peb: no free eraseblocks
UBI error: ubi_attach_mtd_dev: failed to attach mtd5, error -28
ubiattach: error!: cannot attach "/dev/mtd5"
           error 28 (No space left on device)

Neither of those errors are horrible, but both result in an inaccessible
read-only UBIFS.

> > 
> > Note: This was implemented because hardware write-protected CFI
> >       NOR cannot be probed for the physical erase block size.
> > 
> > Signed-off-by: Joe Schultz <jschultz@xes-inc.ccom>
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.ccom>
> > ---
> >  Documentation/devicetree/bindings/mtd/mtd-physmap.txt |  5 +++++
> >  drivers/mtd/chips/map_rom.c                           | 13 ++++++++++++-
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 

[ snip ]

> > +static unsigned int default_erasesize(struct map_info *map)
> > +{
> > +	const __be32 *erase_size = NULL;
> > +#ifdef CONFIG_OF
> > +	erase_size = of_get_property(map->device_node, "erase-size", NULL);
> > +#endif
> 
> I think you can just drop the #ifdef and have the same effect, no?

Yes, you are correct. I was being too cautious.

-Aaron

Re: [PATCH 2/2] mtd: map_rom: Support UBI on ROM

[Brian Norris]

On Thu, Sep 25, 2014 at 12:20:24PM -0500, Aaron Sierra wrote:
> UBI needs to know the physical erase block size, even on read-only
> devices, since it defines the on-device layout. Use a device-tree
> provided value to support previously written UBI on read-only NOR.
> 
> UBI also needs a non-zero writebufsize, so we set it to one.
> 
> Note: This was implemented because hardware write-protected CFI
>       NOR cannot be probed for the physical erase block size.
> 
> Signed-off-by: Joe Schultz <jschultz@xes-inc.ccom>
> Signed-off-by: Aaron Sierra <asierra@xes-inc.ccom>

Pushed to l2-mtd.git with two small tweaks:

> ---
>  Documentation/devicetree/bindings/mtd/mtd-physmap.txt |  5 +++++
>  drivers/mtd/chips/map_rom.c                           | 13 ++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> index 6b9f680..8ab16df 100644
> --- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> +++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> @@ -36,6 +36,11 @@ are defined:
>   - vendor-id : Contains the flash chip's vendor id (1 byte).
>   - device-id : Contains the flash chip's device id (1 byte).
>  
> +For ROM compatible devices (and ROM fallback from cfi-flash), the following
> +additional property is defined:

I noted that this property is optional, just to be clear.

> +
> + - erase-size : The chip's physical erase block size in bytes.
> +
>  The device tree may optionally contain sub-nodes describing partitions of the
>  address space. See partition.txt for more detail.
>  
> diff --git a/drivers/mtd/chips/map_rom.c b/drivers/mtd/chips/map_rom.c
> index 47a43cf..a831265 100644
> --- a/drivers/mtd/chips/map_rom.c
> +++ b/drivers/mtd/chips/map_rom.c
> @@ -11,6 +11,7 @@
>  #include <linux/errno.h>
>  #include <linux/slab.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/map.h>
>  
> @@ -28,6 +29,15 @@ static struct mtd_chip_driver maprom_chipdrv = {
>  	.module	= THIS_MODULE
>  };
>  
> +static unsigned int default_erasesize(struct map_info *map)
> +{
> +	const __be32 *erase_size = NULL;
> +#ifdef CONFIG_OF

I dropped this #ifdef.

> +	erase_size = of_get_property(map->device_node, "erase-size", NULL);
> +#endif
> +	return !erase_size ? map->size : be32_to_cpu(*erase_size);
> +}
> +
>  static struct mtd_info *map_rom_probe(struct map_info *map)
>  {
>  	struct mtd_info *mtd;
> @@ -47,8 +57,9 @@ static struct mtd_info *map_rom_probe(struct map_info *map)
>  	mtd->_sync = maprom_nop;
>  	mtd->_erase = maprom_erase;
>  	mtd->flags = MTD_CAP_ROM;
> -	mtd->erasesize = map->size;
> +	mtd->erasesize = default_erasesize(map);
>  	mtd->writesize = 1;
> +	mtd->writebufsize = 1;
>  
>  	__module_get(THIS_MODULE);
>  	return mtd;

Brian