[v5,06/10] scsi: ufs: core: Move the ufshcd_device_init(hba, true) call

ufshcd_async_scan() is called (asynchronously) only by ufshcd_init().
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. This is safe because
hba->host_sem serializes core code and sysfs callbacks. The
ufshcd_device_init() call is moved before the scsi_add_host() call and
hence happens before any SCSI sysfs attributes are created.

Since ufshcd_add_scsi_host() checks whether or not the MCQ mode is
enabled and since ufshcd_device_init() may modify hba->mcq_enabled,
only call scsi_add_host() from ufshcd_device_init() if the SCSI host
has not yet been added by ufshcd_device_init().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 16 +++++++++-------
 include/ufs/ufshcd.h      |  3 +++
 2 files changed, 12 insertions(+), 7 deletions(-)

On 9/10/2024 2:50 PM, Bart Van Assche wrote:
> ufshcd_async_scan() is called (asynchronously) only by ufshcd_init().
> 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. This is safe because
> hba->host_sem serializes core code and sysfs callbacks. The
> ufshcd_device_init() call is moved before the scsi_add_host() call and
> hence happens before any SCSI sysfs attributes are created.
> 
> Since ufshcd_add_scsi_host() checks whether or not the MCQ mode is
> enabled and since ufshcd_device_init() may modify hba->mcq_enabled,
> only call scsi_add_host() from ufshcd_device_init() if the SCSI host
> has not yet been added by ufshcd_device_init().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufshcd.c | 16 +++++++++-------
>   include/ufs/ufshcd.h      |  3 +++
>   2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index f62d257a92da..a3c5493ccc8f 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8907,16 +8907,12 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
>   static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>   {
>   	struct ufs_hba *hba = (struct ufs_hba *)data;
> -	ktime_t device_init_start;
>   	int ret;
>   
>   	down(&hba->host_sem);
>   	/* Initialize hba, detect and initialize UFS device */
> -	device_init_start = ktime_get();
> -	ret = ufshcd_device_init(hba, /*init_dev_params=*/true);
> -	if (ret == 0)
> -		ret = ufshcd_probe_hba(hba, true);
> -	ufshcd_process_device_init_result(hba, device_init_start, ret);
> +	ret = ufshcd_probe_hba(hba, true);
> +	ufshcd_process_device_init_result(hba, hba->device_init_start, ret);

This patch probably changed the original trace print. In the original 
code, the trace only prints the time spent in probe_hba().
In this patch however the trace print includes the time spent in 
ufshcd_add_scsi_host() plus the time wait for 
async_schedule(ufshcd_async_scan, hba).

>   	up(&hba->host_sem);
>   	if (ret)
>   		goto out;
> @@ -10387,7 +10383,7 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
>   {
>   	int err;
>   
> -	if (!is_mcq_supported(hba)) {
> +	if (!hba->scsi_host_added) {
>   		err = scsi_add_host(hba->host, hba->dev);
>   		if (err) {
>   			dev_err(hba->dev, "scsi_add_host failed\n");
> @@ -10610,6 +10606,12 @@ 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 */
> +	hba->device_init_start = ktime_get();
> +	err = ufshcd_device_init(hba, /*init_dev_params=*/true);
> +	if (err)
> +		goto out_disable;
> +
>   	err = ufshcd_add_scsi_host(hba);
>   	if (err)
>   		goto out_disable;
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index a43b14276bc3..55ec89fa16af 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -839,6 +839,7 @@ enum ufshcd_mcq_opr {
>    * @dev: device handle
>    * @ufs_device_wlun: WLUN that controls the entire UFS device.
>    * @hwmon_device: device instance registered with the hwmon core.
> + * @device_init_start: start time of first ufshcd_device_init() call.
>    * @curr_dev_pwr_mode: active UFS device power mode.
>    * @uic_link_state: active state of the link to the UFS device.
>    * @rpm_lvl: desired UFS power management level during runtime PM.
> @@ -972,6 +973,8 @@ struct ufs_hba {
>   	struct device *hwmon_device;
>   #endif
>   
> +	ktime_t device_init_start;

IMO, it's kinda overkill to add this device_init_start member for the 
purpose of trace print the time spent in the first probe_hba() only.
How about print the time spent in probe_hba() outside of the 
ufshcd_process_device_init_result() function so that you don't need to 
pass device_init_start into ufshcd_process_device_init_result()?

> +
>   	enum ufs_dev_pwr_mode curr_dev_pwr_mode;
>   	enum uic_link_state uic_link_state;
>   	/* Desired UFS power management level during runtime PM */
>

On 9/12/24 10:29 PM, Bao D. Nguyen wrote:
> On 9/10/2024 2:50 PM, Bart Van Assche wrote:
() plus the time wait for
> async_schedule(ufshcd_async_scan, hba).

The above comment is not clear to me. I'm not aware of any code in the
UFS driver that waits for the asynchronously called ufshcd_async_scan()
function to finish? It's not clear to me either how the time spent in
ufshcd_add_scsi_host() would be included in the trace print?

>> +    ktime_t device_init_start;
> 
> IMO, it's kinda overkill to add this device_init_start member for the 
> purpose of trace print the time spent in the first probe_hba() only.
> How about print the time spent in probe_hba() outside of the 
> ufshcd_process_device_init_result() function so that you don't need to 
> pass device_init_start into ufshcd_process_device_init_result()?

I will remove the above struct ufs_hba member again.

Thanks,

Bart.