Re: [PATCH linux-next v4 11/11] mtd: bcm63xxpart: Add NAND partitioning support

From: Brian Norris
Date: Wed Jan 27 2016 - 18:08:07 EST

Next message: KY Srinivasan: "RE: [PATCH] storvsc: use small sg_tablesize on x86"
Previous message: Greg KH: "Re: Move wlan-ng out of staging?"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On Sun, Dec 13, 2015 at 10:53:13PM +0000, Simon Arlott wrote:
> Add partitioning support for BCM963xx boards with NAND flash.
>
> The following partitions are defined:
> "boot": CFE and nvram data
> "rootfs": Currently selected rootfs
> "data": Configuration data
> "rootfs1_update": Container for the whole flash area used
> for the first rootfs to allow it to be
> updated
> "rootfs2_update": Container for the whole flash area used
> for the second rootfs to allow it to be
> updated
> "rootfs_other": The other (not currently selected) rootfs
>
> Example:
> [ 2.157094] nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
> [ 2.163796] nand: Macronix NAND 128MiB 3,3V 8-bit
> [ 2.168648] nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
> [ 2.176588] bcm6368_nand 10000200.nand: detected 128MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, Hamming ECC
> [ 2.189782] Bad block table found at page 65472, version 0x01
> [ 2.196910] Bad block table found at page 65408, version 0x01
> [ 2.204003] nand_read_bbt: bad block at 0x000007480000
> [ 2.225220] bcm63xxpart: rootfs1: CFE image tag found at 0x20000 with version 6, board type 963168VX
> [ 2.236188] bcm63xxpart: rootfs2: CFE image tag found at 0x4000000 with version 6, board type 963168VX
> [ 2.246165] bcm63xxpart: CFE bootline selected latest image rootfs1 (rootfs1_seq=2, rootfs2_seq=1)
> [ 2.255800] 6 bcm63xxpart partitions found on MTD device brcmnand.0
> [ 2.275360] Creating 6 MTD partitions on "brcmnand.0":
> [ 2.280804] 0x000000000000-0x000000020000 : "boot"
> [ 2.294609] 0x000000040000-0x000001120000 : "rootfs"
> [ 2.310078] 0x000007b00000-0x000007f00000 : "data"
> [ 2.324052] 0x000000020000-0x000003ac0000 : "rootfs1_update"
> [ 2.339190] 0x000004000000-0x000007ac0000 : "rootfs2_update"
> [ 2.354290] 0x000004020000-0x000005060000 : "rootfs_other"
>
> The nvram contains the offset and size of the boot, rootfs1, rootfs2
> and data partitions. The presence of CFE and nvram is already verified
> by reading from the boot partition which is assumed to be at offset 0
> and the NAND process aborts if the nvram read indicates that this is
> not the case.
>
> There is bcm_tag information at the start of each rootfs that is used
> to determine which rootfs is newer and what its real offset/size is.
>
> The CFE bootline is used to select a rootfs.
>
> Signed-off-by: Simon Arlott <simon@xxxxxxxxxxx>

Have we gotten any testing besides Simon? Or any Acked-by/Reviewed-by?

