Messages in this thread | | | Subject | Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg | From | Bart Van Assche <> | Date | Fri, 13 Dec 2019 13:59:18 -0700 |
| |
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.
| |