[PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[Chanho Park]

By doing scan as asynchronous way, scsi device scannning can be out of
order execution. It is no problem if there is a ufs host but the scsi
device name of each host can be changed according to the scan sequences.

Ideal Case) host0 scan first
host0 will be started from /dev/sda
 -> /dev/sdb (BootLUN0 of host0)
 -> /dev/sdc (BootLUN1 of host1)
host1 will be started from /dev/sdd

This might be an ideal case and we can easily find the host device by
this mappinng.

However, Abnormal Case) host1 scan first,
host1 will be started from /dev/sda and host0 will be followed later.

To make sure the scan sequences according to the host, we can use a
bitmap which hosts are scanned and wait until previous hosts are
finished to scan.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b8f573a02713..1ced5996e988 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -13,6 +13,7 @@
 #include <linux/devfreq.h>
 #include <linux/nls.h>
 #include <linux/of.h>
+#include <linux/bitmap.h>
 #include <linux/bitfield.h>
 #include <linux/blk-pm.h>
 #include <linux/blkdev.h>
@@ -214,6 +215,10 @@ static struct ufs_dev_fix ufs_fixups[] = {
 	END_FIX
 };
 
+/* Ordered scan host */
+static unsigned long scanned_hosts = 0;
+static wait_queue_head_t scan_wq = __WAIT_QUEUE_HEAD_INITIALIZER(scan_wq);
+
 static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba);
 static void ufshcd_async_scan(void *data, async_cookie_t cookie);
 static int ufshcd_reset_and_restore(struct ufs_hba *hba);
@@ -7709,8 +7714,13 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	if (ret)
 		goto out;
 
-	/* Probe and add UFS logical units  */
+	/* Probe and add UFS logical units. Sequential scan by host_no */
+	wait_event(scan_wq,
+		   find_first_zero_bit(&scanned_hosts, hba->host->max_id) ==
+		   hba->host->host_no);
 	ret = ufshcd_add_lus(hba);