> ---
> v4: Reorganised functions based on earlier new patches in the series,
> no real logic changes other than having to check for nvram->version
> >= 6 within the nand function instead of the nvram read function.
>
> Renamed "curpart" to "i" because it allows the partition layout
> lines to be under 80 characters.
>
> v3: Use COMPILE_TEST.
>
> Ensure that strings read from flash are null terminated and validate
> bcm_tag integer values (this also moves reporting of rootfs sequence
> numbers to later on).
>
> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
>
> Removed support for the nand partition number field, it's not a
> standard Broadcom field (was added by MitraStar Technology Corp.).
>
> drivers/mtd/bcm63xxpart.c | 196 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 190 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
> index 26c38a1..4576b78 100644
> --- a/drivers/mtd/bcm63xxpart.c
> +++ b/drivers/mtd/bcm63xxpart.c
> @@ -16,10 +16,9 @@
> * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> * GNU General Public License for more details.
> *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> - *
> + * NAND flash layout derived from bcm963xx_4.12L.06B_consumer/bcmdrivers/opensource/char/board/bcm963xx/impl1/board.c,
> + * bcm963xx_4.12L.06B_consumer/shared/opensource/include/bcm963xx/bcm_hwdefs.h:
> + * Copyright (c) 2002 Broadcom Corporation
> */
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -124,6 +123,25 @@ static int bcm63xx_read_image_tag(struct mtd_info *master, const char *name,
> return 1;
> }
>
> +static bool bcm63xx_boot_latest(struct bcm963xx_nvram *nvram)
> +{
> + char *p;
> +
> + STR_NULL_TERMINATE(nvram->bootline);
> +
> + /* Find previous image parameter "p" */
> + if (!strncmp(nvram->bootline, "p=", 2))
> + p = nvram->bootline;
> + else
> + p = strstr(nvram->bootline, " p=");
> +
> + if (p == NULL)
> + return true;
> +
> + p += 2;
> + return *p != '0';
> +}
> +
> static int bcm63xx_parse_cfe_nor_partitions(struct mtd_info *master,
> const struct mtd_partition **pparts, struct bcm963xx_nvram *nvram)
> {
> @@ -283,6 +301,171 @@ out:
> return nrparts;
> }
>
> +static bool bcm63xx_parse_nand_image_tag(struct mtd_info *master,
> + const char *name, loff_t tag_offset, u64 *rootfs_offset,
> + u64 *rootfs_size, unsigned int *rootfs_sequence)
> +{
> + struct bcm_tag *buf;
> + int ret;
> + bool rootfs_ok = false;
> +
> + *rootfs_offset = 0;
> + *rootfs_size = 0;
> + *rootfs_sequence = 0;
> +
> + buf = vmalloc(sizeof(struct bcm_tag));
> + if (!buf)
> + goto out;
> +
> + ret = bcm63xx_read_image_tag(master, name, tag_offset, buf);
> + if (!ret) {
> + /* Get rootfs offset and size from tag data */
> + STR_NULL_TERMINATE(buf->flash_image_start);
> + if (kstrtou64(buf->flash_image_start, 10, rootfs_offset) ||
> + *rootfs_offset < BCM963XX_EXTENDED_SIZE) {
> + pr_err("%s: invalid rootfs offset: %*ph\n", name,
> + sizeof(buf->flash_image_start),
> + buf->flash_image_start);
> + goto out;
> + }
> +
> + STR_NULL_TERMINATE(buf->root_length);
> + if (kstrtou64(buf->root_length, 10, rootfs_size) ||
> + rootfs_size == 0) {

Sparse and Smatch have caught errors on this line:

drivers/mtd/bcm63xxpart.c:334:48: warning: Using plain integer as NULL pointer [sparse]
drivers/mtd/bcm63xxpart.c:334 bcm63xx_parse_nand_image_tag() warn: variable dereferenced before check 'rootfs_size' (see line 313) [smatch]

I think you mean '*rootfs_size == 0', not 'rootfs_size == 0'.

Brian

> + pr_err("%s: invalid rootfs size: %*ph\n", name,
> + sizeof(buf->root_length), buf->root_length);
> + goto out;
> + }
> +
> + /* Adjust for flash offset */
> + *rootfs_offset -= BCM963XX_EXTENDED_SIZE;
> +
> + /* Remove bcm_tag data from length */
> + *rootfs_size -= *rootfs_offset - tag_offset;
> +
> + /* Get image sequence number to determine which one is newer */
> + STR_NULL_TERMINATE(buf->image_sequence);
> + if (kstrtouint(buf->image_sequence, 10, rootfs_sequence)) {
> + pr_err("%s: invalid rootfs sequence: %*ph\n", name,
> + sizeof(buf->image_sequence),
> + buf->image_sequence);
> + goto out;
> + }
> +
> + rootfs_ok = true;
> + }
> +
> +out:
> + vfree(buf);
> + return rootfs_ok;
> +}
> +
> +static int bcm63xx_parse_cfe_nand_partitions(struct mtd_info *master,
> + const struct mtd_partition **pparts,
> + struct bcm963xx_nvram *nvram)
> +{
> + int nrparts, i;
> + struct mtd_partition *parts;
> + u64 rootfs1_off, rootfs1_size;
> + unsigned int rootfs1_seq;
> + u64 rootfs2_off, rootfs2_size;
> + unsigned int rootfs2_seq;
> + bool rootfs1, rootfs2;
> + bool use_first;
> +
> + if (nvram->version < 6) {
> + pr_warn("nvram version %u not supported\n", nvram->version);
> + return -EINVAL;
> + }
> +
> + /* We've just read the nvram from offset 0,
> + * so it must be located there.
> + */
> + if (BCM963XX_NVRAM_NAND_PART_OFFSET(nvram, BOOT) != 0)
> + return -EINVAL;
> +
> + /* Get the rootfs partition locations */
> + rootfs1 = bcm63xx_parse_nand_image_tag(master, "rootfs1",
> + BCM963XX_NVRAM_NAND_PART_OFFSET(nvram, ROOTFS_1),
> + &rootfs1_off, &rootfs1_size, &rootfs1_seq);
> + rootfs2 = bcm63xx_parse_nand_image_tag(master, "rootfs2",
> + BCM963XX_NVRAM_NAND_PART_OFFSET(nvram, ROOTFS_2),
> + &rootfs2_off, &rootfs2_size, &rootfs2_seq);
> +
> + /* Determine primary rootfs partition */
> + if (rootfs1 && rootfs2) {
> + bool use_latest = bcm63xx_boot_latest(nvram);
> +
> + /* Compare sequence numbers */
> + if (use_latest)
> + use_first = rootfs1_seq > rootfs2_seq;
> + else
> + use_first = rootfs1_seq < rootfs2_seq;
> +
> + pr_info("CFE bootline selected %s image rootfs%u (rootfs1_seq=%u, rootfs2_seq=%u)\n",
> + use_latest ? "latest" : "previous",
> + use_first ? 1 : 2,
> + rootfs1_seq, rootfs2_seq);
> + } else {
> + use_first = rootfs1;
> + }
> +
> + /* Partitions:
> + * 1 boot
> + * 2 rootfs
> + * 3 data
> + * 4 rootfs1_update
> + * 5 rootfs2_update
> + * 6 rootfs_other
> + */
> + nrparts = 6;
> + i = 0;
> +
> + parts = kcalloc(nrparts, sizeof(*parts), GFP_KERNEL);
> + if (!parts)
> + return -ENOMEM;
> +
> + parts[i].name = "boot";
> + parts[i].offset = BCM963XX_NVRAM_NAND_PART_OFFSET(nvram, BOOT);
> + parts[i].size = BCM963XX_NVRAM_NAND_PART_SIZE(nvram, BOOT);
> + i++;
> +
> + /* Primary rootfs if either is available */
> + if (rootfs1 || rootfs2) {
> + parts[i].name = "rootfs";
> + parts[i].offset = use_first ? rootfs1_off : rootfs2_off;
> + parts[i].size = use_first ? rootfs1_size : rootfs2_size;
> + i++;
> + }
> +
> + parts[i].name = "data";
> + parts[i].offset = BCM963XX_NVRAM_NAND_PART_OFFSET(nvram, DATA);
> + parts[i].size = BCM963XX_NVRAM_NAND_PART_SIZE(nvram, DATA);
> + i++;
> +
> + /* Full rootfs partitions for updates */
> + parts[i].name = "rootfs1_update";
> + parts[i].offset = BCM963XX_NVRAM_NAND_PART_OFFSET(nvram, ROOTFS_1);
> + parts[i].size = BCM963XX_NVRAM_NAND_PART_SIZE(nvram, ROOTFS_1);
> + i++;
> +
> + parts[i].name = "rootfs2_update";
> + parts[i].offset = BCM963XX_NVRAM_NAND_PART_OFFSET(nvram, ROOTFS_2);
> + parts[i].size = BCM963XX_NVRAM_NAND_PART_SIZE(nvram, ROOTFS_2);
> + i++;
> +
> + /* Other rootfs if both are available */
> + if (rootfs1 && rootfs2) {
> + parts[i].name = "rootfs_other";
> + parts[i].offset = use_first ? rootfs2_off : rootfs1_off;
> + parts[i].size = use_first ? rootfs2_size : rootfs1_size;
> + i++;
> + }
> +
> + *pparts = parts;
> + return nrparts;
> +}
> +
> static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
> const struct mtd_partition **pparts,
> struct mtd_part_parser_data *data)
> @@ -304,7 +487,7 @@ static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
> if (!mtd_type_is_nand(master))
> ret = bcm63xx_parse_cfe_nor_partitions(master, pparts, nvram);
> else
> - ret = -EINVAL;
> + ret = bcm63xx_parse_cfe_nand_partitions(master, pparts, nvram);
>
> out:
> vfree(nvram);
> @@ -321,5 +504,6 @@ MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Daniel Dickinson <openwrt@xxxxxxxxxxxxxxxxxxxxx>");
> MODULE_AUTHOR("Florian Fainelli <florian@xxxxxxxxxxx>");
> MODULE_AUTHOR("Mike Albon <malbon@xxxxxxxxxxx>");
> -MODULE_AUTHOR("Jonas Gorski <jonas.gorski@xxxxxxxxx");
> +MODULE_AUTHOR("Jonas Gorski <jonas.gorski@xxxxxxxxx>");
> +MODULE_AUTHOR("Simon Arlott");
> MODULE_DESCRIPTION("MTD partitioning for BCM63XX CFE bootloaders");
> --
> 2.1.4
>
> --
> Simon Arlott

Next message: KY Srinivasan: "RE: [PATCH] storvsc: use small sg_tablesize on x86"
Previous message: Greg KH: "Re: Move wlan-ng out of staging?"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]