Re: [PATCH v2 6/9] ufs: core: Move the ufshcd_device_init(hba, true) call

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Thu, Aug 22, 2024 at 02:36:07PM -0700, Bart Van Assche wrote:
> Move the ufshcd_device_init(hba, true) call from ufshcd_async_scan()
> into ufshcd_init(). This patch prepares for moving both scsi_add_host()
> calls into ufshcd_add_scsi_host(). Calling ufshcd_device_init() from
> ufshcd_init() without holding hba->host_sem is safe because
> hba->host_sem serializes core code with sysfs callback code and because
> the ufshcd_device_init() is moved before the scsi_add_host() call.

I think this last sentence is not complete.

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

One nitpick below.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> ---
>  drivers/ufs/core/ufshcd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 5c8751672bc7..0fdf19889191 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8933,10 +8933,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  	int ret;
>  
>  	down(&hba->host_sem);
> -	/* Initialize hba, detect and initialize UFS device */
> -	ret = ufshcd_device_init(hba, /*init_dev_params=*/true);
> -	if (ret == 0)
> -		ret = ufshcd_probe_hba(hba);
> +	ret = ufshcd_probe_hba(hba);
>  	up(&hba->host_sem);
>  	if (ret)
>  		goto out;
> @@ -10632,6 +10629,11 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  	 */
>  	ufshcd_set_ufs_dev_active(hba);
>  
> +	/* Initialize hba, detect and initialize UFS device */
> +	err = ufshcd_device_init(hba, /*init_dev_params=*/true);

I think this inline comment is not really needed. It is rather decreasing code
readability.

- Mani

> +	if (err)
> +		goto out_disable;
> +
>  	err = ufshcd_add_scsi_host(hba);
>  	if (err)
>  		goto out_disable;

-- 
மணிவண்ணன் சதாசிவம்