diff mbox series

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

Message ID 197a8bcca288f9c36586099aa07606ed3f067c9b.1663894792.git.quic_asutoshd@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Add Multi Circular Queue Support | expand

Commit Message

Asutosh Das Sept. 23, 2022, 1:05 a.m. UTC 
If MCQ support is present, then enabling it would need
reallocating tags and memory. It would also free up the
already allocated memory in Single Doorbell Mode.
So defer invoking scsi_add_host() until MCQ is configured.

Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Asutosh Das <quic_asutoshd@quicinc.com>
---
 drivers/ufs/core/ufshcd.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Bart Van Assche Sept. 30, 2022, 6:32 p.m. UTC | #1 
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().

>   
>   	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.

Thanks,

Bart.
Asutosh Das Oct. 4, 2022, 6:51 p.m. UTC | #2 
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.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 24661fc..426867b 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8184,6 +8184,7 @@  static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 	int ret;
 	unsigned long flags;
 	ktime_t start = ktime_get();
+	struct Scsi_Host *host = hba->host;
 
 	hba->ufshcd_state = UFSHCD_STATE_RESET;
 
@@ -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;
+			}
+		}
 	}
 
 	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) {