[PATCH 0/8] UFS patches for kernel 6.11

[PATCH 0/8] UFS patches for kernel 6.11

[Bart Van Assche]

Hi Martin,

Please consider this series of UFS driver patches for the next merge window.

Thanks,

Bart.

Bart Van Assche (8):
  scsi: ufs: Initialize struct uic_command once
  scsi: ufs: Remove two constants
  scsi: ufs: Inline ufshcd_mcq_vops_get_hba_mac()
  scsi: ufs: Make .get_hba_mac() optional
  scsi: ufs: Declare ufshcd_mcq_poll_cqe_lock() once
  scsi: ufs: Make ufshcd_poll() complain about unsupported arguments
  scsi: ufs: Make the polling code report which command has been
    completed
  scsi: ufs: Check for completion from the timeout handler

 drivers/ufs/core/ufs-mcq.c      |  45 +++++++----
 drivers/ufs/core/ufshcd-priv.h  |  14 +---
 drivers/ufs/core/ufshcd.c       | 131 ++++++++++++++++++++------------
 drivers/ufs/host/ufs-mediatek.c |   2 +-
 drivers/ufs/host/ufs-qcom.c     |   2 +-
 include/ufs/ufshcd.h            |  11 ++-
 include/ufs/ufshci.h            |   2 +-
 7 files changed, 126 insertions(+), 81 deletions(-)

[PATCH 1/8] scsi: ufs: Initialize struct uic_command once

[Bart Van Assche]

Instead of first zero-initializing struct uic_command and next initializing
it memberwise, initialize all members at once.

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 41bf2e249c83..5d784876513e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3993,11 +3993,11 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
  */
 static int ufshcd_dme_link_startup(struct ufs_hba *hba)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_LINK_STARTUP,
+	};
 	int ret;
 
-	uic_cmd.command = UIC_CMD_DME_LINK_STARTUP;
-
 	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
 	if (ret)
 		dev_dbg(hba->dev,
@@ -4015,11 +4015,11 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  */
 static int ufshcd_dme_reset(struct ufs_hba *hba)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_RESET,
+	};
 	int ret;
 
-	uic_cmd.command = UIC_CMD_DME_RESET;
-
 	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
 	if (ret)
 		dev_err(hba->dev,
@@ -4054,11 +4054,11 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_configure_adapt);
  */
 static int ufshcd_dme_enable(struct ufs_hba *hba)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_ENABLE,
+	};
 	int ret;
 
-	uic_cmd.command = UIC_CMD_DME_ENABLE;
-
 	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
 	if (ret)
 		dev_err(hba->dev,
@@ -4111,7 +4111,12 @@ static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
 int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
 			u8 attr_set, u32 mib_val, u8 peer)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = peer ? UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET,
+		.argument1 = attr_sel,
+		.argument2 = UIC_ARG_ATTR_TYPE(attr_set),
+		.argument3 = mib_val,
+	};
 	static const char *const action[] = {
 		"dme-set",
 		"dme-peer-set"
@@ -4120,12 +4125,6 @@ int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
 	int ret;
 	int retries = UFS_UIC_COMMAND_RETRIES;
 
-	uic_cmd.command = peer ?
-		UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET;
-	uic_cmd.argument1 = attr_sel;
-	uic_cmd.argument2 = UIC_ARG_ATTR_TYPE(attr_set);
-	uic_cmd.argument3 = mib_val;
-
 	do {
 		/* for peer attributes we retry upon failure */
 		ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
@@ -4155,7 +4154,11 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_set_attr);
 int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
 			u32 *mib_val, u8 peer)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = peer ? UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET,
+		.argument1 = attr_sel,
+
+	};
 	static const char *const action[] = {
 		"dme-get",
 		"dme-peer-get"
@@ -4189,10 +4192,6 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
 		}
 	}
 
-	uic_cmd.command = peer ?
-		UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET;
-	uic_cmd.argument1 = attr_sel;
-
 	do {
 		/* for peer attributes we retry upon failure */
 		ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
@@ -4325,7 +4324,11 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
  */
 int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_SET,
+		.argument1 = UIC_ARG_MIB(PA_PWRMODE),
+		.argument3 = mode,
+	};
 	int ret;
 
 	if (hba->quirks & UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP) {
@@ -4338,9 +4341,6 @@ int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
 		}
 	}
 
-	uic_cmd.command = UIC_CMD_DME_SET;
-	uic_cmd.argument1 = UIC_ARG_MIB(PA_PWRMODE);
-	uic_cmd.argument3 = mode;
 	ufshcd_hold(hba);
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	ufshcd_release(hba);
@@ -4381,13 +4381,14 @@ EXPORT_SYMBOL_GPL(ufshcd_link_recovery);
 
 int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
 {
-	int ret;
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_HIBER_ENTER,
+	};
 	ktime_t start = ktime_get();
+	int ret;
 
 	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER, PRE_CHANGE);
 
-	uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	trace_ufshcd_profile_hibern8(dev_name(hba->dev), "enter",
 			     ktime_to_us(ktime_sub(ktime_get(), start)), ret);
@@ -4405,13 +4406,14 @@ EXPORT_SYMBOL_GPL(ufshcd_uic_hibern8_enter);
 
 int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
 {
-	struct uic_command uic_cmd = {0};
+	struct uic_command uic_cmd = {
+		.command = UIC_CMD_DME_HIBER_EXIT,
+	};
 	int ret;
 	ktime_t start = ktime_get();
 
 	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_EXIT, PRE_CHANGE);
 
