[PATCH v2 0/2] scsi: ufs: introduce a callback to override OCS value

[PATCH v2 0/2] scsi: ufs: introduce a callback to override OCS value

[Kiwoong Kim]

UFSHCI defines OCS values but doesn't specify what exact
conditions raise them. So I think it needs another callback
to replace the original OCS value with the value that works
the way you want.

v1 -> v2: fix build error for arguments

Kiwoong Kim (2):
  scsi: ufs: core: introduce override_cqe_ocs
  scsi: ufs: ufs-exynos: implement override_cqe_ocs

 drivers/ufs/core/ufshcd-priv.h |  9 +++++++++
 drivers/ufs/core/ufshcd.c      | 11 +++++++----
 drivers/ufs/host/ufs-exynos.c  |  8 ++++++++
 include/ufs/ufshcd.h           |  1 +
 4 files changed, 25 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] scsi: ufs: core: introduce override_cqe_ocs

[Kiwoong Kim]

This patch is to declare override_cqe_ocs callback to
override OCS value.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/ufs/core/ufshcd-priv.h |  9 +++++++++
 drivers/ufs/core/ufshcd.c      | 11 +++++++----
 include/ufs/ufshcd.h           |  1 +
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index ce36154..6ebc830 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -275,6 +275,15 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba)
 	return -EOPNOTSUPP;
 }
 
+static inline enum utp_ocs ufshcd_vops_override_cqe_ocs(struct ufs_hba *hba,
+							enum utp_ocs ocs)
+{
+	if (hba->vops && hba->vops->override_cqe_ocs)
+		return hba->vops->override_cqe_ocs(ocs);
+
+	return ocs;
+}
+
 extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[];
 
 /**
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0dd2605..0615e37 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -821,11 +821,14 @@ static inline bool ufshcd_is_device_present(struct ufs_hba *hba)
  *
  * Return: the OCS field in the UTRD.
  */
-static enum utp_ocs ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp,
+static enum utp_ocs ufshcd_get_tr_ocs(struct ufs_hba *hba,
+				      struct ufshcd_lrb *lrbp,
 				      struct cq_entry *cqe)
 {
 	if (cqe)
-		return le32_to_cpu(cqe->status) & MASK_OCS;
+		return ufshcd_vops_override_cqe_ocs(hba,
+						    le32_to_cpu(cqe->status) &
+						    MASK_OCS);
 
 	return lrbp->utr_descriptor_ptr->header.ocs & MASK_OCS;
 }
@@ -3180,7 +3183,7 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 		 * not trigger any race conditions.
 		 */
 		hba->dev_cmd.complete = NULL;
