Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 2/3] scsi: ufs: Modulize ufs-bsg
To:     Can Guo <cang@codeaurora.org>, asutoshd@codeaurora.org,
        nguyenb@codeaurora.org, rnayak@codeaurora.org,
        linux-scsi@vger.kernel.org, kernel-team@android.com,
        saravanak@google.com, salyzyn@google.com
Cc:     Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>,
        Pedro Sousa <pedrom.sousa@synopsys.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Evan Green <evgreen@chromium.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Stephen Boyd <swboyd@chromium.org>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Bean Huo <beanhuo@micron.com>,
        Venkat Gopalakrishnan <venkatg@codeaurora.org>,
        Tomas Winkler <tomas.winkler@intel.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>,
        open list <linux-kernel@vger.kernel.org>
References: <1576054123-16417-1-git-send-email-cang@codeaurora.org>
 <0101016ef425e749-1808e138-740e-4036-922f-7a49ec02c2b8-000000@us-west-2.amazonses.com>
From:   Bart Van Assche <bvanassche@acm.org>
Message-ID: <9c31a7f5-a39b-02e6-350f-5fe51f1c4275@acm.org>
Date:   Fri, 13 Dec 2019 13:59:18 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <0101016ef425e749-1808e138-740e-4036-922f-7a49ec02c2b8-000000@us-west-2.amazonses.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.