Possible regression regarding the name and size of MTD devices on kernel 5.5.4

* Possible regression regarding the name and size of MTD devices on kernel 5.5.4
@ 2020-03-11 19:58 Victor Fusco
  2020-03-11 20:31 ` Boris Brezillon
  0 siblings, 1 reply; 3+ messages in thread
From: Victor Fusco @ 2020-03-11 19:58 UTC (permalink / raw)
  To: linux-mtd

Hi,

I'm investigating a regression regarding the name and size of MTD devices that
happened here while migrating to kernel 5.5.4. I'm not sure if it is an error
or misuse of our part, but it was working properly on kernel 4.20.8. It seems
that, on our use case, the new implementation is not honoring the
"linux,mtd-name" and "reg" fields defined on the Device Tree.

I patched the kernel to make it work but, as it's not clear if it's an error
or misuse, I decided to send this message to check if somebody could help.

The devices are defined with a combination of the Device Tree entries and
Kernel Command Line parameters. The following are some of the information that
was collected during this investigation.

The relevant entries of the Device Tree:
----------
~ # dtc -I dtb /sys/firmware/fdt
[...]
flash@8000000000000000 {
    #address-cells = <0x02>;
    #size-cells = <0x02>;
    compatible = "mtd-ram";
    bank-width = <0x04>;
    reg = <0x80000000 0x00 0x00 0x3c00000>;
    linux,mtd-name = "flash.0";
};

flash@8000000004000000 {
    #address-cells = <0x02>;
    #size-cells = <0x02>;
    compatible = "mtd-ram";
    bank-width = <0x04>;
    reg = <0x80000000 0x4000000 0x00 0x3c00000>;
    linux,mtd-name = "flash.1";
};
[...]
----------

The log entry for the kernel command line:
----------
[    0.000000] Kernel command line: console=hvc0 rootfstype=ext2
root=/dev/mtdblock0 rw mtdparts=flash.0:-(root);flash.1:-(myfs)
----------

The current log of the MTD drivers:
----------
[    0.035397] physmap-flash 8000000000000000.flash: physmap platform
flash device: [mem 0x8000000000000000-0x8000000003bfffff]
[    0.035934] physmap-flash 8000000004000000.flash: physmap platform
flash device: [mem 0x8000000004000000-0x8000000007bfffff]
----------

The information collected from the /sys/block:
----------
~ # cat /sys/block/mtdblock0/device/name
8000000000000000.flash
~ # cat /sys/block/mtdblock0/device/size
33554432
~ # cat /sys/block/mtdblock1/device/name
8000000004000000.flash
~ # cat /sys/block/mtdblock1/device/size
33554432
----------

The expected names and sizes were:

root, 0x3c00000
myfs, 0x3c00000

But we got:

8000000000000000.flash, 0x2000000
8000000004000000.flash, 0x2000000

And while using it we can observe some errors in the kernel log that seems to
be related to the wrong size being detected:
----------
~ # ls -lR
[...]
./usr/lib:
[ 1528.426466] attempt to access beyond end of device
[ 1528.426481] mtdblock1: rw=0, want=70838, limit=65536
[ 1528.426495] attempt to access beyond end of device
[ 1528.426510] mtdblock1: rw=0, want=88988, limit=65536
[ 1528.426551] EXT2-fs (mtdblock1): error: ext2_readdir: bad page in #590
total 0
[...]
----------

Searching into the git history I've found two changes that seems to have
introduced this new behavior:

1. The merge of physmap_of.c into physmap-core.c made it stop checking the
mtd_name

https://github.com/torvalds/linux/commit/642b1e8dbed7bbbf8c4deb3c9a0496f17278badc#diff-25f9c3817991d18e6c24935d91953344L223

The original implementation was:

drivers/mtd/maps/physmap_of_core.c:223
----------
        info->list[i].map.name = mtd_name ?: dev_name(&dev->dev);
----------

And the new one:

drivers/mtd/maps/physmap-core.c:237
----------
        info->maps[i].name = dev_name(&dev->dev);
----------

2. The merge gpio-addr-flash.c into physmap-core.c made it calculate the size
differently. Not sure if the new implementation is accurate when there is no
gpio.

https://github.com/torvalds/linux/commit/ba32ce95cbd9876eb7f5ec39af87829c8f13a337#diff-82fc46753342e94dc0772828e76af427L372

The original implementation was:

drivers/mtd/maps/physmap-core.c:372
----------
        info->maps[i].size = resource_size(res);
----------

And the new one:

drivers/mtd/maps/physmap-core.c:507
----------
        info->win_order = get_bitmask_order(resource_size(res)) - 1;
        info->maps[i].size = BIT(info->win_order +
                     (info->gpios ?
                      info->gpios->ndescs : 0));
----------

I did a small change on these points to check if the mtd_name was defined like
before and use resouce_size(res) when there is no gpios defined and it seems to
have fixed our problem.

The relevant kernel log entries of the MTD drivers after the patch:
----------
[    0.035116] physmap-flash 8000000000000000.flash: physmap platform
flash device: [mem 0x8000000000000000-0x8000000003bfffff]
[    0.035146] 1 cmdlinepart partitions found on MTD device flash.0
[    0.035163] Creating 1 MTD partitions on "flash.0":
[    0.035178] 0x000000000000-0x000003c00000 : "root"
[    0.035694] physmap-flash 8000000004000000.flash: physmap platform
flash device: [mem 0x8000000004000000-0x8000000007bfffff]
[    0.035723] 1 cmdlinepart partitions found on MTD device flash.1
[    0.035740] Creating 1 MTD partitions on "flash.1":
[    0.035755] 0x000000000000-0x000003c00000 : "myfs"
----------

The information collected from the /sys/block after the patch:
----------
~ # cat /sys/block/mtdblock0/device/name
root
~ # cat /sys/block/mtdblock0/device/size
62914560
~ # cat /sys/block/mtdblock1/device/name
myfs
~ # cat /sys/block/mtdblock1/device/size
62914560
----------

Should our settings on the Device Tree and kernel command line work properly?

Do the changes I made make sense?

Best regards,

Victor Fusco

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 3+ messages in thread