Re: [PATCH 1/1] ubi: Allow ubiblock devices nodes to be created by volume name instead of volume ID.

[Richard Weinberger <richard.weinberger@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2661 bytes --]

On Wed, Aug 14, 2019 at 2:34 PM Emil Lenngren <emil.lenngren@gmail.com> wrote:
> > Isn't this why we have udev, to create fancy by-id/by-path/... naming conventions?
> >
> > Thanks,
> > //richard
>
> I actually implemented the same change some time ago, for the exact
> same reason (swapping two volumes and then reboot). Referring to an
> ubi volume by name is more convenient than volume numbers, since names
> can be changed and numbers can't.
> Is it maybe possible to define both /dev/ubiblock%d_%d and
> /dev/ubiblock%d_%s at the same time?
> How would this be done with udev instead to get "fancy by-id"?

Thanks for bringing this topic up again.
udev allows rules and even helper (scripts) to gain device
attributes. These can then be used to name/symlink devices as you want.

For example, if you want to have device symlinks by name for UBI
volumes, you can create a rule like that:
KERNEL=="ubi?_?", IMPORT{program}="/etc/udev/rules.d/ubi_probe
$devnode", SYMLINK="ubi%E{MTD_NUM}_%E{VOL_NAME}"

With ubi_probe being:
#!/bin/sh
VOL_NAME="$(cat /sys/${DEVPATH}/name)"
MTD_NUM="$(cat /sys/${DEVPATH}/../mtd_num)"
echo "VOL_NAME=${VOL_NAME}"
echo "MTD_NUM=${MTD_NUM}"

I took the chance and reviewed UBI's sysfs interface and found a few
things which need
improvement. With these issues addressed you can also work with
ubiblock by name:
1. UBI does not export the UBI device number via sysfs. In 99% of all
cases mtd_num will be correct,
but you can select the UBI number upon attach time.
2. UBI does not emit a change kevent when an volume is renamed. So
udev does not see the rename command.
3. ubiblock does not set the UBI volume as parent device.

So we can then have a rule like:
KERNEL=="ubiblock?_?",
IMPORT{program}="/etc/udev/rules.d/ubiblock_probe $devnode"
SYMLINK="ubiblock$number_%E{VOL_NAME}"

With ubiblock_probe being:
#!/bin/sh
VOL_NAME="$(cat /sys/${DEVPATH}/device/name)"
echo "VOL_NAME=${VOL_NAME}"

# ls -ltr /dev/ubiblock*
brw-rw---- 1 root disk 251, 0 18. Aug 22:41 /dev/ubiblock7_0
lrwxrwxrwx 1 root root     11 18. Aug 22:41 /dev/ubiblock0_test -> ubiblock7_0
# ubirename /dev/ubi7 test lala
# ls -ltr /dev/ubiblock*
brw-rw---- 1 root disk 251, 0 18. Aug 22:42 /dev/ubiblock7_0
lrwxrwxrwx 1 root root     11 18. Aug 22:42 /dev/ubiblock0_lala -> ubiblock7_0

Does this help? If you need other/more sysfs changes, please tell. :-)
Attached you can find my WIP patch for these changes. I need to double check
a few things first before I will send a formal patch.

I think it would even make sense to integrate a more powerful
ubi_probe (as C program) into systemd-udev
like we already have for mtd itself.

-- 
Thanks,
//richard