-	uic_cmd.command = UIC_CMD_DME_HIBER_EXIT;
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	trace_ufshcd_profile_hibern8(dev_name(hba->dev), "exit",
 			     ktime_to_us(ktime_sub(ktime_get(), start)), ret);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 9e0581115b34..d4d63507d090 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -73,8 +73,8 @@ enum ufs_event_type {
  * @done: UIC command completion
  */
 struct uic_command {
-	u32 command;
-	u32 argument1;
+	const u32 command;
+	const u32 argument1;
 	u32 argument2;
 	u32 argument3;
 	int cmd_active;

RE: [PATCH 1/8] scsi: ufs: Initialize struct uic_command once

[Daejun Park]

Hi Bart,
 
> Instead of first zero-initializing struct uic_command and next initializing
> it memberwise, initialize all members at once.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/ufs/core/ufshcd.c 62 ++++++++++++++++++++-------------------
> include/ufs/ufshcd.h        4 +--
> 2 files changed, 34 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 41bf2e249c83..5d784876513e 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -3993,11 +3993,11 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
>   */
> static int ufshcd_dme_link_startup(struct ufs_hba *hba)
> {
> -        struct uic_command uic_cmd = {0};
> +        struct uic_command uic_cmd = {
> +                .command = UIC_CMD_DME_LINK_STARTUP,
> +        };
>          int ret;
> 
> -        uic_cmd.command = UIC_CMD_DME_LINK_STARTUP;
> -
>          ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
>          if (ret)
>                  dev_dbg(hba->dev,
> @@ -4015,11 +4015,11 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
>   */
> static int ufshcd_dme_reset(struct ufs_hba *hba)
> {
> -        struct uic_command uic_cmd = {0};
> +        struct uic_command uic_cmd = {
> +                .command = UIC_CMD_DME_RESET,
> +        };
>          int ret;
> 
> -        uic_cmd.command = UIC_CMD_DME_RESET;
> -
>          ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
>          if (ret)
>                  dev_err(hba->dev,
> @@ -4054,11 +4054,11 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_configure_adapt);
>   */
> static int ufshcd_dme_enable(struct ufs_hba *hba)
> {
> -        struct uic_command uic_cmd = {0};
> +        struct uic_command uic_cmd = {
> +                .command = UIC_CMD_DME_ENABLE,
> +        };
>          int ret;
> 
> -        uic_cmd.command = UIC_CMD_DME_ENABLE;
> -
>          ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
>          if (ret)
>                  dev_err(hba->dev,
> @@ -4111,7 +4111,12 @@ static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
> int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
>                          u8 attr_set, u32 mib_val, u8 peer)
> {
> -        struct uic_command uic_cmd = {0};
> +        struct uic_command uic_cmd = {
> +                .command = peer ? UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET,
> +                .argument1 = attr_sel,
> +                .argument2 = UIC_ARG_ATTR_TYPE(attr_set),
> +                .argument3 = mib_val,
> +        };
>          static const char *const action[] = {
>                  "dme-set",
>                  "dme-peer-set"
> @@ -4120,12 +4125,6 @@ int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
>          int ret;
>          int retries = UFS_UIC_COMMAND_RETRIES;
> 
> -        uic_cmd.command = peer ?
> -                UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET;
> -        uic_cmd.argument1 = attr_sel;
> -        uic_cmd.argument2 = UIC_ARG_ATTR_TYPE(attr_set);
> -        uic_cmd.argument3 = mib_val;
> -
>          do {
>                  /* for peer attributes we retry upon failure */
>                  ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> @@ -4155,7 +4154,11 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_set_attr);
> int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
>                          u32 *mib_val, u8 peer)
> {
> -        struct uic_command uic_cmd = {0};
> +        struct uic_command uic_cmd = {
> +                .command = peer ? UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET,
> +                .argument1 = attr_sel,
> +
Empty line.

> +        };
>          static const char *const action[] = {
>                  "dme-get",
>                  "dme-peer-get"
> @@ -4189,10 +4192,6 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
>                  }
>          }
> 
> -        uic_cmd.command = peer ?
> -                UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET;
> -        uic_cmd.argument1 = attr_sel;
> -
>          do {
>                  /* for peer attributes we retry upon failure */
>                  ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> @@ -4325,7 +4324,11 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>   */
> int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
> {
> -        struct uic_command uic_cmd = {0};
> +        struct uic_command uic_cmd = {
> +                .command = UIC_CMD_DME_SET,
> +                .argument1 = UIC_ARG_MIB(PA_PWRMODE),
> +                .argument3 = mode,
> +        };
>          int ret;
> 
>          if (hba->quirks & UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP) {
> @@ -4338,9 +4341,6 @@ int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
>                  }
>          }
> 
> -        uic_cmd.command = UIC_CMD_DME_SET;
> -        uic_cmd.argument1 = UIC_ARG_MIB(PA_PWRMODE);
> -        uic_cmd.argument3 = mode;
>          ufshcd_hold(hba);
>          ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>          ufshcd_release(hba);
> @@ -4381,13 +4381,14 @@ EXPORT_SYMBOL_GPL(ufshcd_link_recovery);
> 
> int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
> {
> -        int ret;
> -        struct uic_command uic_cmd = {0};
> +        struct uic_command uic_cmd = {
> +                .command = UIC_CMD_DME_HIBER_ENTER,
> +        };
>          ktime_t start = ktime_get();
> +        int ret;
> 
>          ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER, PRE_CHANGE);
> 
> -        uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
>          ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>          trace_ufshcd_profile_hibern8(dev_name(hba->dev), "enter",
>                              ktime_to_us(ktime_sub(ktime_get(), start)), ret);
> @@ -4405,13 +4406,14 @@ EXPORT_SYMBOL_GPL(ufshcd_uic_hibern8_enter);
> 
> int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
> {
> -        struct uic_command uic_cmd = {0};
> +        struct uic_command uic_cmd = {
> +                .command = UIC_CMD_DME_HIBER_EXIT,
> +        };
>          int ret;
>          ktime_t start = ktime_get();
> 
>          ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_EXIT, PRE_CHANGE);
> 
> -        uic_cmd.command = UIC_CMD_DME_HIBER_EXIT;
>          ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>          trace_ufshcd_profile_hibern8(dev_name(hba->dev), "exit",
>                              ktime_to_us(ktime_sub(ktime_get(), start)), ret);
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 9e0581115b34..d4d63507d090 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -73,8 +73,8 @@ enum ufs_event_type {
>   * @done: UIC command completion
>   */
> struct uic_command {
> -        u32 command;
> -        u32 argument1;
> +        const u32 command;
> +        const u32 argument1;
>          u32 argument2;
>          u32 argument3;
>          int cmd_active;

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,

Daejun

Re: [PATCH 1/8] scsi: ufs: Initialize struct uic_command once

[Bart Van Assche]

On 6/17/24 6:25 PM, Daejun Park wrote:
>> @@ -4155,7 +4154,11 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_set_attr);
>> int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
>>                           u32 *mib_val, u8 peer)
>> {
>> -        struct uic_command uic_cmd = {0};
>> +        struct uic_command uic_cmd = {
>> +                .command = peer ? UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET,
>> +                .argument1 = attr_sel,
>> +
> Empty line.

Thanks for the feedback. I will remove this empty line before I repost this patch
series.

Bart.

RE: [PATCH 1/8] scsi: ufs: Initialize struct uic_command once

[Avri Altman]

> Instead of first zero-initializing struct uic_command and next initializing
> it memberwise, initialize all members at once.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Re: [PATCH 1/8] scsi: ufs: Initialize struct uic_command once

[Manivannan Sadhasivam]

On Mon, Jun 17, 2024 at 02:07:40PM -0700, Bart Van Assche wrote:
> Instead of first zero-initializing struct uic_command and next initializing
> it memberwise, initialize all members at once.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

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

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 62 ++++++++++++++++++++-------------------
>  include/ufs/ufshcd.h      |  4 +--
>  2 files changed, 34 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 41bf2e249c83..5d784876513e 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -3993,11 +3993,11 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
>   */
>  static int ufshcd_dme_link_startup(struct ufs_hba *hba)
>  {
> -	struct uic_command uic_cmd = {0};
> +	struct uic_command uic_cmd = {
> +		.command = UIC_CMD_DME_LINK_STARTUP,
> +	};
>  	int ret;
>  
> -	uic_cmd.command = UIC_CMD_DME_LINK_STARTUP;
> -
>  	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
>  	if (ret)
>  		dev_dbg(hba->dev,
> @@ -4015,11 +4015,11 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
>   */
>  static int ufshcd_dme_reset(struct ufs_hba *hba)
>  {
> -	struct uic_command uic_cmd = {0};
> +	struct uic_command uic_cmd = {
> +		.command = UIC_CMD_DME_RESET,
> +	};
>  	int ret;
>  
> -	uic_cmd.command = UIC_CMD_DME_RESET;
> -
>  	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
>  	if (ret)
>  		dev_err(hba->dev,
> @@ -4054,11 +4054,11 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_configure_adapt);
>   */
>  static int ufshcd_dme_enable(struct ufs_hba *hba)
>  {
> -	struct uic_command uic_cmd = {0};
> +	struct uic_command uic_cmd = {
> +		.command = UIC_CMD_DME_ENABLE,
> +	};
>  	int ret;
>  
> -	uic_cmd.command = UIC_CMD_DME_ENABLE;
> -
>  	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
>  	if (ret)
>  		dev_err(hba->dev,
> @@ -4111,7 +4111,12 @@ static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
>  int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
>  			u8 attr_set, u32 mib_val, u8 peer)
>  {
> -	struct uic_command uic_cmd = {0};
> +	struct uic_command uic_cmd = {
> +		.command = peer ? UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET,
> +		.argument1 = attr_sel,
> +		.argument2 = UIC_ARG_ATTR_TYPE(attr_set),
> +		.argument3 = mib_val,
> +	};
>  	static const char *const action[] = {
>  		"dme-set",
>  		"dme-peer-set"
> @@ -4120,12 +4125,6 @@ int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
>  	int ret;
>  	int retries = UFS_UIC_COMMAND_RETRIES;
>  
> -	uic_cmd.command = peer ?
> -		UIC_CMD_DME_PEER_SET : UIC_CMD_DME_SET;
> -	uic_cmd.argument1 = attr_sel;
> -	uic_cmd.argument2 = UIC_ARG_ATTR_TYPE(attr_set);
> -	uic_cmd.argument3 = mib_val;
> -
>  	do {
>  		/* for peer attributes we retry upon failure */
>  		ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> @@ -4155,7 +4154,11 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_set_attr);
>  int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
>  			u32 *mib_val, u8 peer)
>  {
> -	struct uic_command uic_cmd = {0};
> +	struct uic_command uic_cmd = {
> +		.command = peer ? UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET,
> +		.argument1 = attr_sel,
> +
> +	};
>  	static const char *const action[] = {
>  		"dme-get",
>  		"dme-peer-get"
> @@ -4189,10 +4192,6 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
>  		}
>  	}
>  
> -	uic_cmd.command = peer ?
> -		UIC_CMD_DME_PEER_GET : UIC_CMD_DME_GET;
> -	uic_cmd.argument1 = attr_sel;
> -
>  	do {
>  		/* for peer attributes we retry upon failure */
>  		ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> @@ -4325,7 +4324,11 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>   */
>  int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
>  {
> -	struct uic_command uic_cmd = {0};
> +	struct uic_command uic_cmd = {
> +		.command = UIC_CMD_DME_SET,
> +		.argument1 = UIC_ARG_MIB(PA_PWRMODE),
> +		.argument3 = mode,
> +	};
>  	int ret;
>  
>  	if (hba->quirks & UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP) {
> @@ -4338,9 +4341,6 @@ int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
>  		}
>  	}
>  
> -	uic_cmd.command = UIC_CMD_DME_SET;
> -	uic_cmd.argument1 = UIC_ARG_MIB(PA_PWRMODE);
> -	uic_cmd.argument3 = mode;
>  	ufshcd_hold(hba);
>  	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>  	ufshcd_release(hba);
> @@ -4381,13 +4381,14 @@ EXPORT_SYMBOL_GPL(ufshcd_link_recovery);
>  
>  int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
>  {
> -	int ret;
> -	struct uic_command uic_cmd = {0};
> +	struct uic_command uic_cmd = {
> +		.command = UIC_CMD_DME_HIBER_ENTER,
> +	};
>  	ktime_t start = ktime_get();
> +	int ret;
>  
>  	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER, PRE_CHANGE);
>  
> -	uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
>  	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>  	trace_ufshcd_profile_hibern8(dev_name(hba->dev), "enter",
>  			     ktime_to_us(ktime_sub(ktime_get(), start)), ret);
> @@ -4405,13 +4406,14 @@ EXPORT_SYMBOL_GPL(ufshcd_uic_hibern8_enter);
>  
>  int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
>  {
> -	struct uic_command uic_cmd = {0};
> +	struct uic_command uic_cmd = {
> +		.command = UIC_CMD_DME_HIBER_EXIT,
> +	};
>  	int ret;
>  	ktime_t start = ktime_get();
>  
>  	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_EXIT, PRE_CHANGE);
>  
> -	uic_cmd.command = UIC_CMD_DME_HIBER_EXIT;
>  	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>  	trace_ufshcd_profile_hibern8(dev_name(hba->dev), "exit",
>  			     ktime_to_us(ktime_sub(ktime_get(), start)), ret);
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 9e0581115b34..d4d63507d090 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -73,8 +73,8 @@ enum ufs_event_type {
>   * @done: UIC command completion
>   */
>  struct uic_command {
> -	u32 command;
> -	u32 argument1;
> +	const u32 command;
> +	const u32 argument1;
>  	u32 argument2;
>  	u32 argument3;
>  	int cmd_active;

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

[PATCH 2/8] scsi: ufs: Remove two constants

[Bart Van Assche]

The SCSI host template members .cmd_per_lun and .can_queue are copied
into the SCSI host data structure. Before these are used, these are
overwritten by ufshcd_init(). Hence, this patch does not change any
functionality.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 5d784876513e..7761ccca2115 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -164,8 +164,6 @@ EXPORT_SYMBOL_GPL(ufshcd_dump_regs);
 enum {
 	UFSHCD_MAX_CHANNEL	= 0,
 	UFSHCD_MAX_ID		= 1,
-	UFSHCD_CMD_PER_LUN	= 32 - UFSHCD_NUM_RESERVED,
-	UFSHCD_CAN_QUEUE	= 32 - UFSHCD_NUM_RESERVED,
 };
 
 static const char *const ufshcd_state_name[] = {
@@ -8959,8 +8957,6 @@ static const struct scsi_host_template ufshcd_driver_template = {
 	.eh_timed_out		= ufshcd_eh_timed_out,
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
-	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
-	.can_queue		= UFSHCD_CAN_QUEUE,
 	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
 	.max_sectors		= SZ_1M / SECTOR_SIZE,
 	.max_host_blocked	= 1,

Re: [PATCH 2/8] scsi: ufs: Remove two constants

[Manivannan Sadhasivam]

On Mon, Jun 17, 2024 at 02:07:41PM -0700, Bart Van Assche wrote:
> The SCSI host template members .cmd_per_lun and .can_queue are copied
> into the SCSI host data structure. Before these are used, these are
> overwritten by ufshcd_init(). Hence, this patch does not change any
> functionality.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

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

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 5d784876513e..7761ccca2115 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -164,8 +164,6 @@ EXPORT_SYMBOL_GPL(ufshcd_dump_regs);
>  enum {
>  	UFSHCD_MAX_CHANNEL	= 0,
>  	UFSHCD_MAX_ID		= 1,
> -	UFSHCD_CMD_PER_LUN	= 32 - UFSHCD_NUM_RESERVED,
> -	UFSHCD_CAN_QUEUE	= 32 - UFSHCD_NUM_RESERVED,
>  };
>  
>  static const char *const ufshcd_state_name[] = {
> @@ -8959,8 +8957,6 @@ static const struct scsi_host_template ufshcd_driver_template = {
>  	.eh_timed_out		= ufshcd_eh_timed_out,
>  	.this_id		= -1,
>  	.sg_tablesize		= SG_ALL,
> -	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
> -	.can_queue		= UFSHCD_CAN_QUEUE,
>  	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
>  	.max_sectors		= SZ_1M / SECTOR_SIZE,
>  	.max_host_blocked	= 1,

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

[PATCH 3/8] scsi: ufs: Inline ufshcd_mcq_vops_get_hba_mac()

[Bart Van Assche]

Make ufshcd_mcq_decide_queue_depth() easier to read by inlining
ufshcd_mcq_vops_get_hba_mac().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufs-mcq.c     | 18 +++++++++++-------
 drivers/ufs/core/ufshcd-priv.h |  8 --------
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 4bcae410c268..0482c7a1e419 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -144,14 +144,14 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
  */
 int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
 {
-	int mac;
+	int mac = -EOPNOTSUPP;
 
-	/* Mandatory to implement get_hba_mac() */
-	mac = ufshcd_mcq_vops_get_hba_mac(hba);
-	if (mac < 0) {
-		dev_err(hba->dev, "Failed to get mac, err=%d\n", mac);
-		return mac;
-	}
+	if (!hba->vops || !hba->vops->get_hba_mac)
+		goto err;
+
+	mac = hba->vops->get_hba_mac(hba);
+	if (mac < 0)
+		goto err;
 
 	WARN_ON_ONCE(!hba->dev_info.bqueuedepth);
 	/*
@@ -160,6 +160,10 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
 	 * shared queuing architecture is enabled.
 	 */
 	return min_t(int, mac, hba->dev_info.bqueuedepth);
+
+err:
+	dev_err(hba->dev, "Failed to get mac, err=%d\n", mac);
+	return mac;
 }
 
 static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba)
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index f42d99ce5bf1..a1add22205db 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -255,14 +255,6 @@ static inline int ufshcd_vops_mcq_config_resource(struct ufs_hba *hba)
 	return -EOPNOTSUPP;
 }
 
-static inline int ufshcd_mcq_vops_get_hba_mac(struct ufs_hba *hba)
-{
-	if (hba->vops && hba->vops->get_hba_mac)
-		return hba->vops->get_hba_mac(hba);
-
-	return -EOPNOTSUPP;
-}
-
 static inline int ufshcd_mcq_vops_op_runtime_config(struct ufs_hba *hba)
 {
 	if (hba->vops && hba->vops->op_runtime_config)

RE: [PATCH 3/8] scsi: ufs: Inline ufshcd_mcq_vops_get_hba_mac()

[Avri Altman]

> Make ufshcd_mcq_decide_queue_depth() easier to read by inlining
> ufshcd_mcq_vops_get_hba_mac().
This changes its functionality by making it non-mandatory.
Maybe relate to that in the commit log.
Also, it would make sense to squash it to the next patch, so your line of reasoning is clear.

Thanks,
Avri

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufs-mcq.c     | 18 +++++++++++-------
>  drivers/ufs/core/ufshcd-priv.h |  8 --------
>  2 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index
> 4bcae410c268..0482c7a1e419 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -144,14 +144,14 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
>   */
>  int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)  {
> -       int mac;
> +       int mac = -EOPNOTSUPP;
> 
> -       /* Mandatory to implement get_hba_mac() */
> -       mac = ufshcd_mcq_vops_get_hba_mac(hba);
> -       if (mac < 0) {
> -               dev_err(hba->dev, "Failed to get mac, err=%d\n", mac);
> -               return mac;
> -       }
> +       if (!hba->vops || !hba->vops->get_hba_mac)
> +               goto err;
> +
> +       mac = hba->vops->get_hba_mac(hba);
> +       if (mac < 0)
> +               goto err;
> 
>         WARN_ON_ONCE(!hba->dev_info.bqueuedepth);
>         /*
> @@ -160,6 +160,10 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba
> *hba)
>          * shared queuing architecture is enabled.
>          */
>         return min_t(int, mac, hba->dev_info.bqueuedepth);
> +
> +err:
> +       dev_err(hba->dev, "Failed to get mac, err=%d\n", mac);
> +       return mac;
>  }
> 
>  static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) diff --git
> a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index
> f42d99ce5bf1..a1add22205db 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -255,14 +255,6 @@ static inline int
> ufshcd_vops_mcq_config_resource(struct ufs_hba *hba)
>         return -EOPNOTSUPP;
>  }
> 
> -static inline int ufshcd_mcq_vops_get_hba_mac(struct ufs_hba *hba) -{
> -       if (hba->vops && hba->vops->get_hba_mac)
> -               return hba->vops->get_hba_mac(hba);
> -
> -       return -EOPNOTSUPP;
> -}
> -
>  static inline int ufshcd_mcq_vops_op_runtime_config(struct ufs_hba *hba)  {
>         if (hba->vops && hba->vops->op_runtime_config)

Re: [PATCH 3/8] scsi: ufs: Inline ufshcd_mcq_vops_get_hba_mac()

[Bart Van Assche]

On 6/17/24 11:23 PM, Avri Altman wrote:
> 
>> Make ufshcd_mcq_decide_queue_depth() easier to read by inlining
>> ufshcd_mcq_vops_get_hba_mac().
 >
> This changes its functionality by making it non-mandatory.
> Maybe relate to that in the commit log.

I do not agree. Even with this patch applied, .get_hba_mac() is still
mandatory.

> Also, it would make sense to squash it to the next patch, so your line of reasoning is clear.

I prefer to keep the inlining (no functional change) separate from the
patch that introduces a behavior change.

Thanks,

Bart.

[PATCH 4/8] scsi: ufs: Make .get_hba_mac() optional

[Bart Van Assche]

UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report
the maximum number of supported commands in the controller capabilities
register. Use that value if .get_hba_mac == NULL.

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

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 0482c7a1e419..d6f966f4abef 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -138,7 +138,6 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
  *
  * MAC - Max. Active Command of the Host Controller (HC)
  * HC wouldn't send more than this commands to the device.
- * It is mandatory to implement get_hba_mac() to enable MCQ mode.
  * Calculates and adjusts the queue depth based on the depth
  * supported by the HC and ufs device.
  */
@@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
 {
 	int mac = -EOPNOTSUPP;
 
-	if (!hba->vops || !hba->vops->get_hba_mac)
-		goto err;
-
-	mac = hba->vops->get_hba_mac(hba);
+	if (!hba->vops || !hba->vops->get_hba_mac) {
+		hba->capabilities =
+			ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
+		mac = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1;
+	} else {
+		mac = hba->vops->get_hba_mac(hba);
+	}
 	if (mac < 0)
 		goto err;
 
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d4d63507d090..d32637d267f3 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -325,7 +325,9 @@ struct ufs_pwr_mode_info {
  * @event_notify: called to notify important events
  * @reinit_notify: called to notify reinit of UFSHCD during max gear switch
  * @mcq_config_resource: called to configure MCQ platform resources
- * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode
+ * @get_hba_mac: reports maximum number of outstanding commands supported by
+ *	the controller. Should be implemented for UFSHCI 4.0 or later
+ *	controllers that are not compliant with the UFSHCI 4.0 specification.
  * @op_runtime_config: called to config Operation and runtime regs Pointers
  * @get_outstanding_cqs: called to get outstanding completion queues
  * @config_esi: called to config Event Specific Interrupt
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index c50f92bf2e1d..899077bba2d2 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -67,7 +67,7 @@ enum {
 
 /* Controller capability masks */
 enum {
-	MASK_TRANSFER_REQUESTS_SLOTS		= 0x0000001F,
+	MASK_TRANSFER_REQUESTS_SLOTS		= 0x000000FF,
 	MASK_NUMBER_OUTSTANDING_RTT		= 0x0000FF00,
 	MASK_TASK_MANAGEMENT_REQUEST_SLOTS	= 0x00070000,
 	MASK_EHSLUTRD_SUPPORTED			= 0x00400000,

RE: [PATCH 4/8] scsi: ufs: Make .get_hba_mac() optional

[Daejun Park]

Hi Bart,

> UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report
> the maximum number of supported commands in the controller capabilities
> register. Use that value if .get_hba_mac == NULL.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/ufs/core/ufs-mcq.c 12 +++++++-----
> include/ufs/ufshcd.h        4 +++-
> include/ufs/ufshci.h        2 +-
> 3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 0482c7a1e419..d6f966f4abef 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -138,7 +138,6 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
>   *
>   * MAC - Max. Active Command of the Host Controller (HC)
>   * HC wouldn't send more than this commands to the device.
> - * It is mandatory to implement get_hba_mac() to enable MCQ mode.
>   * Calculates and adjusts the queue depth based on the depth
>   * supported by the HC and ufs device.
>   */
> @@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
> {
>          int mac = -EOPNOTSUPP;
This initialization can be removed.

> 
> -        if (!hba->vops !hba->vops->get_hba_mac)
> -                goto err;
> -
> -        mac = hba->vops->get_hba_mac(hba);
> +        if (!hba->vops !hba->vops->get_hba_mac) {
> +                hba->capabilities =
> +                        ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
> +                mac = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1;
> +        } else {
> +                mac = hba->vops->get_hba_mac(hba);
> +        }
>          if (mac < 0)
>                  goto err;
> 
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index d4d63507d090..d32637d267f3 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -325,7 +325,9 @@ struct ufs_pwr_mode_info {
>   * @event_notify: called to notify important events
>   * @reinit_notify: called to notify reinit of UFSHCD during max gear switch
>   * @mcq_config_resource: called to configure MCQ platform resources
> - * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode
> + * @get_hba_mac: reports maximum number of outstanding commands supported by
> + *        the controller. Should be implemented for UFSHCI 4.0 or later
> + *        controllers that are not compliant with the UFSHCI 4.0 specification.
>   * @op_runtime_config: called to config Operation and runtime regs Pointers
>   * @get_outstanding_cqs: called to get outstanding completion queues
>   * @config_esi: called to config Event Specific Interrupt
> diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
> index c50f92bf2e1d..899077bba2d2 100644
> --- a/include/ufs/ufshci.h
> +++ b/include/ufs/ufshci.h
> @@ -67,7 +67,7 @@ enum {
> 
> /* Controller capability masks */
> enum {
> -        MASK_TRANSFER_REQUESTS_SLOTS                = 0x0000001F,
> +        MASK_TRANSFER_REQUESTS_SLOTS                = 0x000000FF,
>          MASK_NUMBER_OUTSTANDING_RTT                = 0x0000FF00,
>          MASK_TASK_MANAGEMENT_REQUEST_SLOTS        = 0x00070000,
>          MASK_EHSLUTRD_SUPPORTED                        = 0x00400000,

Reviewed-by: Daejun Park <daejun7.park@samsung.com>

Thanks,
Daejun

Re: [PATCH 4/8] scsi: ufs: Make .get_hba_mac() optional

[Bart Van Assche]

On 6/17/24 6:28 PM, Daejun Park wrote:
>> @@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
>> {
>>           int mac = -EOPNOTSUPP;
> This initialization can be removed.

I will remove the initialization.

Thanks,

Bart.

Re: [PATCH 4/8] scsi: ufs: Make .get_hba_mac() optional

[Manivannan Sadhasivam]

On Mon, Jun 17, 2024 at 02:07:43PM -0700, Bart Van Assche wrote:
> UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report
> the maximum number of supported commands in the controller capabilities
> register. Use that value if .get_hba_mac == NULL.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufs-mcq.c | 12 +++++++-----
>  include/ufs/ufshcd.h       |  4 +++-
>  include/ufs/ufshci.h       |  2 +-
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 0482c7a1e419..d6f966f4abef 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -138,7 +138,6 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
>   *
>   * MAC - Max. Active Command of the Host Controller (HC)
>   * HC wouldn't send more than this commands to the device.
> - * It is mandatory to implement get_hba_mac() to enable MCQ mode.
>   * Calculates and adjusts the queue depth based on the depth
>   * supported by the HC and ufs device.
>   */
> @@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
>  {
>  	int mac = -EOPNOTSUPP;
>  
> -	if (!hba->vops || !hba->vops->get_hba_mac)
> -		goto err;
> -
> -	mac = hba->vops->get_hba_mac(hba);
> +	if (!hba->vops || !hba->vops->get_hba_mac) {
> +		hba->capabilities =
> +			ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
> +		mac = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1;
> +	} else {
> +		mac = hba->vops->get_hba_mac(hba);
> +	}
>  	if (mac < 0)
>  		goto err;
>  
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index d4d63507d090..d32637d267f3 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -325,7 +325,9 @@ struct ufs_pwr_mode_info {
>   * @event_notify: called to notify important events
>   * @reinit_notify: called to notify reinit of UFSHCD during max gear switch
>   * @mcq_config_resource: called to configure MCQ platform resources
> - * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode
> + * @get_hba_mac: reports maximum number of outstanding commands supported by
> + *	the controller. Should be implemented for UFSHCI 4.0 or later
> + *	controllers that are not compliant with the UFSHCI 4.0 specification.
>   * @op_runtime_config: called to config Operation and runtime regs Pointers
>   * @get_outstanding_cqs: called to get outstanding completion queues
>   * @config_esi: called to config Event Specific Interrupt
> diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
> index c50f92bf2e1d..899077bba2d2 100644
> --- a/include/ufs/ufshci.h
> +++ b/include/ufs/ufshci.h
> @@ -67,7 +67,7 @@ enum {
>  
>  /* Controller capability masks */
>  enum {
> -	MASK_TRANSFER_REQUESTS_SLOTS		= 0x0000001F,
> +	MASK_TRANSFER_REQUESTS_SLOTS		= 0x000000FF,

This should be a separate fix that comes before this patch. But I believe this
came up during MCQ review as well and I don't remember what was the reply from
Can. 0x1F is the mask for SDB mode and 0xFF is the mask for MCQ mode.

Can, can you comment more?

- Mani

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

Re: [PATCH 4/8] scsi: ufs: Make .get_hba_mac() optional

[Manivannan Sadhasivam]

+ Can

On Wed, Jun 19, 2024 at 12:43:29PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jun 17, 2024 at 02:07:43PM -0700, Bart Van Assche wrote:
> > UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report
> > the maximum number of supported commands in the controller capabilities
> > register. Use that value if .get_hba_mac == NULL.
> > 
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> >  drivers/ufs/core/ufs-mcq.c | 12 +++++++-----
> >  include/ufs/ufshcd.h       |  4 +++-
> >  include/ufs/ufshci.h       |  2 +-
> >  3 files changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> > index 0482c7a1e419..d6f966f4abef 100644
> > --- a/drivers/ufs/core/ufs-mcq.c
> > +++ b/drivers/ufs/core/ufs-mcq.c
> > @@ -138,7 +138,6 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr);
> >   *
> >   * MAC - Max. Active Command of the Host Controller (HC)
> >   * HC wouldn't send more than this commands to the device.
> > - * It is mandatory to implement get_hba_mac() to enable MCQ mode.
> >   * Calculates and adjusts the queue depth based on the depth
> >   * supported by the HC and ufs device.
> >   */
> > @@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba)
> >  {
> >  	int mac = -EOPNOTSUPP;
> >  
> > -	if (!hba->vops || !hba->vops->get_hba_mac)
> > -		goto err;
> > -
> > -	mac = hba->vops->get_hba_mac(hba);
> > +	if (!hba->vops || !hba->vops->get_hba_mac) {
> > +		hba->capabilities =
> > +			ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
> > +		mac = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1;
> > +	} else {
> > +		mac = hba->vops->get_hba_mac(hba);
> > +	}
> >  	if (mac < 0)
> >  		goto err;
> >  
> > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> > index d4d63507d090..d32637d267f3 100644
> > --- a/include/ufs/ufshcd.h
> > +++ b/include/ufs/ufshcd.h
> > @@ -325,7 +325,9 @@ struct ufs_pwr_mode_info {
> >   * @event_notify: called to notify important events
> >   * @reinit_notify: called to notify reinit of UFSHCD during max gear switch
> >   * @mcq_config_resource: called to configure MCQ platform resources
> > - * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode
> > + * @get_hba_mac: reports maximum number of outstanding commands supported by
> > + *	the controller. Should be implemented for UFSHCI 4.0 or later
> > + *	controllers that are not compliant with the UFSHCI 4.0 specification.
> >   * @op_runtime_config: called to config Operation and runtime regs Pointers
> >   * @get_outstanding_cqs: called to get outstanding completion queues
> >   * @config_esi: called to config Event Specific Interrupt
> > diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
> > index c50f92bf2e1d..899077bba2d2 100644
> > --- a/include/ufs/ufshci.h
> > +++ b/include/ufs/ufshci.h
> > @@ -67,7 +67,7 @@ enum {
> >  
> >  /* Controller capability masks */
> >  enum {
> > -	MASK_TRANSFER_REQUESTS_SLOTS		= 0x0000001F,
> > +	MASK_TRANSFER_REQUESTS_SLOTS		= 0x000000FF,
> 
> This should be a separate fix that comes before this patch. But I believe this
> came up during MCQ review as well and I don't remember what was the reply from
> Can. 0x1F is the mask for SDB mode and 0xFF is the mask for MCQ mode.
> 
> Can, can you comment more?
> 

Oops. Can is not CCed. Added now.

- Mani


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

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

Re: [PATCH 4/8] scsi: ufs: Make .get_hba_mac() optional

[Peter Wang (王信友)]

On Wed, 2024-06-19 at 13:27 +0530, Manivannan Sadhasivam wrote:
> > > --- a/include/ufs/ufshci.h
> > > +++ b/include/ufs/ufshci.h
> > > @@ -67,7 +67,7 @@ enum {
> > >  
> > >  /* Controller capability masks */
> > >  enum {
> > > -MASK_TRANSFER_REQUESTS_SLOTS= 0x0000001F,
> > > +MASK_TRANSFER_REQUESTS_SLOTS= 0x000000FF,
> > 
> > This should be a separate fix that comes before this patch. But I
> believe this
> > came up during MCQ review as well and I don't remember what was the
> reply from
> > Can. 0x1F is the mask for SDB mode and 0xFF is the mask for MCQ
> mode.
> > 
> > Can, can you comment more?
> > 
> 

Hi Bart and Mani,

It is correct that 0x1F is SDB mode.
ufshcd_mcq_decide_queue_depth is running before mcq enable.
So that value read is still SDB value, not MCQ value.


Thanks.
Peter


> Oops. Can is not CCed. Added now.
> 
> - Mani
> 
> 
> > - Mani
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH 4/8] scsi: ufs: Make .get_hba_mac() optional

[manivannan.sadhasivam]

On Fri, Jun 21, 2024 at 03:32:18AM +0000, Peter Wang (王信友) wrote:
> On Wed, 2024-06-19 at 13:27 +0530, Manivannan Sadhasivam wrote:
> > > > --- a/include/ufs/ufshci.h
> > > > +++ b/include/ufs/ufshci.h
> > > > @@ -67,7 +67,7 @@ enum {
> > > >  
> > > >  /* Controller capability masks */
> > > >  enum {
> > > > -MASK_TRANSFER_REQUESTS_SLOTS= 0x0000001F,
> > > > +MASK_TRANSFER_REQUESTS_SLOTS= 0x000000FF,
> > > 
> > > This should be a separate fix that comes before this patch. But I
> > believe this
> > > came up during MCQ review as well and I don't remember what was the
> > reply from
> > > Can. 0x1F is the mask for SDB mode and 0xFF is the mask for MCQ
> > mode.
> > > 
> > > Can, can you comment more?
> > > 
> > 
> 
> Hi Bart and Mani,
> 
> It is correct that 0x1F is SDB mode.
> ufshcd_mcq_decide_queue_depth is running before mcq enable.
> So that value read is still SDB value, not MCQ value.
> 

I don't quite understand. My concern was that if we change the mask for MCQ,
then existing 'nutrs' value for SDB could be impacted with this change.

Perhaps we should use different masks?

- Mani

> 
> Thanks.
> Peter
> 
> 
> > Oops. Can is not CCed. Added now.
> > 
> > - Mani
> > 
> > 
> > > - Mani
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

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

Re: [PATCH 4/8] scsi: ufs: Make .get_hba_mac() optional

[Peter Wang (王信友)]

On Sun, 2024-06-23 at 19:03 +0530, manivannan.sadhasivam@linaro.org
wrote:
>  	 
> 
> > 
> > Hi Bart and Mani,
> > 
> > It is correct that 0x1F is SDB mode.
> > ufshcd_mcq_decide_queue_depth is running before mcq enable.
> > So that value read is still SDB value, not MCQ value.
> > 
> 
> I don't quite understand. My concern was that if we change the mask
> for MCQ,
> then existing 'nutrs' value for SDB could be impacted with this
> change.
> 
> Perhaps we should use different masks?
> 
> - Mani
> 

Hi Mani,

Yes, it is better use different mask with different mode.

And we can only use 0xFF mask if 0x300[0] = 1 (MCQ enable)
use 0x1F mask if 0x300[0] = 0 (Single doorbell mode)

Mediatek design in MCQ mode is 64, Single doorbell mode is 32.

Thanks.
Peter

Re: [PATCH 4/8] scsi: ufs: Make .get_hba_mac() optional

[Bart Van Assche]

On 6/24/24 1:39 AM, Peter Wang (王信友) wrote:
> On Sun, 2024-06-23 at 19:03 +0530, manivannan.sadhasivam@linaro.org
>> Perhaps we should use different masks?
> 
> Yes, it is better use different mask with different mode.
I will introduce different masks for the SDB and MCQ modes.

Thanks,

Bart.

[PATCH 5/8] scsi: ufs: Declare ufshcd_mcq_poll_cqe_lock() once

[Bart Van Assche]

ufshcd_mcq_poll_cqe_lock() is declared in include/ufs/ufshcd.h and also in
drivers/ufs/core/ufshcd-priv.h. Remove the declaration from the latter file.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd-priv.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index a1add22205db..fb4457a84d11 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -72,8 +72,6 @@ u32 ufshcd_mcq_read_cqis(struct ufs_hba *hba, int i);
 void ufshcd_mcq_write_cqis(struct ufs_hba *hba, u32 val, int i);
 struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba,
 					   struct request *req);
-unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
-				       struct ufs_hw_queue *hwq);
 void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
 				    struct ufs_hw_queue *hwq);
 bool ufshcd_cmd_inflight(struct scsi_cmnd *cmd);

RE: [PATCH 5/8] scsi: ufs: Declare ufshcd_mcq_poll_cqe_lock() once

[Avri Altman]

> ufshcd_mcq_poll_cqe_lock() is declared in include/ufs/ufshcd.h and also in
> drivers/ufs/core/ufshcd-priv.h. Remove the declaration from the latter file.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

[PATCH 6/8] scsi: ufs: Make ufshcd_poll() complain about unsupported arguments

[Bart Van Assche]

The ufshcd_poll() implementation does not support queue_num ==
UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. Hence complain
if queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7761ccca2115..db374a788140 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5562,6 +5562,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
 	struct ufs_hw_queue *hwq;
 
 	if (is_mcq_enabled(hba)) {
+		WARN_ON_ONCE(queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT);
 		hwq = &hba->uhq[queue_num];
 
 		return ufshcd_mcq_poll_cqe_lock(hba, hwq);

Re: [PATCH 6/8] scsi: ufs: Make ufshcd_poll() complain about unsupported arguments

[Manivannan Sadhasivam]

On Mon, Jun 17, 2024 at 02:07:45PM -0700, Bart Van Assche wrote:
> The ufshcd_poll() implementation does not support queue_num ==
> UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. Hence complain
> if queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 7761ccca2115..db374a788140 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5562,6 +5562,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
>  	struct ufs_hw_queue *hwq;
>  
>  	if (is_mcq_enabled(hba)) {
> +		WARN_ON_ONCE(queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT);

So what does the user has to do if this WARN_ON gets triggered? Can't we use
dev_err()/dev_warn() and return instead if the intention is to just report the
error or warning.

I know that UFS code has WARN_ON sprinkled all over, but those should be audited
at some point and also the new additions.

Also, this is a bug fix as it essentially fixes array out of the bounds issue.
So should have a fixes tag and CC stable list for backporting.

- Mani

>  		hwq = &hba->uhq[queue_num];
>  
>  		return ufshcd_mcq_poll_cqe_lock(hba, hwq);

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

Re: [PATCH 6/8] scsi: ufs: Make ufshcd_poll() complain about unsupported arguments

[Bart Van Assche]

On 6/19/24 12:32 AM, Manivannan Sadhasivam wrote:
> On Mon, Jun 17, 2024 at 02:07:45PM -0700, Bart Van Assche wrote:
>> The ufshcd_poll() implementation does not support queue_num ==
>> UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. Hence complain
>> if queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode.
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/ufs/core/ufshcd.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 7761ccca2115..db374a788140 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -5562,6 +5562,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
>>   	struct ufs_hw_queue *hwq;
>>   
>>   	if (is_mcq_enabled(hba)) {
>> +		WARN_ON_ONCE(queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT);
> 
> So what does the user has to do if this WARN_ON gets triggered? Can't we use
> dev_err()/dev_warn() and return instead if the intention is to just report the
> error or warning.
> 
> I know that UFS code has WARN_ON sprinkled all over, but those should be audited
> at some point and also the new additions.
> 
> Also, this is a bug fix as it essentially fixes array out of the bounds issue.
> So should have a fixes tag and CC stable list for backporting.

No, this is not a bug fix. There is only one caller that passes the value
UFSHCD_POLL_FROM_INTERRUPT_CONTEXT as the 'queue_num' argument and it is a code
path that supports legacy mode (single queue mode). Since the above WARN_ON_ONCE()
is in an MCQ code path, it will never be triggered. The above WARN_ON_ONCE() can
be seen as a form of documentation and also as defensive programming. I think
using WARN_ON_ONCE() to document which code paths will never be triggered is fine.

Thanks,

Bart.

Re: [PATCH 6/8] scsi: ufs: Make ufshcd_poll() complain about unsupported arguments

[Manivannan Sadhasivam]

On Thu, Jun 20, 2024 at 01:13:10PM -0700, Bart Van Assche wrote:
> On 6/19/24 12:32 AM, Manivannan Sadhasivam wrote:
> > On Mon, Jun 17, 2024 at 02:07:45PM -0700, Bart Van Assche wrote:
> > > The ufshcd_poll() implementation does not support queue_num ==
> > > UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode. Hence complain
> > > if queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT in MCQ mode.
> > > 
> > > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > > ---
> > >   drivers/ufs/core/ufshcd.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > > index 7761ccca2115..db374a788140 100644
> > > --- a/drivers/ufs/core/ufshcd.c
> > > +++ b/drivers/ufs/core/ufshcd.c
> > > @@ -5562,6 +5562,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
> > >   	struct ufs_hw_queue *hwq;
> > >   	if (is_mcq_enabled(hba)) {
> > > +		WARN_ON_ONCE(queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT);
> > 
> > So what does the user has to do if this WARN_ON gets triggered? Can't we use
> > dev_err()/dev_warn() and return instead if the intention is to just report the
> > error or warning.
> > 
> > I know that UFS code has WARN_ON sprinkled all over, but those should be audited
> > at some point and also the new additions.
> > 
> > Also, this is a bug fix as it essentially fixes array out of the bounds issue.
> > So should have a fixes tag and CC stable list for backporting.
> 
> No, this is not a bug fix. There is only one caller that passes the value
> UFSHCD_POLL_FROM_INTERRUPT_CONTEXT as the 'queue_num' argument and it is a code
> path that supports legacy mode (single queue mode). Since the above WARN_ON_ONCE()
> is in an MCQ code path, it will never be triggered. The above WARN_ON_ONCE() can
> be seen as a form of documentation and also as defensive programming. I think
> using WARN_ON_ONCE() to document which code paths will never be triggered is fine.
> 

Why should we insert a warning in a dead code? WARN_ON* makes sense if a certain
condition is never expected to happen, but if that happens then most likely
something wrong happened seriously so the users should be warned.

But here I don't see a possibility to get this triggered at all. Please correct
me if I'm wrong.

- Mani

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

[PATCH 7/8] scsi: ufs: Make the polling code report which command has been completed

[Bart Van Assche]

Prepare for introducing a new __ufshcd_poll() caller that will need to
know whether or not a specific command has been completed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufs-mcq.c      | 25 +++++++++++++-------
 drivers/ufs/core/ufshcd-priv.h  |  4 ++--
 drivers/ufs/core/ufshcd.c       | 41 +++++++++++++++++++++++----------
 drivers/ufs/host/ufs-mediatek.c |  2 +-
 drivers/ufs/host/ufs-qcom.c     |  2 +-
 include/ufs/ufshcd.h            |  3 ++-
 6 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index d6f966f4abef..c9155759934c 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -294,17 +294,22 @@ static int ufshcd_mcq_get_tag(struct ufs_hba *hba, struct cq_entry *cqe)
 	return div_u64(addr, ufshcd_get_ucd_size(hba));
 }
 
-static void ufshcd_mcq_process_cqe(struct ufs_hba *hba,
-				   struct ufs_hw_queue *hwq)
+/* Returns true if and only if @compl_cmd has been completed. */
+static bool ufshcd_mcq_process_cqe(struct ufs_hba *hba,
+				   struct ufs_hw_queue *hwq,
+				   struct scsi_cmnd *compl_cmd)
 {
 	struct cq_entry *cqe = ufshcd_mcq_cur_cqe(hwq);
-	int tag = ufshcd_mcq_get_tag(hba, cqe);
 
 	if (cqe->command_desc_base_addr) {
-		ufshcd_compl_one_cqe(hba, tag, cqe);
-		/* After processed the cqe, mark it empty (invalid) entry */
+		const int tag = ufshcd_mcq_get_tag(hba, cqe);
+
+		/* Mark the CQE as invalid. */
 		cqe->command_desc_base_addr = 0;
+
+		return ufshcd_compl_one_cqe(hba, tag, cqe, compl_cmd);
 	}
+	return false;
 }
 
 void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
@@ -315,7 +320,7 @@ void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
 
 	spin_lock_irqsave(&hwq->cq_lock, flags);
 	while (entries > 0) {
-		ufshcd_mcq_process_cqe(hba, hwq);
+		ufshcd_mcq_process_cqe(hba, hwq, NULL);
 		ufshcd_mcq_inc_cq_head_slot(hwq);
 		entries--;
 	}
@@ -325,8 +330,10 @@ void ufshcd_mcq_compl_all_cqes_lock(struct ufs_hba *hba,
 	spin_unlock_irqrestore(&hwq->cq_lock, flags);
 }
 
+/* Clears *@compl_cmd if and only if *@compl_cmd has been completed. */
 unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
-				       struct ufs_hw_queue *hwq)
+				       struct ufs_hw_queue *hwq,
+				       struct scsi_cmnd **compl_cmd)
 {
 	unsigned long completed_reqs = 0;
 	unsigned long flags;
@@ -334,7 +341,9 @@ unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
 	spin_lock_irqsave(&hwq->cq_lock, flags);
 	ufshcd_mcq_update_cq_tail_slot(hwq);
 	while (!ufshcd_mcq_is_cq_empty(hwq)) {
-		ufshcd_mcq_process_cqe(hba, hwq);
+		if (ufshcd_mcq_process_cqe(hba, hwq,
+					   compl_cmd ? *compl_cmd : NULL))
+			*compl_cmd = NULL;
 		ufshcd_mcq_inc_cq_head_slot(hwq);
 		completed_reqs++;
 	}
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index fb4457a84d11..42802fd689fb 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -61,8 +61,8 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
 int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 	enum flag_idn idn, u8 index, bool *flag_res);
 void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit);
-void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
-			  struct cq_entry *cqe);
+bool ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
+			  struct cq_entry *cqe, struct scsi_cmnd *compl_cmd);
 int ufshcd_mcq_init(struct ufs_hba *hba);
 int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba);
 int ufshcd_mcq_memory_alloc(struct ufs_hba *hba);
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index db374a788140..e3835e61e4b1 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5488,9 +5488,12 @@ void ufshcd_release_scsi_cmd(struct ufs_hba *hba,
  * @hba: per adapter instance
  * @task_tag: the task tag of the request to be completed
  * @cqe: pointer to the completion queue entry
+ * @compl_cmd: if not NULL, check whether this command has been completed
+ *
+ * Returns: true if and only if @compl_cmd has been completed.
  */
-void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
-			  struct cq_entry *cqe)
+bool ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
+			  struct cq_entry *cqe, struct scsi_cmnd *compl_cmd)
 {
 	struct ufshcd_lrb *lrbp;
 	struct scsi_cmnd *cmd;
@@ -5507,6 +5510,7 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 		ufshcd_release_scsi_cmd(hba, lrbp);
 		/* Do not touch lrbp after scsi done */
 		scsi_done(cmd);
+		return cmd == compl_cmd;
 	} else if (hba->dev_cmd.complete) {
 		if (cqe) {
 			ocs = le32_to_cpu(cqe->status) & MASK_OCS;
@@ -5514,20 +5518,26 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 		}
 		complete(hba->dev_cmd.complete);
 	}
+	return false;
 }
 
 /**
  * __ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
  * @completed_reqs: bitmask that indicates which requests to complete
+ * @compl_cmd: if not NULL, check whether *@compl_cmd has been completed.
+ *	Clear *@compl_cmd if it has been completed.
  */
 static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
-					unsigned long completed_reqs)
+					unsigned long completed_reqs,
+					struct scsi_cmnd **compl_cmd)
 {
 	int tag;
 
 	for_each_set_bit(tag, &completed_reqs, hba->nutrs)
-		ufshcd_compl_one_cqe(hba, tag, NULL);
+		if (ufshcd_compl_one_cqe(hba, tag, NULL,
+					 compl_cmd ? *compl_cmd : NULL))
+			*compl_cmd = NULL;
 }
 
 /* Any value that is not an existing queue number is fine for this constant. */
@@ -5554,7 +5564,8 @@ static void ufshcd_clear_polled(struct ufs_hba *hba,
  * Return: > 0 if one or more commands have been completed or 0 if no
  * requests have been completed.
  */
-static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
+static int __ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num,
+			 struct scsi_cmnd **compl_cmd)
 {
 	struct ufs_hba *hba = shost_priv(shost);
 	unsigned long completed_reqs, flags;
@@ -5565,7 +5576,7 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
 		WARN_ON_ONCE(queue_num == UFSHCD_POLL_FROM_INTERRUPT_CONTEXT);
 		hwq = &hba->uhq[queue_num];
 
-		return ufshcd_mcq_poll_cqe_lock(hba, hwq);
+		return ufshcd_mcq_poll_cqe_lock(hba, hwq, compl_cmd);
 	}
 
 	spin_lock_irqsave(&hba->outstanding_lock, flags);
@@ -5582,11 +5593,16 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
 	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
 
 	if (completed_reqs)
-		__ufshcd_transfer_req_compl(hba, completed_reqs);
+		__ufshcd_transfer_req_compl(hba, completed_reqs, compl_cmd);
 
 	return completed_reqs != 0;
 }
 
+static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
+{
+	return __ufshcd_poll(shost, queue_num, NULL);
+}
+
 /**
  * ufshcd_mcq_compl_pending_transfer - MCQ mode function. It is
  * invoked from the error handler context or ufshcd_host_reset_and_restore()
@@ -5630,7 +5646,7 @@ static void ufshcd_mcq_compl_pending_transfer(struct ufs_hba *hba,
 			}
 			spin_unlock_irqrestore(&hwq->cq_lock, flags);
 		} else {
-			ufshcd_mcq_poll_cqe_lock(hba, hwq);
+			ufshcd_mcq_poll_cqe_lock(hba, hwq, NULL);
 		}
 	}
 }
@@ -6905,7 +6921,7 @@ static irqreturn_t ufshcd_handle_mcq_cq_events(struct ufs_hba *hba)
 			ufshcd_mcq_write_cqis(hba, events, i);
 
 		if (events & UFSHCD_MCQ_CQIS_TAIL_ENT_PUSH_STS)
-			ufshcd_mcq_poll_cqe_lock(hba, hwq);
+			ufshcd_mcq_poll_cqe_lock(hba, hwq, NULL);
 	}
 
 	return IRQ_HANDLED;
@@ -7398,7 +7414,7 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
 			    lrbp->lun == lun) {
 				ufshcd_clear_cmd(hba, pos);
 				hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(lrbp->cmd));
-				ufshcd_mcq_poll_cqe_lock(hba, hwq);
+				ufshcd_mcq_poll_cqe_lock(hba, hwq, NULL);
 			}
 		}
 		err = 0;
@@ -7426,7 +7442,8 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
 				__func__, pos);
 		}
 	}
-	__ufshcd_transfer_req_compl(hba, pending_reqs & ~not_cleared_mask);
+	__ufshcd_transfer_req_compl(hba, pending_reqs & ~not_cleared_mask,
+				    NULL);
 
 out:
 	hba->req_abort_count = 0;
@@ -7603,7 +7620,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		dev_err(hba->dev,
 		"%s: cmd was completed, but without a notifying intr, tag = %d",
 		__func__, tag);
-		__ufshcd_transfer_req_compl(hba, 1UL << tag);
+		__ufshcd_transfer_req_compl(hba, 1UL << tag, NULL);
 		goto release;
 	}
 
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index c7a0ab9b1f59..4dbe334b36bb 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1715,7 +1715,7 @@ static irqreturn_t ufs_mtk_mcq_intr(int irq, void *__intr_info)
 		ufshcd_mcq_write_cqis(hba, events, qid);
 
 	if (events & UFSHCD_MCQ_CQIS_TAIL_ENT_PUSH_STS)
-		ufshcd_mcq_poll_cqe_lock(hba, hwq);
+		ufshcd_mcq_poll_cqe_lock(hba, hwq, NULL);
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index cca190d1c577..cc1d9614fcd6 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1715,7 +1715,7 @@ static irqreturn_t ufs_qcom_mcq_esi_handler(int irq, void *data)
 	struct ufs_hw_queue *hwq = &hba->uhq[id];
 
 	ufshcd_mcq_write_cqis(hba, 0x1, id);
-	ufshcd_mcq_poll_cqe_lock(hba, hwq);
+	ufshcd_mcq_poll_cqe_lock(hba, hwq, NULL);
 
 	return IRQ_HANDLED;
 }
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d32637d267f3..443afb97a637 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1284,7 +1284,8 @@ unsigned int ufshcd_mcq_queue_cfg_addr(struct ufs_hba *hba);
 u32 ufshcd_mcq_read_cqis(struct ufs_hba *hba, int i);
 void ufshcd_mcq_write_cqis(struct ufs_hba *hba, u32 val, int i);
 unsigned long ufshcd_mcq_poll_cqe_lock(struct ufs_hba *hba,
-					 struct ufs_hw_queue *hwq);
+				       struct ufs_hw_queue *hwq,
+				       struct scsi_cmnd **compl_cmd);
 void ufshcd_mcq_make_queues_operational(struct ufs_hba *hba);
 void ufshcd_mcq_enable_esi(struct ufs_hba *hba);
 void ufshcd_mcq_enable(struct ufs_hba *hba);

[PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Bart Van Assche]

If ufshcd_abort() returns SUCCESS for an already completed command then
that command is completed twice. This results in a crash. Prevent this by
checking whether a command has completed without completion interrupt from
the timeout handler. This CL fixes the following kernel crash:

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
Call trace:
 dma_direct_map_sg+0x70/0x274
 scsi_dma_map+0x84/0x124
 ufshcd_queuecommand+0x3fc/0x880
 scsi_queue_rq+0x7d0/0x111c
 blk_mq_dispatch_rq_list+0x440/0xebc
 blk_mq_do_dispatch_sched+0x5a4/0x6b8
 __blk_mq_sched_dispatch_requests+0x150/0x220
 __blk_mq_run_hw_queue+0xf0/0x218
 __blk_mq_delay_run_hw_queue+0x8c/0x18c
 blk_mq_run_hw_queue+0x1a4/0x360
 blk_mq_sched_insert_requests+0x130/0x334
 blk_mq_flush_plug_list+0x138/0x234
 blk_flush_plug_list+0x118/0x164
 blk_finish_plug()
 read_pages+0x38c/0x408
 page_cache_ra_unbounded+0x230/0x2f8
 do_sync_mmap_readahead+0x1a4/0x208
 filemap_fault+0x27c/0x8f4
 f2fs_filemap_fault+0x28/0xfc
 __do_fault+0xc4/0x208
 handle_pte_fault+0x290/0xe04
 do_handle_mm_fault+0x52c/0x858
 do_page_fault+0x5dc/0x798
 do_translation_fault+0x40/0x54
 do_mem_abort+0x60/0x134
 el0_da+0x40/0xb8
 el0t_64_sync_handler+0xc4/0xe4
 el0t_64_sync+0x1b4/0x1b8

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index e3835e61e4b1..47cc0802c4f4 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8922,7 +8922,28 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 
 static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
 {
-	struct ufs_hba *hba = shost_priv(scmd->device->host);
+	struct scsi_device *sdev = scmd->device;
+	struct ufs_hba *hba = shost_priv(sdev->host);
+	struct scsi_cmnd *cmd2 = scmd;
+	const u32 unique_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd));
+
+	WARN_ON_ONCE(!scmd);
+
+	if (is_mcq_enabled(hba)) {
+		struct request *rq = scsi_cmd_to_rq(scmd);
+		struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
+
+		ufshcd_mcq_poll_cqe_lock(hba, hwq, &cmd2);
+	} else {
+		__ufshcd_poll(hba->host, UFSHCD_POLL_FROM_INTERRUPT_CONTEXT,
+			      &cmd2);
+	}
+	if (cmd2 == NULL) {
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: cmd with tag %#x has already been completed\n",
+			    __func__, unique_tag);
+		return SCSI_EH_DONE;
+	}
 
 	if (!hba->system_suspending) {
 		/* Activate the error handler in the SCSI core. */

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Peter Wang (王信友)]

On Mon, 2024-06-17 at 14:07 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  If ufshcd_abort() returns SUCCESS for an already completed command
> then
> that command is completed twice. This results in a crash. Prevent
> this by
> checking whether a command has completed without completion interrupt
> from
> the timeout handler. This CL fixes the following kernel crash:
> 
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000000
> Call trace:
>  dma_direct_map_sg+0x70/0x274
>  scsi_dma_map+0x84/0x124
>  ufshcd_queuecommand+0x3fc/0x880
>  scsi_queue_rq+0x7d0/0x111c
>  blk_mq_dispatch_rq_list+0x440/0xebc
>  blk_mq_do_dispatch_sched+0x5a4/0x6b8
>  __blk_mq_sched_dispatch_requests+0x150/0x220
>  __blk_mq_run_hw_queue+0xf0/0x218
>  __blk_mq_delay_run_hw_queue+0x8c/0x18c
>  blk_mq_run_hw_queue+0x1a4/0x360
>  blk_mq_sched_insert_requests+0x130/0x334
>  blk_mq_flush_plug_list+0x138/0x234
>  blk_flush_plug_list+0x118/0x164
>  blk_finish_plug()
>  read_pages+0x38c/0x408
>  page_cache_ra_unbounded+0x230/0x2f8
>  do_sync_mmap_readahead+0x1a4/0x208
>  filemap_fault+0x27c/0x8f4
>  f2fs_filemap_fault+0x28/0xfc
>  __do_fault+0xc4/0x208
>  handle_pte_fault+0x290/0xe04
>  do_handle_mm_fault+0x52c/0x858
>  do_page_fault+0x5dc/0x798
>  do_translation_fault+0x40/0x54
>  do_mem_abort+0x60/0x134
>  el0_da+0x40/0xb8
>  el0t_64_sync_handler+0xc4/0xe4
>  el0t_64_sync+0x1b4/0x1b8
> 

Hi Bart,

This backtrace is ufshcd_queuecommand KE.
If ufshcd_abort() complete an already completed command, 
it should be KE with ufshcd_abort backtrace?

More, if a command is completed by irq.
The rq may be release and ufshcd_mcq_req_to_hwq(hba, rq) will get KE
Here is our backtrace of this case.

platform +platform:112b0000.ufshci ufshcd-mtk 112b0000.ufshci:
ufshcd_try_to_abort_task: cmd at tag 41 not pending in the device.
platform +platform:112b0000.ufshci ufshcd-mtk 112b0000.ufshci:
ufshcd_try_to_abort_task: cmd at tag=41 is cleared.
platform +platform:112b0000.ufshci ufshcd-mtk 112b0000.ufshci: Aborting
tag 41 / CDB 0x28 succeeded
  Unable to handle kernel NULL pointer dereference at virtual address
0000000000000194
  pc : [0xffffffddd7a79bf8] blk_mq_unique_tag+0x8/0x14
  lr : [0xffffffddd6155b84] ufshcd_mcq_req_to_hwq+0x1c/0x40
[ufs_mediatek_mod_ise]
   do_mem_abort+0x58/0x118
   el1_abort+0x3c/0x5c
   el1h_64_sync_handler+0x54/0x90
   el1h_64_sync+0x68/0x6c
   blk_mq_unique_tag+0x8/0x14
   ufshcd_err_handler+0xae4/0xfa8 [ufs_mediatek_mod_ise]
   process_one_work+0x208/0x4fc
   worker_thread+0x228/0x438
   kthread+0x104/0x1d4
   ret_from_fork+0x10/0x20

Thanks.
Peter



> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index e3835e61e4b1..47cc0802c4f4 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8922,7 +8922,28 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
>  
>  static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd
> *scmd)
>  {
> -struct ufs_hba *hba = shost_priv(scmd->device->host);
> +struct scsi_device *sdev = scmd->device;
> +struct ufs_hba *hba = shost_priv(sdev->host);
> +struct scsi_cmnd *cmd2 = scmd;
> +const u32 unique_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd));
> +
> +WARN_ON_ONCE(!scmd);
> +
> +if (is_mcq_enabled(hba)) {
> +struct request *rq = scsi_cmd_to_rq(scmd);
> +struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
> +
> +ufshcd_mcq_poll_cqe_lock(hba, hwq, &cmd2);
> +} else {
> +__ufshcd_poll(hba->host, UFSHCD_POLL_FROM_INTERRUPT_CONTEXT,
> +      &cmd2);
> +}
> +if (cmd2 == NULL) {
> +sdev_printk(KERN_INFO, sdev,
> +    "%s: cmd with tag %#x has already been completed\n",
> +    __func__, unique_tag);
> +return SCSI_EH_DONE;
> +}
>  
>  if (!hba->system_suspending) {
>  /* Activate the error handler in the SCSI core. */

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Bart Van Assche]

On 6/20/24 11:54 PM, Peter Wang (王信友) wrote:
>    Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000194
>    pc : [0xffffffddd7a79bf8] blk_mq_unique_tag+0x8/0x14
>    lr : [0xffffffddd6155b84] ufshcd_mcq_req_to_hwq+0x1c/0x40
> [ufs_mediatek_mod_ise]
>     do_mem_abort+0x58/0x118
>     el1_abort+0x3c/0x5c
>     el1h_64_sync_handler+0x54/0x90
>     el1h_64_sync+0x68/0x6c
>     blk_mq_unique_tag+0x8/0x14
>     ufshcd_err_handler+0xae4/0xfa8 [ufs_mediatek_mod_ise]
>     process_one_work+0x208/0x4fc
>     worker_thread+0x228/0x438
>     kthread+0x104/0x1d4
>     ret_from_fork+0x10/0x20

Hi Peter,

The above backtrace can only occur with MCQ enabled. The backtrace I
posted was triggered on a system with a UFSHCI 3.0 controller (no MCQ).
So I think that the backtraces have different root causes and hence that
different patches are required to fix both crashes.

Thanks,

Bart.

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Peter Wang (王信友)]

On Fri, 2024-06-21 at 10:23 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 6/20/24 11:54 PM, Peter Wang (王信友) wrote:
> >    Unable to handle kernel NULL pointer dereference at virtual
> address
> > 0000000000000194
> >    pc : [0xffffffddd7a79bf8] blk_mq_unique_tag+0x8/0x14
> >    lr : [0xffffffddd6155b84] ufshcd_mcq_req_to_hwq+0x1c/0x40
> > [ufs_mediatek_mod_ise]
> >     do_mem_abort+0x58/0x118
> >     el1_abort+0x3c/0x5c
> >     el1h_64_sync_handler+0x54/0x90
> >     el1h_64_sync+0x68/0x6c
> >     blk_mq_unique_tag+0x8/0x14
> >     ufshcd_err_handler+0xae4/0xfa8 [ufs_mediatek_mod_ise]
> >     process_one_work+0x208/0x4fc
> >     worker_thread+0x228/0x438
> >     kthread+0x104/0x1d4
> >     ret_from_fork+0x10/0x20
> 
> Hi Peter,
> 
> The above backtrace can only occur with MCQ enabled. The backtrace I
> posted was triggered on a system with a UFSHCI 3.0 controller (no
> MCQ).
> So I think that the backtraces have different root causes and hence
> that
> different patches are required to fix both crashes.
> 
> Thanks,
> 
> Bart.
> 

Hi Bart,

Your backtrace is Single doorbell mode. But I am curious that
how could it happen if complete a cmd twice and get null pointer
next time queuecommand? could you describe the exactly flow?

More, because ufshcd_eh_timed_out could run in MCQ mode.
So, this patch will get null pointer when racing happen in MCQ mode.

Thanks
Peter

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Bart Van Assche]

On 6/24/24 1:54 AM, Peter Wang (王信友) wrote:
> Your backtrace is Single doorbell mode. But I am curious that
> how could it happen if complete a cmd twice and get null pointer
> next time queuecommand? could you describe the exactly flow?

SCSI commands are completed only once. See also the code in the SCSI
core that sets the SCMD_STATE_COMPLETE bit:

$ git grep -nH 'test_and_set_bit(SCMD_STATE_COMPLETE'
drivers/scsi/scsi_error.c:362:	if (test_and_set_bit(SCMD_STATE_COMPLETE, 
&scmd->state))
drivers/scsi/scsi_lib.c:1716:	if 
(unlikely(test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->state)))

In other words, either the regular completion code is executed by
scsi_done_internal() or error handling is initiated by scsi_timeout().
Only one of the two happens.

The only exception is that .eh_timed_out() may be called concurrently
with the regular completion handler. Hence this patch for
ufshcd_eh_timed_out().

Bart.

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Peter Wang (王信友)]

On Mon, 2024-06-24 at 11:12 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 6/24/24 1:54 AM, Peter Wang (王信友) wrote:
> > Your backtrace is Single doorbell mode. But I am curious that
> > how could it happen if complete a cmd twice and get null pointer
> > next time queuecommand? could you describe the exactly flow?
> 
> SCSI commands are completed only once. See also the code in the SCSI
> core that sets the SCMD_STATE_COMPLETE bit:
> 
> $ git grep -nH 'test_and_set_bit(SCMD_STATE_COMPLETE'
> drivers/scsi/scsi_error.c:362:if
> (test_and_set_bit(SCMD_STATE_COMPLETE, 
> &scmd->state))
> drivers/scsi/scsi_lib.c:1716:if 
> (unlikely(test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->state)))
> 
> In other words, either the regular completion code is executed by
> scsi_done_internal() or error handling is initiated by
> scsi_timeout().
> Only one of the two happens.
> 
> The only exception is that .eh_timed_out() may be called concurrently
> with the regular completion handler. Hence this patch for
> ufshcd_eh_timed_out().
> 
> Bart.


Hi Bart,

I still confused.
When eh_timed_out() called concurrently with the regular completion
handler.
Both process use test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->state) to
set SCMD_STATE_COMPLETE flag.
test_and_set_bit should be atomice operation? and only one can set this
bit than run forward?

BTW, I still don't understand if both eh_timed_out and regular
completion handler set SCMD_STATE_COMPLETE,
what is the relation between SCMD_STATE_COMPLETE and
ufshcd_queuecommand null pointer?

Thanks.
Peter

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Bart Van Assche]

On 6/25/24 3:04 AM, Peter Wang (王信友) wrote:
> When eh_timed_out() called concurrently with the regular completion
> handler.
> Both process use test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->state) to
> set SCMD_STATE_COMPLETE flag.
> test_and_set_bit should be atomice operation? and only one can set this
> bit than run forward?

Yes, only one of the two test_and_set_bit(SCMD_STATE_COMPLETE,
&cmd->state) calls will set the SCMD_STATE_COMPLETE bit and only
one of these two calls will return the value 'true'.

> BTW, I still don't understand if both eh_timed_out and regular
> completion handler set SCMD_STATE_COMPLETE,
> what is the relation between SCMD_STATE_COMPLETE and
> ufshcd_queuecommand null pointer?

While ufshcd_eh_timed_out() does not set the SCMD_STATE_COMPLETE bit,
its caller may set that bit after ufshcd_eh_timed_out() has returned.
Hence, a command may be completed while ufshcd_eh_timed_out() is in
progress.

Thanks,

Bart.

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Peter Wang (王信友)]

On Tue, 2024-06-25 at 09:33 -0700, Bart Van Assche wrote:
> 
> 
> While ufshcd_eh_timed_out() does not set the SCMD_STATE_COMPLETE bit,
> its caller may set that bit after ufshcd_eh_timed_out() has returned.
> Hence, a command may be completed while ufshcd_eh_timed_out() is in
> progress.
> 
> Thanks,
> 
> Bart.

Hi Bart,

So, when this concurrency issue happen, which one set the
SCMD_STATE_COMPLETE flag?
scsi_timeout or scsi_done_internal?
And why ufshcd_queuecommand got null pointer? which pointer is null?



Thanks.
Peter

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Bart Van Assche]

On 6/25/24 8:54 PM, Peter Wang (王信友) wrote:
> And why ufshcd_queuecommand got null pointer? which pointer is null?

I'm not sure. faddr2line reports that the crash happens in the source
code line with the following assignment: "sg_dma_len(sg) = sg->length".
That seems weird to me. If the sg pointer would be invalid then an
earlier dereference of the 'sg' pointer should already have triggered a
crash. The entire function is as follows:

int dma_direct_map_sg(struct device *dev, struct scatterlist *sgl, int 
nents,
		enum dma_data_direction dir, unsigned long attrs)
{
	int i;
	struct scatterlist *sg;

	for_each_sg(sgl, sg, nents, i) {
		sg->dma_address = dma_direct_map_page(dev, sg_page(sg),
				sg->offset, sg->length, dir, attrs);
		if (sg->dma_address == DMA_MAPPING_ERROR)
			goto out_unmap;
		sg_dma_len(sg) = sg->length;
	}

	return nents;

out_unmap:
	dma_direct_unmap_sg(dev, sgl, i, dir, attrs | DMA_ATTR_SKIP_CPU_SYNC);
	return -EIO;
}

Bart.

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Peter Wang (王信友)]

On Wed, 2024-06-26 at 14:54 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 6/25/24 8:54 PM, Peter Wang (王信友) wrote:
> > And why ufshcd_queuecommand got null pointer? which pointer is
> null?
> 
> I'm not sure. faddr2line reports that the crash happens in the source
> code line with the following assignment: "sg_dma_len(sg) = sg-
> >length".
> That seems weird to me. If the sg pointer would be invalid then an
> earlier dereference of the 'sg' pointer should already have triggered
> a
> 



Hi Bart,

This is really weird.
Perphas it is dram corrupt issue?
And is unrelated to the ufshcd_abort racing I think.


Thanks.
Peter

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Bart Van Assche]

On 6/27/24 3:56 AM, Peter Wang (王信友) wrote:
> This is really weird.
> Perphas it is dram corrupt issue?
> And is unrelated to the ufshcd_abort racing I think.

We have seen this crash five times with kernel 5.15. I think that
the number of occurrences is too high to be caused by DRAM corruption.
Anyway, I will leave out patches 7/8 and 8/8 from this patch series.
We can revisit these patches if the issue would be observed again with
a later kernel.

Bart.

Re: [PATCH 8/8] scsi: ufs: Check for completion from the timeout handler

[Wenchao Hao]

On 2024/6/18 5:07, Bart Van Assche wrote:
> If ufshcd_abort() returns SUCCESS for an already completed command then
> that command is completed twice. This results in a crash. Prevent this by
> checking whether a command has completed without completion interrupt from
> the timeout handler. This CL fixes the following kernel crash:
> 

Hi Bart,

Could you describe in more detail about how command completed twice happened?
I think this would not happen, below is my analysis, if it is wrong, please
correct me.

In your description, ufshcd_abort() returns SUCCESS is condition which trigger
the issue.

There are 2 paths would call ufshcd_abort(). The first one is triggered by
block layer's timeout, which is:

scsi_timeout()
        hostt->eh_timed_out() [ufshcd_eh_timed_out]
                                // return SCSI_EH_NOT_HANDLED
        test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state)
                                // return true
        scsi_abort_command()
                queue_delayed_work(shost->tmf_work_q, &scmd->abort_work, ...)

The above flow would trigger ufshcd_abort() be called in workqueue:

scmd_eh_abort_handler()
        scsi_try_to_abort_cmd()
                hostt->eh_abort_handler() [ufshcd_abort]

While when ufshcd_abort() is called by above flow, the command has been marked
as SCMD_STATE_COMPLETE, and it means scsi_timeout() win the scsi_done(), so 
normal context(usually  triggered by driver's irq handler which call scsi_done())
would not handle this command any more.
The only path which would handle this command is scmd_eh_abort_handler() if
ufshcd_abort() return SUCCESS.

Is any other context which would finish this command?

Another path is:

scsi_send_eh_cmnd()
        shost->hostt->queuecommand() // queued command did not finish in time
        scsi_abort_eh_cmnd()

This is error handler path when host is set to RECOVERY state, no new normal
command would be send any more. What's more, if ufshcd_abort() is called now,
the aborted command is also command send in scsi_send_eh_cmnd().

> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> Call trace:
>  dma_direct_map_sg+0x70/0x274
>  scsi_dma_map+0x84/0x124
>  ufshcd_queuecommand+0x3fc/0x880
>  scsi_queue_rq+0x7d0/0x111c
>  blk_mq_dispatch_rq_list+0x440/0xebc
>  blk_mq_do_dispatch_sched+0x5a4/0x6b8
>  __blk_mq_sched_dispatch_requests+0x150/0x220
>  __blk_mq_run_hw_queue+0xf0/0x218
>  __blk_mq_delay_run_hw_queue+0x8c/0x18c
>  blk_mq_run_hw_queue+0x1a4/0x360
>  blk_mq_sched_insert_requests+0x130/0x334
>  blk_mq_flush_plug_list+0x138/0x234
>  blk_flush_plug_list+0x118/0x164
>  blk_finish_plug()
>  read_pages+0x38c/0x408
>  page_cache_ra_unbounded+0x230/0x2f8
>  do_sync_mmap_readahead+0x1a4/0x208
>  filemap_fault+0x27c/0x8f4
>  f2fs_filemap_fault+0x28/0xfc
>  __do_fault+0xc4/0x208
>  handle_pte_fault+0x290/0xe04
>  do_handle_mm_fault+0x52c/0x858
>  do_page_fault+0x5dc/0x798
>  do_translation_fault+0x40/0x54
>  do_mem_abort+0x60/0x134
>  el0_da+0x40/0xb8
>  el0t_64_sync_handler+0xc4/0xe4
>  el0t_64_sync+0x1b4/0x1b8
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index e3835e61e4b1..47cc0802c4f4 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8922,7 +8922,28 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  
>  static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>  {
> -	struct ufs_hba *hba = shost_priv(scmd->device->host);
> +	struct scsi_device *sdev = scmd->device;
> +	struct ufs_hba *hba = shost_priv(sdev->host);
> +	struct scsi_cmnd *cmd2 = scmd;
> +	const u32 unique_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd));
> +
> +	WARN_ON_ONCE(!scmd);
> +
> +	if (is_mcq_enabled(hba)) {
> +		struct request *rq = scsi_cmd_to_rq(scmd);
> +		struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
> +
> +		ufshcd_mcq_poll_cqe_lock(hba, hwq, &cmd2);
> +	} else {
> +		__ufshcd_poll(hba->host, UFSHCD_POLL_FROM_INTERRUPT_CONTEXT,
> +			      &cmd2);
> +	}
> +	if (cmd2 == NULL) {
> +		sdev_printk(KERN_INFO, sdev,
> +			    "%s: cmd with tag %#x has already been completed\n",
> +			    __func__, unique_tag);
> +		return SCSI_EH_DONE;
> +	}
>  
>  	if (!hba->system_suspending) {
>  		/* Activate the error handler in the SCSI core. */
>