[RFC PATCH v1 0/2] support various values per device

[RFC PATCH v1 0/2] support various values per device

[Kiwoong Kim]

Respective UFS devices have their own characteristics and
many of them could be a form of numbers, such as timeout
and a number of retires. This introduces the way to set
those things per specific device vendor or specific device.

I wrote this like the style of ufs_fixups stuffs.

Kiwoong Kim (2):
  ufs: support various values per device
  ufs: change the way to complete fDeviceInit

 drivers/scsi/ufs/ufs_quirks.h | 20 +++++++++++++
 drivers/scsi/ufs/ufshcd.c     | 70 +++++++++++++++++++++++++++++++++++++------
 drivers/scsi/ufs/ufshcd.h     |  1 +
 3 files changed, 82 insertions(+), 9 deletions(-)

-- 
2.7.4

[RFC PATCH v1 1/2] ufs: support various values per device

[Kiwoong Kim]

Respective UFS devices have their own characteristics and
many of them could be a form of numbers, such as timeout
and a number of retires. This introduces the way to set
those things per specific device vendor or specific device.

I wrote this like the style of ufs_fixups stuffs.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/scsi/ufs/ufs_quirks.h | 20 ++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.c     | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h     |  1 +
 3 files changed, 59 insertions(+)

diff --git a/drivers/scsi/ufs/ufs_quirks.h b/drivers/scsi/ufs/ufs_quirks.h
index 2a00414..9896d98 100644
--- a/drivers/scsi/ufs/ufs_quirks.h
+++ b/drivers/scsi/ufs/ufs_quirks.h
@@ -29,6 +29,19 @@ struct ufs_dev_fix {
 	unsigned int quirk;
 };
 
+enum dev_val_type {
+	DEV_VAL_FDEVICEINIT	= 0x0,
+	DEV_VAL_NUM,
+};
+
+struct ufs_dev_value {
+	u16 wmanufacturerid;
+	u8 *model;
+	u32 key;
+	u32 val;
+	bool enable;
+};
+
 #define END_FIX { }
 
 /* add specific device quirk */
@@ -38,6 +51,13 @@ struct ufs_dev_fix {
 	.quirk = (_quirk),		   \
 }
 
+#define UFS_DEV_VAL(_vendor, _model, _key, _val) { \
+	.wmanufacturerid = (_vendor),\
+	.model = (_model),		\
+	.key = (_key),			\
+	.val = (_val),			\
+}
+
 /*
  * Some vendor's UFS device sends back to back NACs for the DL data frames
  * causing the host controller to raise the DFES error status. Sometimes
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 52abe82..b6bc333 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -207,6 +207,21 @@ static struct ufs_dev_fix ufs_fixups[] = {
 	END_FIX
 };
 
+static struct ufs_dev_value ufs_dev_values[] = {
+	END_FIX
+};
+
+static inline bool
+ufs_get_dev_specific_value(struct ufs_hba *hba,
+			   enum dev_val_type type, u32 *val)
+{
+	if (!ufs_dev_values[type].enable)
+		return false;
+
+	*val = hba->dev_value[type];
+	return true;
+}
+
 static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba);
 static void ufshcd_async_scan(void *data, async_cookie_t cookie);
 static int ufshcd_reset_and_restore(struct ufs_hba *hba);
@@ -6923,11 +6938,34 @@ void ufshcd_fixup_dev_quirks(struct ufs_hba *hba, struct ufs_dev_fix *fixups)
 }
 EXPORT_SYMBOL_GPL(ufshcd_fixup_dev_quirks);
 
+void ufshcd_set_dev_values(struct ufs_hba *hba, struct ufs_dev_value *value)
+{
+	struct ufs_dev_value *f;
+	struct ufs_dev_info *dev_info = &hba->dev_info;
+
+	if (!value)
+		return;
+
+	for (f = value; f->val; f++) {
+		if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
+		     f->wmanufacturerid == UFS_ANY_VENDOR) &&
+		     ((dev_info->model &&
+		       STR_PRFX_EQUAL(f->model, dev_info->model)) ||
+		      !strcmp(f->model, UFS_ANY_MODEL))) {
+			f->enable = true;
+			hba->dev_value[f->key] = f->val;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(ufshcd_set_dev_values);
+
 static void ufs_fixup_device_setup(struct ufs_hba *hba)
 {
 	/* fix by general quirk table */
 	ufshcd_fixup_dev_quirks(hba, ufs_fixups);
 
