[U-Boot] [PATCH v7 09/12] cmd: mtd: add 'mtd' command

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Thu,  6 Sep 2018 09:08:51 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> +
> +int mtd_probe_devices(void)
> +{
> +	const char *mtdparts = env_get("mtdparts");
> +	const char *mtdids = env_get("mtdids");
> +	bool remaining_partitions = true;
> +	struct mtd_info *mtd;
> +	int i;
> +
> +	mtd_probe_uclass_mtd_devs();
> +
> +	/* Check if mtdparts/mtdids changed since last call, otherwise: exit */
> +	if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
> +		return CMD_RET_SUCCESS;
> +
> +	/* Update the local copy of mtdparts */
> +	free(old_mtdparts);
> +	free(old_mtdids);
> +	old_mtdparts = strdup(mtdparts);
> +	old_mtdids = strdup(mtdids);
> +
> +	/* If at least one partition is still in use, do not delete anything */
> +	mtd_for_each_device(mtd) {
> +		if (mtd->usecount) {
> +			printf("Partition \"%s\" already in use, aborting\n",
> +			       mtd->name);
> +			return CMD_RET_FAILURE;
> +		}
> +	}
> +
> +	/*
> +	 * Everything looks clear, remove all partitions. It is not safe to
> +	 * remove entries from the mtd_for_each_device loop as it uses idr
> +	 * indexes and the partitions removal is done in bulk (all partitions of
> +	 * one device at the same time), so break and iterate from start each
> +	 * time a new partition is found and deleted.
> +	 */
> +	while (remaining_partitions) {
> +		remaining_partitions = false;
> +		mtd_for_each_device(mtd) {
> +			if (!mtd_is_partition(mtd) && mtd_has_partitions(mtd)) {
> +				del_mtd_partitions(mtd);
> +				remaining_partitions = true;
> +				break;
> +			}
> +		}
> +	}

Just a detail, but I think this logic could be moved in moved in
the core (mtd_uboot.c).

> +
> +	/* Start the parsing by ignoring the extra 'mtdparts=' prefix, if any */
> +	if (strstr(mtdparts, "mtdparts="))
> +		mtdparts += 9;
> +
> +	/* For each MTD device in mtdparts */
> +	while (mtdparts[0] != '\0') {
> +		char mtd_name[MTD_NAME_MAX_LEN], *colon;
> +		struct mtd_partition *parts;
> +		int mtd_name_len, len;
> +		int ret;
> +
> +		colon = strchr(mtdparts, ':');
> +		if (!colon) {
> +			printf("Wrong mtdparts: %s\n", mtdparts);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		mtd_name_len = colon - mtdparts;
> +		strncpy(mtd_name, mtdparts, mtd_name_len);
> +		mtd_name[mtd_name_len] = '\0';
> +		/* Move the pointer forward (including the ':') */
> +		mtdparts += mtd_name_len + 1;
> +		mtd = get_mtd_device_nm(mtd_name);
> +		if (IS_ERR_OR_NULL(mtd)) {
> +			char linux_name[MTD_NAME_MAX_LEN];
> +
> +			/*
> +			 * The MTD device named "mtd_name" does not exist. Try
> +			 * to find a correspondance with an MTD device having
> +			 * the same type and number as defined in the mtdids.
> +			 */
> +			debug("No device named %s\n", mtd_name);
> +			ret = mtd_search_alternate_name(mtd_name, linux_name,
> +							MTD_NAME_MAX_LEN);
> +			if (!ret)
> +				mtd = get_mtd_device_nm(linux_name);
> +
> +			/*
> +			 * If no device could be found, move the mtdparts
> +			 * pointer forward until the next set of partitions.
> +			 */
> +			if (ret || IS_ERR_OR_NULL(mtd)) {
> +				printf("Could not find a valid device for %s\n",
> +				       mtd_name);
> +				mtdparts = strchr(mtdparts, ';');
> +				if (mtdparts)
> +					mtdparts++;
> +
> +				continue;
> +			}
> +		}
> +		put_mtd_device(mtd);

Don't know if the refcounting is a nop or not, but shouldn't we keep
a reference to mtd until we're done manipulating it (after the call to
add_mtd_partitions())?

> +
> +		/*
> +		 * Parse the MTD device partitions. It will update the mtdparts
> +		 * pointer, create an array of parts (that must be freed), and
> +		 * return the number of partition structures in the array.
> +		 */
> +		ret = mtd_parse_partitions(mtd, &mtdparts, &parts, &len);
> +		if (ret) {
> +			printf("Could not parse device %s\n", mtd->name);
> +			return CMD_RET_FAILURE;
> +		}
> +
> +		if (!len)
> +			continue;
> +
> +		/*
> +		 * Create the new MTD partitions and free the structures
> +		 * allocated during the parsing.
> +		 */
> +		add_mtd_partitions(mtd, parts, len);
> +
> +		for (i = 0; i < len; i++)
> +			free((char *)parts[i].name);
> +		free(parts);

I'd recommend creating an helper for parts deletion:

void mtd_free_partitions(struct mtd_partitions *parts,
			 unsigned int nparts)
{
	for (i = 0; i < len; i++)
		free(parts[i].name);

	free(parts);
}

> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}

Actually, I think the whole mtd_probe_devices() function (and its
dependencies) should be moved in the core (mtd_uboot.c?) so that other
components (ubi, cmd/mtdparts) can use it without depending/selecting
CONFIG_CMD_MTD.