| Message ID | 673100209.65720.1411665624206.JavaMail.zimbra@xes-inc.com |
|---|---|
| State | Accepted |
| Headers | show |
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
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
----- 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
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
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
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;
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(-)