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.