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

Bart Van Assche
2019-12-13T21:00:05Z

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 PM_DEVFREQ
> select DEVFREQ_GOV_SIMPLE_ONDEMAND
> select NLS
> + select BLK_DEV_BSGLIB
> ---help---
> 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?

> +late_initcall_sync(ufs_bsg_init);
> +module_exit(ufs_bsg_exit);

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;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_get_hba_list_lock);

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

Thanks,

Bart.