Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg

From: Bart Van Assche
Date: Fri Dec 13 2019 - 16:00:06 EST

On 12/11/19 3:49 AM, Can Guo wrote:
In order to improve the flexibility of ufs-bsg, modulizing it is a good
choice. This change introduces tristate to ufs-bsg to allow users compile
it as an external module.

Did you perhaps mean "modularize" instead of "modulize"? Additionally, should "modulizing" perhaps be changed into "modularizing"?

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index d14c224..72620ce 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -38,6 +38,7 @@ config SCSI_UFSHCD
select NLS
This selects the support for UFS devices in Linux, say Y and make
sure that you know the name of your UFS host adapter (the card

I do not understand the above change. Doesn't moving the BSG code into a separate module remove the dependency of SCSI_UFSHCD on BLK_DEV_BSGLIB?

+static int __init ufs_bsg_init(void)
+ struct list_head *hba_list = NULL;
+ struct ufs_hba *hba;
+ int ret = 0;
+ ufshcd_get_hba_list_lock(&hba_list);
+ list_for_each_entry(hba, hba_list, list) {
+ ret = ufs_bsg_probe(hba);
+ if (ret)
+ break;
+ }
+ ufshcd_put_hba_list_unlock();
+ return ret;

What if ufs_bsg_probe() succeeds for some UFS adapters but not for others? Shouldn't ufs_bgs_remove() be called in that case for the adapters for which ufs_bsg_probe() succeeded?


Why late_initcall_sync() instead of module_init()?

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a86b0fd..7a83a8f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -108,6 +108,22 @@
16, 4, buf, __len, false); \
} while (0)
+static LIST_HEAD(ufs_hba_list);
+static DEFINE_MUTEX(ufs_hba_list_lock);
+void ufshcd_get_hba_list_lock(struct list_head **list)
+ mutex_lock(&ufs_hba_list_lock);
+ *list = &ufs_hba_list;

Please make ufshcd_get_hba_list_lock() return the list_head pointer instead of the above.