[-- Attachment #2: ubi_sysfs_wip.diff --]
[-- Type: text/x-patch, Size: 5625 bytes --]

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 6025398955a2..6caab85907bd 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -345,7 +345,7 @@ static const struct blk_mq_ops ubiblock_mq_ops = {
 	.init_request	= ubiblock_init_request,
 };
 
-int ubiblock_create(struct ubi_volume_info *vi)
+int ubiblock_create(struct ubi_volume_info *vi, struct device *ubi_dev)
 {
 	struct ubiblock *dev;
 	struct gendisk *gd;
@@ -433,7 +433,7 @@ int ubiblock_create(struct ubi_volume_info *vi)
 	list_add_tail(&dev->list, &ubiblock_devices);
 
 	/* Must be the last step: anyone can call file ops from now on */
-	add_disk(dev->gd);
+	device_add_disk(ubi_dev, dev->gd, NULL);
 	dev_info(disk_to_dev(dev->gd), "created from ubi%d:%d(%s)",
 		 dev->ubi_num, dev->vol_id, vi->name);
 	mutex_unlock(&devices_mutex);
@@ -504,6 +504,23 @@ int ubiblock_remove(struct ubi_volume_info *vi)
 	return ret;
 }
 
+static void ubiblock_notify_rename(struct ubi_volume_info *vi)
+{
+	struct ubiblock *dev;
+
+	mutex_lock(&devices_mutex);
+	dev = find_dev_nolock(vi->ubi_num, vi->vol_id);
+	if (!dev) {
+		mutex_unlock(&devices_mutex);
+		return;
+	}
+
+	mutex_lock(&dev->dev_mutex);
+	kobject_uevent(&disk_to_dev(dev->gd)->kobj, KOBJ_CHANGE);
+	mutex_unlock(&dev->dev_mutex);
+	mutex_unlock(&devices_mutex);
+}
+
 static int ubiblock_resize(struct ubi_volume_info *vi)
 {
 	struct ubiblock *dev;
@@ -565,6 +582,8 @@ static int ubiblock_notify(struct notifier_block *nb,
 		if (nt->vi.vol_type == UBI_STATIC_VOLUME)
 			ubiblock_resize(&nt->vi);
 		break;
+	case UBI_VOLUME_RENAMED:
+		ubiblock_notify_rename(&nt->vi);
 	default:
 		break;
 	}
@@ -613,9 +632,8 @@ static void __init ubiblock_create_from_param(void)
 		}
 
 		ubi_get_volume_info(desc, &vi);
+		ret = ubiblock_create(&vi, &desc->vol->dev);
 		ubi_close_volume(desc);
-
-		ret = ubiblock_create(&vi);
 		if (ret) {
 			pr_err(
 			       "UBI: block: can't add '%s' volume on ubi%d_%d, err=%d\n",
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index d636bbe214cb..134492a1718c 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -140,6 +140,8 @@ static struct device_attribute dev_mtd_num =
 	__ATTR(mtd_num, S_IRUGO, dev_attribute_show, NULL);
 static struct device_attribute dev_ro_mode =
 	__ATTR(ro_mode, S_IRUGO, dev_attribute_show, NULL);
+static struct device_attribute dev_ubi_num =
+	__ATTR(ubi_num, S_IRUGO, dev_attribute_show, NULL);
 
 /**
  * ubi_volume_notify - send a volume change notification.
@@ -378,6 +380,8 @@ static ssize_t dev_attribute_show(struct device *dev,
 		ret = sprintf(buf, "%d\n", ubi->mtd->index);
 	else if (attr == &dev_ro_mode)
 		ret = sprintf(buf, "%d\n", ubi->ro_mode);
+	else if (attr == &dev_ubi_num)
+		ret = sprintf(buf, "%d\n", ubi->ubi_num);
 	else
 		ret = -EINVAL;
 
@@ -398,6 +402,7 @@ static struct attribute *ubi_dev_attrs[] = {
 	&dev_bgt_enabled.attr,
 	&dev_mtd_num.attr,
 	&dev_ro_mode.attr,
+	&dev_ubi_num.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(ubi_dev);
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 1b77fff9f892..a7ac50cecc05 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -562,7 +562,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 		struct ubi_volume_info vi;
 
 		ubi_get_volume_info(desc, &vi);
-		err = ubiblock_create(&vi);
+		err = ubiblock_create(&vi, &desc->vol->dev);
 		break;
 	}
 
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 721b6aa7936c..cb90d61f5f07 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -976,12 +976,12 @@ static inline void ubi_fastmap_destroy_checkmap(struct ubi_volume *vol) {}
 #ifdef CONFIG_MTD_UBI_BLOCK
 int ubiblock_init(void);
 void ubiblock_exit(void);
-int ubiblock_create(struct ubi_volume_info *vi);
+int ubiblock_create(struct ubi_volume_info *vi, struct device *dev);
 int ubiblock_remove(struct ubi_volume_info *vi);
 #else
 static inline int ubiblock_init(void) { return 0; }
 static inline void ubiblock_exit(void) {}
-static inline int ubiblock_create(struct ubi_volume_info *vi)
+static inline int ubiblock_create(struct ubi_volume_info *vi, struct device *dev)
 {
 	return -ENOSYS;
 }
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 139ee132bfbc..c18b5739ca8e 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -38,6 +38,8 @@ static struct device_attribute attr_vol_data_bytes =
 	__ATTR(data_bytes, S_IRUGO, vol_attribute_show, NULL);
 static struct device_attribute attr_vol_upd_marker =
 	__ATTR(upd_marker, S_IRUGO, vol_attribute_show, NULL);
+static struct device_attribute attr_vol_id =
+	__ATTR(id, S_IRUGO, vol_attribute_show, NULL);
 
 /*
  * "Show" method for files in '/<sysfs>/class/ubi/ubiX_Y/'.
@@ -94,6 +96,8 @@ static ssize_t vol_attribute_show(struct device *dev,
 		ret = sprintf(buf, "%lld\n", vol->used_bytes);
 	else if (attr == &attr_vol_upd_marker)
 		ret = sprintf(buf, "%d\n", vol->upd_marker);
+	else if (attr == &attr_vol_id)
+		ret = sprintf(buf, "%d\n", vol->vol_id);
 	else
 		/* This must be a bug */
 		ret = -EINVAL;
@@ -116,6 +120,7 @@ static struct attribute *volume_dev_attrs[] = {
 	&attr_vol_usable_eb_size.attr,
 	&attr_vol_data_bytes.attr,
 	&attr_vol_upd_marker.attr,
+	&attr_vol_id.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(volume_dev);
@@ -555,6 +560,7 @@ int ubi_rename_volumes(struct ubi_device *ubi, struct list_head *rename_list)
 			memcpy(vol->name, re->new_name, re->new_name_len + 1);
 			spin_unlock(&ubi->volumes_lock);
 			ubi_volume_notify(ubi, vol, UBI_VOLUME_RENAMED);
+			kobject_uevent(&vol->dev.kobj, KOBJ_CHANGE);
 		}
 	}
 

[-- Attachment #3: Type: text/plain, Size: 144 bytes --]

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