diff mbox series

[V3] scsi: ufs: core: Check LSDBS cap when !mcq

Message ID 20240708052542epcms2p5358e41c0116c936d2a43ad99f7c1e4a8@epcms2p5 (mailing list archive)
State Superseded
Headers show
Series [V3] scsi: ufs: core: Check LSDBS cap when !mcq | expand

Commit Message

Kyoungrul Kim July 8, 2024, 5:25 a.m. UTC
if the user set use_mcq_mode to 0, the host will try to activate the
lsdb mode unconditionally even when the lsdbs of device hci cap is 1. so
it makes timeout cmds and fail to device probing.

To prevent that problem. check the lsdbs cap when mcq is not supported
case.

Signed-off-by: k831.kim <k831.kim@samsung.com>
---
Changes to v1: Fix wrong bit of lsdb support.
Changes to v2: Fix extra space and wrong commit messeage.
---
 drivers/ufs/core/ufshcd.c | 16 ++++++++++++++++
 include/ufs/ufshcd.h      |  1 +
 include/ufs/ufshci.h      |  1 +
 3 files changed, 18 insertions(+)

Comments

Bart Van Assche July 9, 2024, 6:18 p.m. UTC | #1
On 7/7/24 10:25 PM, Kyoungrul Kim wrote:
> if the user set use_mcq_mode to 0, the host will try to activate the

set -> sets

> lsdb mode unconditionally even when the lsdbs of device hci cap is 1. so
> it makes timeout cmds and fail to device probing.

> +	/*
> +	 * UFS 3.0 has no MCQ_SUPPORT and LSDB_SUPPORT, but [31:29] as reserved
> +	 * bits with reset value 0s, which means we can simply read values
> +	 * regardless to version
> +	 */

Please change "UFS 3.0 has no" into "The UFSHCI 3.0 specification does
not define"

>   	hba->mcq_sup = FIELD_GET(MASK_MCQ_SUPPORT, hba->capabilities);
> +	/*
> +	 * 0h: legacy single doorbell support is available
> +	 * 1h: indicate that legacy single doorbell support have been removed
> +	 */

have -> has