+	ufshcd_set_dev_values(hba, ufs_dev_values);
+
 	/* allow vendors to fix quirks */
 	ufshcd_vops_fixup_dev_quirks(hba);
 }
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c774012..f221ca7 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -670,6 +670,7 @@ struct ufs_hba {
 
 	/* Device deviations from standard UFS device spec. */
 	unsigned int dev_quirks;
+	u32 dev_value[DEV_VAL_NUM];
 
 	struct blk_mq_tag_set tmf_tag_set;
 	struct request_queue *tmf_queue;
-- 
2.7.4

Re: [RFC PATCH v1 1/2] ufs: support various values per device

[Bart Van Assche]

On 2020-06-20 00:31, Kiwoong Kim wrote:
>  #define END_FIX { }

The name of this macro is longer than its value. Additionally, this
macro makes code harder to read instead of easier. Please include a
patch to remove this macro from the UFS driver.

> +#define UFS_DEV_VAL(_vendor, _model, _key, _val) { \
> +	.wmanufacturerid = (_vendor),\
> +	.model = (_model),		\
> +	.key = (_key),			\
> +	.val = (_val),			\
> +}

A macro like the above also makes code harder to read instead of easier.
Please remove this macro definition and use the designated
initialization style directly.

Thanks,

Bart.

RE: [RFC PATCH v1 1/2] ufs: support various values per device

[Kiwoong Kim]

> On 2020-06-20 00:31, Kiwoong Kim wrote:
> >  #define END_FIX { }
> 
> The name of this macro is longer than its value. Additionally, this macro
> makes code harder to read instead of easier. Please include a patch to
> remove this macro from the UFS driver.
> 
> > +#define UFS_DEV_VAL(_vendor, _model, _key, _val) { \
> > +	.wmanufacturerid = (_vendor),\
> > +	.model = (_model),		\
> > +	.key = (_key),			\
> > +	.val = (_val),			\
> > +}
> 
> A macro like the above also makes code harder to read instead of easier.
> Please remove this macro definition and use the designated initialization
> style directly.
> 
> Thanks,
> 
> Bart.

Got it. Thanks !

[RFC PATCH v1 2/2] ufs: change the way to complete fDeviceInit

[Kiwoong Kim]

Currently, UFS driver checks if fDeviceInit
is cleared at several times, not period. This patch
is to wait its completion with the period, not retrying.
Many device vendors usually provides the specification on
it with just period, not a combination of a number of retrying
and period. So it could be proper to regard to the information
coming from device vendors.

I first added one value regarding the information.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b6bc333..a1d85c6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -208,6 +208,8 @@ static struct ufs_dev_fix ufs_fixups[] = {
 };
 
 static struct ufs_dev_value ufs_dev_values[] = {
+	UFS_DEV_VAL(UFS_VENDOR_TOSHIBA, UFS_ANY_MODEL,
+			DEV_VAL_FDEVICEINIT, 2000),
 	END_FIX
 };
 
@@ -4162,9 +4164,12 @@ EXPORT_SYMBOL_GPL(ufshcd_config_pwr_mode);
  */
 static int ufshcd_complete_dev_init(struct ufs_hba *hba)
 {
-	int i;
+	u32 dev_init_compl_in_ms = 500;
+	unsigned long timeout;
 	int err;
 	bool flag_res = true;
+	bool is_dev_val;
+	u32 val;
 
 	err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
 		QUERY_FLAG_IDN_FDEVICEINIT, 0, NULL);
@@ -4175,19 +4180,28 @@ static int ufshcd_complete_dev_init(struct ufs_hba *hba)
 		goto out;
 	}
 
