Re: [PATCH v1 03/16] ufs: core: Defer adding host to scsi if mcq is supported

[Asutosh Das]

On Fri, Sep 30 2022 at 11:32 -0700, Bart Van Assche wrote:
> On 9/22/22 18:05, Asutosh Das wrote:
> > @@ -8218,6 +8219,14 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
> >  		ret = ufshcd_device_params_init(hba);
> >  		if (ret)
> >  			goto out;
> > +
> > +		if (is_mcq_supported(hba)) {
> > +			ret = scsi_add_host(host, hba->dev);
> > +			if (ret) {
> > +				dev_err(hba->dev, "scsi_add_host failed\n");
> > +				goto out;
> > +			}
> > +		}
> >  	}
>
> Calling scsi_add_host() from ufshcd_probe_hba() seems wrong to me 
> since that function is not only called when probing a host controller 
> but also when resetting a host controller. See also 
> ufshcd_host_reset_and_restore().
The scsi_add_host() is only invoked from ufshcd_probe_hba() if init_dev_params
is true. That flag is false when ufshcd_probe_hba() is invoked from
ufshcd_host_reset_and_restore(). So scsi_add_host() won't be invoked from
ufshcd_host_reset_and_restore(). Even ufshcd_device_params_init() is invoked the
same way now.

> >  	ufshcd_tune_unipro_params(hba);
> > @@ -9764,10 +9773,12 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
> >  		hba->is_irq_enabled = true;
> >  	}
> > -	err = scsi_add_host(host, hba->dev);
> > -	if (err) {
> > -		dev_err(hba->dev, "scsi_add_host failed\n");
> > -		goto out_disable;
> > +	if (!is_mcq_supported(hba)) {
> > +		err = scsi_add_host(host, hba->dev);
> > +		if (err) {
> > +			dev_err(hba->dev, "scsi_add_host failed\n");
> > +			goto out_disable;
> > +		}
> >  	}
> >  	hba->tmf_tag_set = (struct blk_mq_tag_set) {
>
> Please make sure there is only a single scsi_add_host() call in the 
> UFS host controller driver.
One way would be to move the scsi_add_host() to ufshcd_probe_hba().
PLMK if you think this is not OK.

> Thanks,
>
> Bart.