+	set_bit(hba->host->host_no, &scanned_hosts);
+	wake_up(&scan_wq);
 out:
 	/*
 	 * If we failed to initialize the device or the device is not
-- 
2.28.0

Re: [PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[Bart Van Assche]

On 10/20/20 12:05 AM, Chanho Park wrote:
> By doing scan as asynchronous way, scsi device scannning can be out of
> order execution. It is no problem if there is a ufs host but the scsi
> device name of each host can be changed according to the scan sequences.
> 
> Ideal Case) host0 scan first
> host0 will be started from /dev/sda
>  -> /dev/sdb (BootLUN0 of host0)
>  -> /dev/sdc (BootLUN1 of host1)
> host1 will be started from /dev/sdd
> 
> This might be an ideal case and we can easily find the host device by
> this mappinng.
> 
> However, Abnormal Case) host1 scan first,
> host1 will be started from /dev/sda and host0 will be followed later.
> 
> To make sure the scan sequences according to the host, we can use a
> bitmap which hosts are scanned and wait until previous hosts are
> finished to scan.

This sounds completely wrong to me. No code should depend on the order in
which LUNs are scanned. Please use the soft links created by udev instead
of serializing LUN scanning.

Thanks,

Bart.

RE: [PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[chanho61.park]

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

Hi,

> -----Original Message-----
> From: Bart Van Assche <bvanassche@acm.org>
> Sent: Wednesday, October 21, 2020 12:15 PM
> To: Chanho Park <chanho61.park@samsung.com>; jejb@linux.ibm.com;
> martin.petersen@oracle.com
> Cc: alim.akhtar@samsung.com; avri.altman@wdc.com; linux-
> scsi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] scsi: ufs: make sure scan sequence for multiple
hosts
> 
> On 10/20/20 12:05 AM, Chanho Park wrote:
> > By doing scan as asynchronous way, scsi device scannning can be out of
> > order execution. It is no problem if there is a ufs host but the scsi
> > device name of each host can be changed according to the scan
sequences.
> >
> > Ideal Case) host0 scan first
> > host0 will be started from /dev/sda
> >  -> /dev/sdb (BootLUN0 of host0)
> >  -> /dev/sdc (BootLUN1 of host1)
> > host1 will be started from /dev/sdd
> >
> > This might be an ideal case and we can easily find the host device by
> > this mappinng.
> >
> > However, Abnormal Case) host1 scan first,
> > host1 will be started from /dev/sda and host0 will be followed later.
> >
> > To make sure the scan sequences according to the host, we can use a
> > bitmap which hosts are scanned and wait until previous hosts are
> > finished to scan.
> 
> This sounds completely wrong to me. No code should depend on the order in
> which LUNs are scanned. Please use the soft links created by udev instead
> of serializing LUN scanning.
> 

Thanks for your review.
Did you mean /dev/disk/by-[part]label/ symlink? It's quite reasonable to
use them by udev in userspace such as initramfs but some cases does not use
initramfs or initrd. In that case, we need to load the root
device(/dev/sda[N]) directly from kernel.
Anyway, scsi disk(sd) case, the scan order will not be changed until the
port has changed so they'll have permanent device names. I'd like to make
permanent UFS device names.

Best Regards,
Chanho Park

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 2630 bytes --]

Re: [PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[Bart Van Assche]

On 10/20/20 9:23 PM, chanho61.park wrote:
> Did you mean /dev/disk/by-[part]label/ symlink? It's quite reasonable to
> use them by udev in userspace such as initramfs but some cases does not use
> initramfs or initrd. In that case, we need to load the root
> device(/dev/sda[N]) directly from kernel.

Please use udev or systemd instead of adding code in the UFS driver that is
not necessary when udev or systemd is used.

Thanks,

Bart.

RE: [PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[Chanho Park]

> > Did you mean /dev/disk/by-[part]label/ symlink? It's quite reasonable to
> > use them by udev in userspace such as initramfs but some cases does not
> use
> > initramfs or initrd. In that case, we need to load the root
> > device(/dev/sda[N]) directly from kernel.
> 
> Please use udev or systemd instead of adding code in the UFS driver that
> is
> not necessary when udev or systemd is used.
>

What I mentioned was how it can be handled when we mount rootfs directly from kernel.

1) kernel -> initramfs (mount root) -> systemd
2) kernel (mount root) -> systemd
 -> In this case, we normally use root=/dev/sda1 from kernel commandline to mount the rootfs.

Like fstab can support legacy node mount, ufs driver also needs to provide an option for using the permanent legacy node. If you're really worry about adding a new codes for all UFS driver, we can put this as controller specific or optional feature.

Best Regards,
Chanho Park

RE: [PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[Avri Altman]

> 
> > > Did you mean /dev/disk/by-[part]label/ symlink? It's quite reasonable to
> > > use them by udev in userspace such as initramfs but some cases does not
> > use
> > > initramfs or initrd. In that case, we need to load the root
> > > device(/dev/sda[N]) directly from kernel.
> >
> > Please use udev or systemd instead of adding code in the UFS driver that
> > is
> > not necessary when udev or systemd is used.
> >
> 
> What I mentioned was how it can be handled when we mount rootfs directly
> from kernel.
> 
> 1) kernel -> initramfs (mount root) -> systemd
> 2) kernel (mount root) -> systemd
>  -> In this case, we normally use root=/dev/sda1 from kernel commandline to
> mount the rootfs.
> 
> Like fstab can support legacy node mount, ufs driver also needs to provide an
> option for using the permanent legacy node. If you're really worry about adding
> a new codes for all UFS driver, we can put this as controller specific or optional
> feature.
In case you'll convince Bart that this code is needed, maybe use a IDA handle for that?

Thanks,
Avri

RE: [PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[Chanho Park]

> In case you'll convince Bart that this code is needed, maybe use a IDA
> handle for that?

Thanks for your suggestion.
IDA is useful to maintain unique IDs with bitmap structure but the patch assumes host_no is the unique number and can be used in the bitmap for making sequential order.

Best Regards,
Chanho Park

Re: [PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[Bart Van Assche]

On 10/21/20 9:59 PM, Chanho Park wrote:
>> Please use udev or systemd instead of adding code in the UFS driver that
>> is
>> not necessary when udev or systemd is used.
>>
> 
> What I mentioned was how it can be handled when we mount rootfs directly from kernel.
> 
> 1) kernel -> initramfs (mount root) -> systemd
> 2) kernel (mount root) -> systemd
>   -> In this case, we normally use root=/dev/sda1 from kernel commandline to mount the rootfs.
> 
> Like fstab can support legacy node mount, ufs driver also needs to provide
> an option for using the permanent legacy node. If you're really worry about > adding a new codes for all UFS driver, we can put this as controller specific> or optional feature.

The only code that should occur in the UFS driver is code that is specific
to UFS devices. Since the functionality in this patch is not UFS specific,
it should be added elsewhere.

Additionally, since this functionality can be implemented easily in user
space, I think it should be implemented in user space instead of in the
kernel. Even when not using systemd or udev, it is easy to implement a loop
that waits until a certain LUN or WWID appears.

Bart.

[PATCH] scsi: ufs: make sure scan sequence for multiple hosts

[Chanho Park]

By doing scan as asynchronous way, scsi device scannning can be out of
order execution. It is no problem if there is a ufs host but the scsi
device name of each host can be changed according to the scan sequences.

Ideal Case) host0 scan first
host0 will be started from /dev/sda
 -> /dev/sdb (BootLUN0 of host0)
 -> /dev/sdc (BootLUN1 of host1)
host1 will be started from /dev/sdd

This might be an ideal case and we can easily find the host device by
this mappinng.

However, Abnormal Case) host1 scan first,
host1 will be started from /dev/sda and host0 will be followed later.

To make sure the scan sequences according to the host, we can use a
bitmap which hosts are scanned and wait until previous hosts are
finished to scan.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b8f573a02713..1ced5996e988 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -13,6 +13,7 @@
 #include <linux/devfreq.h>
 #include <linux/nls.h>
 #include <linux/of.h>
+#include <linux/bitmap.h>
 #include <linux/bitfield.h>
 #include <linux/blk-pm.h>
 #include <linux/blkdev.h>
@@ -214,6 +215,10 @@ static struct ufs_dev_fix ufs_fixups[] = {
 	END_FIX
 };
 
+/* Ordered scan host */
+static unsigned long scanned_hosts = 0;
+static wait_queue_head_t scan_wq = __WAIT_QUEUE_HEAD_INITIALIZER(scan_wq);
+
 static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba);
 static void ufshcd_async_scan(void *data, async_cookie_t cookie);
 static int ufshcd_reset_and_restore(struct ufs_hba *hba);
@@ -7709,8 +7714,13 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	if (ret)
 		goto out;
 
-	/* Probe and add UFS logical units  */
+	/* Probe and add UFS logical units. Sequential scan by host_no */
+	wait_event(scan_wq,
+		   find_first_zero_bit(&scanned_hosts, hba->host->max_id) ==
+		   hba->host->host_no);
 	ret = ufshcd_add_lus(hba);
+	set_bit(hba->host->host_no, &scanned_hosts);
+	wake_up(&scan_wq);
 out:
 	/*
 	 * If we failed to initialize the device or the device is not
-- 
2.28.0