-	/* poll for max. 1000 iterations for fDeviceInit flag to clear */
-	for (i = 0; i < 1000 && !err && flag_res; i++)
-		err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
-			QUERY_FLAG_IDN_FDEVICEINIT, 0, &flag_res);
+	/* Poll fDeviceInit flag to be cleared */
+	is_dev_val = ufs_get_dev_specific_value(hba, DEV_VAL_FDEVICEINIT, &val);
+	dev_init_compl_in_ms = (is_dev_val) ? val : 500;
+	timeout = jiffies + msecs_to_jiffies(dev_init_compl_in_ms);
+	do {
+		err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,
+					QUERY_FLAG_IDN_FDEVICEINIT, 0, &flag_res);
+		if (!flag_res)
+			break;
+		usleep_range(1000, 2000);
+	} while (time_before(jiffies, timeout));
 
-	if (err)
+	if (err) {
 		dev_err(hba->dev,
-			"%s reading fDeviceInit flag failed with error %d\n",
-			__func__, err);
-	else if (flag_res)
+				"%s reading fDeviceInit flag failed with error %d\n",
+				__func__, err);
+	} else if (flag_res) {
 		dev_err(hba->dev,
 			"%s fDeviceInit was not cleared by the device\n",
 			__func__);
+		err = -EBUSY;
+	}
 
 out:
 	return err;
-- 
2.7.4

Re: [RFC PATCH v1 2/2] ufs: change the way to complete fDeviceInit

[Bart Van Assche]

On 2020-06-20 00:31, Kiwoong Kim wrote:
>  static struct ufs_dev_value ufs_dev_values[] = {
> +	UFS_DEV_VAL(UFS_VENDOR_TOSHIBA, UFS_ANY_MODEL,
> +			DEV_VAL_FDEVICEINIT, 2000),
>  	END_FIX
>  };

Can this array be declared 'const'?

Thanks,

Bart.

RE: [RFC PATCH v1 2/2] ufs: change the way to complete fDeviceInit

[Kiwoong Kim]

> On 2020-06-20 00:31, Kiwoong Kim wrote:
> >  static struct ufs_dev_value ufs_dev_values[] = {
> > +	UFS_DEV_VAL(UFS_VENDOR_TOSHIBA, UFS_ANY_MODEL,
> > +			DEV_VAL_FDEVICEINIT, 2000),
> >  	END_FIX
> >  };
> 
> Can this array be declared 'const'?
> 
> Thanks,
> 
> Bart.

Sure. Thanks !

RE: [RFC PATCH v1 0/2] support various values per device

[Avri Altman]

 
> 
> Respective UFS devices have their own characteristics and
> many of them could be a form of numbers, such as timeout
> and a number of retires. This introduces the way to set
> those things per specific device vendor or specific device.
> 
> I wrote this like the style of ufs_fixups stuffs.
Looks like your preparing the ground of making quirks a common practice...
Anyway, we already have that option per platform/vendor - 
what is that you are missing?

And as far as fDeviceInit  - the spec says:
"The host polls the fDeviceInit flag to check the completion of the process."
Your changes are align with that definition, so I am not sure that even a new quirk is needed?

Thanks,
Avri

RE: [RFC PATCH v1 0/2] support various values per device

[Kiwoong Kim]

> > Respective UFS devices have their own characteristics and many of them
> > could be a form of numbers, such as timeout and a number of retires.
> > This introduces the way to set those things per specific device vendor
> > or specific device.
> >
> > I wrote this like the style of ufs_fixups stuffs.
> Looks like your preparing the ground of making quirks a common practice...
> Anyway, we already have that option per platform/vendor - what is that you
> are missing?
> 
> And as far as fDeviceInit  - the spec says:
> "The host polls the fDeviceInit flag to check the completion of the
> process."
> Your changes are align with that definition, so I am not sure that even a
> new quirk is needed?
> 
> Thanks,
> Avri

I see what you're saying and thank you for your opinion.

But I don’t think what the spec mentions fits in operating system all the time.
In fact, current driver doesn't work exactly the same with the description,
since current driver somehow terminates the waiting after a series of retrying.
Besides, I'm not sure if waiting something infinitely is great for Linux kernel,
Many are suffering from things like that point in the mobile industry
because that makes harder to figure out what's happening. Someone could tell
that kernel log shows it but in many development environments, it's difficult to 
check and occurring errors for timed-out is beneficial, I think.

That's why errors for not completing something have been introduced in some domains,
possibly even some architectures which don't define that.