-		err = ufshcd_get_tr_ocs(lrbp, NULL);
+		err = ufshcd_get_tr_ocs(hba, lrbp, NULL);
 		if (!err)
 			err = ufshcd_dev_cmd_completion(hba, lrbp);
 	} else {
@@ -5351,7 +5354,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
 		scsi_set_resid(lrbp->cmd, resid);
 
 	/* overall command status of utrd */
-	ocs = ufshcd_get_tr_ocs(lrbp, cqe);
+	ocs = ufshcd_get_tr_ocs(hba, lrbp, cqe);
 
 	if (hba->quirks & UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR) {
 		if (lrbp->ucd_rsp_ptr->header.response ||
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a43b142..3dbd3e4 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -382,6 +382,7 @@ struct ufs_hba_variant_ops {
 	int	(*get_outstanding_cqs)(struct ufs_hba *hba,
 				       unsigned long *ocqs);
 	int	(*config_esi)(struct ufs_hba *hba);
+	enum utp_ocs	(*override_cqe_ocs)(enum utp_ocs);
 };
 
 /* clock gating state  */
-- 
2.7.4

[PATCH v2 2/2] scsi: ufs: ufs-exynos: implement override_cqe_ocs

[Kiwoong Kim]

Exynos host reports OCS_ABORT when a command is nullifed
or cleaned up with MCQ enabled. I think the command in those
situations should be issued again, rather than fail, because
when some conditions that caused the nullification or cleaning up
disppears after recovery, the command could be processed.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/ufs/host/ufs-exynos.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 16ad352..7ff0e84 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1376,6 +1376,13 @@ static void exynos_ufs_fmp_resume(struct ufs_hba *hba)
 
 #endif /* !CONFIG_SCSI_UFS_CRYPTO */
 
+static enum utp_ocs exynos_ufs_override_cqe_ocs(enum utp_ocs ocs)
+{
+	if (ocs == OCS_ABORTED)
+		ocs = OCS_INVALID_COMMAND_STATUS;
+	return ocs;
+}
+
 static int exynos_ufs_init(struct ufs_hba *hba)
 {
 	struct device *dev = hba->dev;
@@ -1926,6 +1933,7 @@ static const struct ufs_hba_variant_ops ufs_hba_exynos_ops = {
 	.suspend			= exynos_ufs_suspend,
 	.resume				= exynos_ufs_resume,
 	.fill_crypto_prdt		= exynos_ufs_fmp_fill_prdt,
+	.override_cqe_ocs		= exynos_ufs_override_cqe_ocs,
 };
 
 static struct ufs_hba_variant_ops ufs_hba_exynosauto_vh_ops = {
-- 
2.7.4

Re: [PATCH v2 0/2] scsi: ufs: introduce a callback to override OCS value

[Bean Huo]

On Thu, 2024-08-22 at 20:15 +0900, Kiwoong Kim wrote:
> UFSHCI defines OCS values but doesn't specify what exact
> conditions raise them. So I think it needs another callback
> to replace the original OCS value with the value that works
> the way you want.
> 
> v1 -> v2: fix build error for arguments
> 
> Kiwoong Kim (2):
>   scsi: ufs: core: introduce override_cqe_ocs
>   scsi: ufs: ufs-exynos: implement override_cqe_ocs

Hi kiwoong Kim,

I didn't see your above two patches following your cover-letter, did
you send patch with "--thread" optioin?


Kind regards,
Bean

> 
>  drivers/ufs/core/ufshcd-priv.h |  9 +++++++++
>  drivers/ufs/core/ufshcd.c      | 11 +++++++----
>  drivers/ufs/host/ufs-exynos.c  |  8 ++++++++
>  include/ufs/ufshcd.h           |  1 +
>  4 files changed, 25 insertions(+), 4 deletions(-)
> 

Re: [PATCH v2 0/2] scsi: ufs: introduce a callback to override OCS value

[Bean Huo]

On Thu, 2024-08-22 at 20:15 +0900, Kiwoong Kim wrote:
> Kiwoong Kim (2):
>   scsi: ufs: core: introduce override_cqe_ocs
>   scsi: ufs: ufs-exynos: implement override_cqe_ocs


Hi Kiwoong Kim,

I didn't see your patch email,just post your second patch here, and
provide my comments:

 
+static enum utp_ocs exynos_ufs_override_cqe_ocs(enum utp_ocs ocs)
+{
+	if (ocs == OCS_ABORTED)
+		ocs = OCS_INVALID_COMMAND_STATUS;
+	return ocs;
+}


I wonder if you have considered the case where the command is aborted
by the host software or by the device itself?

If you change OCS to OCS_INVALID_COMMAND_STATUS, there will report a
DID_REQUEUE to SCSI.


Kind regards,
Bean

Re: [PATCH v2 0/2] scsi: ufs: introduce a callback to override OCS value

[Bart Van Assche]

On 8/22/24 7:54 AM, Bean Huo wrote:
> I didn't see your above two patches following your cover-letter, did
> you send patch with "--thread" optioin?

I haven't received these patches either but found them here:

https://lore.kernel.org/linux-scsi/763ab716ba0207ecdad6f55ce38edf2d1bc7d04b.1724325280.git.kwmad.kim@samsung.com/

Bart.

Re: [PATCH v2 0/2] scsi: ufs: introduce a callback to override OCS value

[Bart Van Assche]

On 8/22/24 8:42 AM, Bean Huo wrote:
> On Thu, 2024-08-22 at 20:15 +0900, Kiwoong Kim wrote:
>> Kiwoong Kim (2):
>>    scsi: ufs: core: introduce override_cqe_ocs
>>    scsi: ufs: ufs-exynos: implement override_cqe_ocs
> 
> 
> Hi Kiwoong Kim,
> 
> I didn't see your patch email,just post your second patch here, and
> provide my comments:
> 
>   
> +static enum utp_ocs exynos_ufs_override_cqe_ocs(enum utp_ocs ocs)
> +{
> +	if (ocs == OCS_ABORTED)
> +		ocs = OCS_INVALID_COMMAND_STATUS;
> +	return ocs;
> +}
> 
> 
> I wonder if you have considered the case where the command is aborted
> by the host software or by the device itself?
> 
> If you change OCS to OCS_INVALID_COMMAND_STATUS, there will report a
> DID_REQUEUE to SCSI.

The decision about what to do probably should depend on whether or not
the command has been nullified.

Thanks,

Bart.

Re: [PATCH v2 1/2] scsi: ufs: core: introduce override_cqe_ocs

[kernel test robot]

Hi Kiwoong,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jejb-scsi/for-next]
[also build test WARNING on mkp-scsi/for-next krzk/for-next linus/master v6.11-rc5 next-20240826]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kiwoong-Kim/scsi-ufs-ufs-exynos-implement-override_cqe_ocs/20240826-111954
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
patch link:    https://lore.kernel.org/r/6aaeaa5fd70f76a1ac751ae3c5a3f0e37bc697b1.1724325280.git.kwmad.kim%40samsung.com
patch subject: [PATCH v2 1/2] scsi: ufs: core: introduce override_cqe_ocs
config: i386-buildonly-randconfig-002-20240826 (https://download.01.org/0day-ci/archive/20240826/202408262058.4pPayDSg-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240826/202408262058.4pPayDSg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408262058.4pPayDSg-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/ufs/core/ufshcd.c:827: warning: Function parameter or struct member 'hba' not described in 'ufshcd_get_tr_ocs'


vim +827 drivers/ufs/core/ufshcd.c

7a3e97b0dc4bba drivers/scsi/ufs/ufshcd.c Santosh Yaraganavi 2012-02-29  814  
7a3e97b0dc4bba drivers/scsi/ufs/ufshcd.c Santosh Yaraganavi 2012-02-29  815  /**
7a3e97b0dc4bba drivers/scsi/ufs/ufshcd.c Santosh Yaraganavi 2012-02-29  816   * ufshcd_get_tr_ocs - Get the UTRD Overall Command Status
8aa29f192ca675 drivers/scsi/ufs/ufshcd.c Bart Van Assche    2018-03-01  817   * @lrbp: pointer to local command reference block
c30d8d010b5efd drivers/ufs/core/ufshcd.c Asutosh Das        2023-01-13  818   * @cqe: pointer to the completion queue entry
7a3e97b0dc4bba drivers/scsi/ufs/ufshcd.c Santosh Yaraganavi 2012-02-29  819   *
7a3e97b0dc4bba drivers/scsi/ufs/ufshcd.c Santosh Yaraganavi 2012-02-29  820   * This function is used to get the OCS field from UTRD
3a17fefe0f1960 drivers/ufs/core/ufshcd.c Bart Van Assche    2023-07-27  821   *
3a17fefe0f1960 drivers/ufs/core/ufshcd.c Bart Van Assche    2023-07-27  822   * Return: the OCS field in the UTRD.
7a3e97b0dc4bba drivers/scsi/ufs/ufshcd.c Santosh Yaraganavi 2012-02-29  823   */
f214402c84a246 drivers/ufs/core/ufshcd.c Kiwoong Kim        2024-08-22  824  static enum utp_ocs ufshcd_get_tr_ocs(struct ufs_hba *hba,
f214402c84a246 drivers/ufs/core/ufshcd.c Kiwoong Kim        2024-08-22  825  				      struct ufshcd_lrb *lrbp,
c30d8d010b5efd drivers/ufs/core/ufshcd.c Asutosh Das        2023-01-13  826  				      struct cq_entry *cqe)
7a3e97b0dc4bba drivers/scsi/ufs/ufshcd.c Santosh Yaraganavi 2012-02-29 @827  {
c30d8d010b5efd drivers/ufs/core/ufshcd.c Asutosh Das        2023-01-13  828  	if (cqe)
f214402c84a246 drivers/ufs/core/ufshcd.c Kiwoong Kim        2024-08-22  829  		return ufshcd_vops_override_cqe_ocs(hba,
f214402c84a246 drivers/ufs/core/ufshcd.c Kiwoong Kim        2024-08-22  830  						    le32_to_cpu(cqe->status) &
f214402c84a246 drivers/ufs/core/ufshcd.c Kiwoong Kim        2024-08-22  831  						    MASK_OCS);
c30d8d010b5efd drivers/ufs/core/ufshcd.c Asutosh Das        2023-01-13  832  
67a2a8973832cb drivers/ufs/core/ufshcd.c Bart Van Assche    2023-07-27  833  	return lrbp->utr_descriptor_ptr->header.ocs & MASK_OCS;
7a3e97b0dc4bba drivers/scsi/ufs/ufshcd.c Santosh Yaraganavi 2012-02-29  834  }
7a3e97b0dc4bba drivers/scsi/ufs/ufshcd.c Santosh Yaraganavi 2012-02-29  835  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH v2 0/2] scsi: ufs: introduce a callback to override OCS value

[김기웅]

> +static enum utp_ocs exynos_ufs_override_cqe_ocs(enum utp_ocs ocs) {
> +	if (ocs == OCS_ABORTED)
> +		ocs = OCS_INVALID_COMMAND_STATUS;
> +	return ocs;
> +}
> 
> 
> I wonder if you have considered the case where the command is aborted
> by the host software or by the device itself?

I mean by the host software and Exynos host reports OCS_ABORTED only for the case when MCQ is enabled.

> 
> If you change OCS to OCS_INVALID_COMMAND_STATUS, there will report a
> DID_REQUEUE to SCSI.

Yes. That's what I meant, in order to process them properly after UFS initialization.

> 
> 
> Kind regards,
> Bean

Thank you.

RE: [PATCH v2 0/2] scsi: ufs: introduce a callback to override OCS value

[Kiwoong Kim]

> https://lore.kernel.org/linux-
> scsi/763ab716ba0207ecdad6f55ce38edf2d1bc7d04b.1724325280.git.kwmad.kim@sam
> sung.com/
> 
> Bart.

I included you but sending the patch set was blocked.
Sorry. Let me fix this as soon as possible.

Thank you.

RE: [PATCH v2 0/2] scsi: ufs: introduce a callback to override OCS value

[Kiwoong Kim]

> > I wonder if you have considered the case where the command is aborted
> > by the host software or by the device itself?
> >
> > If you change OCS to OCS_INVALID_COMMAND_STATUS, there will report a
> > DID_REQUEUE to SCSI.
> 
> The decision about what to do probably should depend on whether or not
> the command has been nullified.
> 
> Thanks,
> 
> Bart.

When MCQ is enabled, Exynos host reports OCS_ABORTED only for nullified cases.

Thanks.