> +	hba->lsdb_sup = !FIELD_GET(MASK_LSDB_SUPPORT, hba->capabilities);
>   	if (!hba->mcq_sup)
>   		return 0;
>   
> @@ -10449,6 +10459,12 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>   	}
>   
>   	if (!is_mcq_supported(hba)) {
> +		if (!hba->lsdb_sup) {
> +			dev_err(hba->dev, "%s: failed to initialize (legacy doorbell mode not supported\n",
> +				__func__);

A closing parenthesis is missing (")").

> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index bad88bd91995..fd391f6eee73 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1074,6 +1074,7 @@ struct ufs_hba {
>   	bool ext_iid_sup;
>   	bool scsi_host_added;
>   	bool mcq_sup;
> +	bool lsdb_sup;
>   	bool mcq_enabled;
>   	struct ufshcd_res_info res[RES_MAX];
>   	void __iomem *mcq_base;

Please update the kernel-doc comment above struct ufs_hba for the new
lsdb_sup member and please move the new member above mcq_sup such that
the mcq_sup and mcq_enabled definitions remain adjacent.

Thanks,

Bart.
Kyoungrul Kim July 9, 2024, 11:15 p.m. UTC | #2
Thanks for your comment.
I will revise that.
  
 
 
--------- Original Message ---------
Sender : Bart Van Assche <bvanassche@acm.org>
Date : 2024-07-10 03:19 (GMT+9)
Title : Re: [PATCH V3] scsi: ufs: core: Check LSDBS cap when !mcq
 
On 7/7/24 10:25 PM, Kyoungrul Kim wrote:
> if the user set use_mcq_mode to 0, the host will try to activate the

set -> sets

> lsdb mode unconditionally even when the lsdbs of device hci cap is 1. so
> it makes timeout cmds and fail to device probing.

> +        /*
> +         * UFS 3.0 has no MCQ_SUPPORT and LSDB_SUPPORT, but [31:29] as reserved
> +         * bits with reset value 0s, which means we can simply read values
> +         * regardless to version
> +         */

Please change "UFS 3.0 has no" into "The UFSHCI 3.0 specification does
not define"

>           hba->mcq_sup = FIELD_GET(MASK_MCQ_SUPPORT, hba->capabilities);
> +        /*
> +         * 0h: legacy single doorbell support is available
> +         * 1h: indicate that legacy single doorbell support have been removed
> +         */

have -> has

> +        hba->lsdb_sup = !FIELD_GET(MASK_LSDB_SUPPORT, hba->capabilities);
>           if (!hba->mcq_sup)
>                   return 0;

> @@ -10449,6 +10459,12 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>           }

>           if (!is_mcq_supported(hba)) {
> +                if (!hba->lsdb_sup) {
> +                        dev_err(hba->dev, "%s: failed to initialize (legacy doorbell mode not supported\n",
> +                                __func__);

A closing parenthesis is missing (")").

> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index bad88bd91995..fd391f6eee73 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1074,6 +1074,7 @@ struct ufs_hba {
>           bool ext_iid_sup;
>           bool scsi_host_added;
>           bool mcq_sup;
> +        bool lsdb_sup;
>           bool mcq_enabled;
>           struct ufshcd_res_info res[RES_MAX];
>           void __iomem *mcq_base;

Please update the kernel-doc comment above struct ufs_hba for the new
lsdb_sup member and please move the new member above mcq_sup such that
the mcq_sup and mcq_enabled definitions remain adjacent.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 1b65e6ae4137..9bea6dc38d64 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2412,7 +2412,17 @@  static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
 		return err;
 	}
 
+	/*
+	 * UFS 3.0 has no MCQ_SUPPORT and LSDB_SUPPORT, but [31:29] as reserved
+	 * bits with reset value 0s, which means we can simply read values
+	 * regardless to version
+	 */
 	hba->mcq_sup = FIELD_GET(MASK_MCQ_SUPPORT, hba->capabilities);
+	/*
+	 * 0h: legacy single doorbell support is available
+	 * 1h: indicate that legacy single doorbell support have been removed
+	 */
+	hba->lsdb_sup = !FIELD_GET(MASK_LSDB_SUPPORT, hba->capabilities);
 	if (!hba->mcq_sup)
 		return 0;
 
@@ -10449,6 +10459,12 @@  int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	}
 
 	if (!is_mcq_supported(hba)) {
+		if (!hba->lsdb_sup) {
+			dev_err(hba->dev, "%s: failed to initialize (legacy doorbell mode not supported\n",
+				__func__);
+			err = -EINVAL;
+			goto out_disable;
+		}
 		err = scsi_add_host(host, hba->dev);
 		if (err) {
 			dev_err(hba->dev, "scsi_add_host failed\n");
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index bad88bd91995..fd391f6eee73 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1074,6 +1074,7 @@  struct ufs_hba {
 	bool ext_iid_sup;
 	bool scsi_host_added;
 	bool mcq_sup;
+	bool lsdb_sup;
 	bool mcq_enabled;
 	struct ufshcd_res_info res[RES_MAX];
 	void __iomem *mcq_base;
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index 385e1c6b8d60..22ba85e81d8c 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -75,6 +75,7 @@  enum {
 	MASK_OUT_OF_ORDER_DATA_DELIVERY_SUPPORT	= 0x02000000,
 	MASK_UIC_DME_TEST_MODE_SUPPORT		= 0x04000000,
 	MASK_CRYPTO_SUPPORT			= 0x10000000,
+	MASK_LSDB_SUPPORT			= 0x20000000,
 	MASK_MCQ_SUPPORT			= 0x40000000,
 };