[PATCH v4 0/5] scsi: ufs: cleanup ufs initialization

[PATCH v4 0/5] scsi: ufs: cleanup ufs initialization

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Cleanup UFS descriptor length initialization, and delete some unnecessary code.

Changelog:

v3 - v4:
    1. add desc_id >= QUERY_DESC_IDN_MAX check in patch 4/5 (Avri Altman)
    2. update buff_len to hold the true descriptor size in 4/5 (Avri Altman)
    3. add new patch 3/5

v2 - v3:
    1. Fix typo in the commit message (Avri Altman & Bart van Assche)
    2. Delete ufshcd_init_desc_sizes() in patch 3/4 (Stanley Chu)
    3. Remove max_t() and buff_len in patch 1/4 (Bart van Assche)
    4. Add patch 4/4 to compatable with 3.1 UFS unit descriptor length

v1 - v2:
    1. split patch
    2. fix one compiling WARNING (Reported-by: kbuild test robot <lkp@intel.com>)

Bean Huo (5):
  scsi: ufs: remove max_t in ufs_get_device_desc
  scsi: ufs: delete ufshcd_read_desc()
  scsi: ufs: fix potential access NULL pointer while memcpy
  scsi: ufs: cleanup ufs initialization path
  scsi: ufs: add compatibility with 3.1 UFS unit descriptor length

 drivers/scsi/ufs/ufs.h     |  11 +-
 drivers/scsi/ufs/ufs_bsg.c |   5 +-
 drivers/scsi/ufs/ufshcd.c  | 205 +++++++++----------------------------
 drivers/scsi/ufs/ufshcd.h  |  16 +--
 4 files changed, 53 insertions(+), 184 deletions(-)

-- 
2.17.1

[PATCH v4 1/5] scsi: ufs: remove max_t in ufs_get_device_desc

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

For the UFS device, the maximum descriptor size is 255, max_t called
in ufs_get_device_desc() is useless.

Signed-off-by: Bart van Assche <bvanassche@acm.org>
Signed-off-by: Bean Huo <beanhuo@micron.com>
Acked-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index aca50ed39844..f57acfbf9d60 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6876,14 +6876,11 @@ static void ufs_fixup_device_setup(struct ufs_hba *hba)
 static int ufs_get_device_desc(struct ufs_hba *hba)
 {
 	int err;
-	size_t buff_len;
 	u8 model_index;
 	u8 *desc_buf;
 	struct ufs_dev_info *dev_info = &hba->dev_info;
 
-	buff_len = max_t(size_t, hba->desc_size.dev_desc,
-			 QUERY_DESC_MAX_SIZE + 1);
-	desc_buf = kmalloc(buff_len, GFP_KERNEL);
+	desc_buf = kmalloc(QUERY_DESC_MAX_SIZE, GFP_KERNEL);
 	if (!desc_buf) {
 		err = -ENOMEM;
 		goto out;
-- 
2.17.1

[PATCH v4 2/5] scsi: ufs: delete ufshcd_read_desc()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Delete ufshcd_read_desc(). Instead, let caller directly call
ufshcd_read_desc_param().

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bart van Assche <bvanassche@acm.org>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f57acfbf9d60..f7e8bfefe3d4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3221,16 +3221,6 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 	return ret;
 }
 
-static inline int ufshcd_read_desc(struct ufs_hba *hba,
-				   enum desc_idn desc_id,
-				   int desc_index,
-				   void *buf,
-				   u32 size)
-{
-	return ufshcd_read_desc_param(hba, desc_id, desc_index, 0, buf, size);
-}
-
-
 /**
  * struct uc_string_id - unicode string
  *
@@ -3278,9 +3268,8 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
 	if (!uc_str)
 		return -ENOMEM;
 
-	ret = ufshcd_read_desc(hba, QUERY_DESC_IDN_STRING,
-			       desc_index, uc_str,
-			       QUERY_DESC_MAX_SIZE);
+	ret = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_STRING, desc_index, 0,
+				     (u8 *)uc_str, QUERY_DESC_MAX_SIZE);
 	if (ret < 0) {
 		dev_err(hba->dev, "Reading String Desc failed after %d retries. err = %d\n",
 			QUERY_REQ_RETRIES, ret);
@@ -6684,8 +6673,8 @@ static void ufshcd_set_active_icc_lvl(struct ufs_hba *hba)
 	if (!desc_buf)
 		return;
 
-	ret = ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0,
-			desc_buf, buff_len);
+	ret = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_POWER, 0, 0,
+				     desc_buf, buff_len);
 	if (ret) {
 		dev_err(hba->dev,
 			"%s: Failed reading power descriptor.len = %d ret = %d",
@@ -6886,8 +6875,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 		goto out;
 	}
 
-	err = ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, desc_buf,
-			hba->desc_size.dev_desc);
+	err = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_DEVICE, 0, 0, desc_buf,
+				     hba->desc_size.dev_desc);
 	if (err) {
 		dev_err(hba->dev, "%s: Failed reading Device Desc. err = %d\n",
 			__func__, err);
@@ -7168,8 +7157,8 @@ static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
 		goto out;
 	}
 
-	err = ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0,
-			desc_buf, buff_len);
+	err = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_GEOMETRY, 0, 0,
+				     desc_buf, buff_len);
 	if (err) {
 		dev_err(hba->dev, "%s: Failed reading Geometry Desc. err = %d\n",
 				__func__, err);
-- 
2.17.1

[PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer while memcpy

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

If param_offset is not 0, the memcpy length shouldn't be the
true descriptor length.

Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out ufshcd_read_desc_param")
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f7e8bfefe3d4..bc52a0e89cd3 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3211,7 +3211,7 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 
 	/* Check wherher we will not copy more data, than available */
 	if (is_kmalloc && param_size > buff_len)
-		param_size = buff_len;
+		param_size = buff_len - param_offset;
 
 	if (is_kmalloc)
 		memcpy(param_read_buf, &desc_buf[param_offset], param_size);
-- 
2.17.1

[PATCH v4 4/5] scsi: ufs: cleanup ufs initialization path

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

At UFS initialization stage, to get the length of the descriptor,
ufshcd_read_desc_length() being called 6 times. Instead, we will
capture the descriptor size the first time  we'll read it.

Delete unnecessary redundant code, remove ufshcd_read_desc_length(),
ufshcd_init_desc_sizes(), and boost UFS initialization.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Bart van Assche <bvanassche@acm.org>
Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs.h     |  10 ---
 drivers/scsi/ufs/ufs_bsg.c |   5 +-
 drivers/scsi/ufs/ufshcd.c  | 168 ++++++++-----------------------------
 drivers/scsi/ufs/ufshcd.h  |  16 +---
 4 files changed, 38 insertions(+), 161 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index fadba3a3bbcd..6548ef102eb9 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -200,16 +200,6 @@ enum desc_header_offset {
 	QUERY_DESC_DESC_TYPE_OFFSET	= 0x01,
 };
 
-enum ufs_desc_def_size {
-	QUERY_DESC_DEVICE_DEF_SIZE		= 0x59,
-	QUERY_DESC_CONFIGURATION_DEF_SIZE	= 0x90,
-	QUERY_DESC_UNIT_DEF_SIZE		= 0x2D,
-	QUERY_DESC_INTERCONNECT_DEF_SIZE	= 0x06,
-	QUERY_DESC_GEOMETRY_DEF_SIZE		= 0x48,
-	QUERY_DESC_POWER_DEF_SIZE		= 0x62,
-	QUERY_DESC_HEALTH_DEF_SIZE		= 0x25,
-};
-
 /* Unit descriptor parameters offsets in bytes*/
 enum unit_desc_param {
 	UNIT_DESC_PARAM_LEN			= 0x0,
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 53dd87628cbe..27f54615ee84 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -11,13 +11,12 @@ static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
 {
 	int desc_size = be16_to_cpu(qr->length);
 	int desc_id = qr->idn;
-	int ret;
 
 	if (desc_size <= 0)
 		return -EINVAL;
 
-	ret = ufshcd_map_desc_id_to_length(hba, desc_id, desc_len);
-	if (ret || !*desc_len)
+	ufshcd_map_desc_id_to_length(hba, desc_id, desc_len);
+	if (!*desc_len)
 		return -EINVAL;
 
 	*desc_len = min_t(int, *desc_len, desc_size);
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index bc52a0e89cd3..45d1fef982a9 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3052,95 +3052,32 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
 	return err;
 }
 
-/**
- * ufshcd_read_desc_length - read the specified descriptor length from header
- * @hba: Pointer to adapter instance
- * @desc_id: descriptor idn value
- * @desc_index: descriptor index
- * @desc_length: pointer to variable to read the length of descriptor
- *
- * Return 0 in case of success, non-zero otherwise
- */
-static int ufshcd_read_desc_length(struct ufs_hba *hba,
-	enum desc_idn desc_id,
-	int desc_index,
-	int *desc_length)
-{
-	int ret;
-	u8 header[QUERY_DESC_HDR_SIZE];
-	int header_len = QUERY_DESC_HDR_SIZE;
-
-	if (desc_id >= QUERY_DESC_IDN_MAX)
-		return -EINVAL;
-
-	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
-					desc_id, desc_index, 0, header,
-					&header_len);
-
-	if (ret) {
-		dev_err(hba->dev, "%s: Failed to get descriptor header id %d",
-			__func__, desc_id);
-		return ret;
-	} else if (desc_id != header[QUERY_DESC_DESC_TYPE_OFFSET]) {
-		dev_warn(hba->dev, "%s: descriptor header id %d and desc_id %d mismatch",
-			__func__, header[QUERY_DESC_DESC_TYPE_OFFSET],
-			desc_id);
-		ret = -EINVAL;
-	}
-
-	*desc_length = header[QUERY_DESC_LENGTH_OFFSET];
-	return ret;
-
-}
-
 /**
  * ufshcd_map_desc_id_to_length - map descriptor IDN to its length
  * @hba: Pointer to adapter instance
  * @desc_id: descriptor idn value
  * @desc_len: mapped desc length (out)
- *
- * Return 0 in case of success, non-zero otherwise
  */
-int ufshcd_map_desc_id_to_length(struct ufs_hba *hba,
-	enum desc_idn desc_id, int *desc_len)
+void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
+				  int *desc_len)
 {
-	switch (desc_id) {
-	case QUERY_DESC_IDN_DEVICE:
-		*desc_len = hba->desc_size.dev_desc;
-		break;
-	case QUERY_DESC_IDN_POWER:
-		*desc_len = hba->desc_size.pwr_desc;
-		break;
-	case QUERY_DESC_IDN_GEOMETRY:
-		*desc_len = hba->desc_size.geom_desc;
-		break;
-	case QUERY_DESC_IDN_CONFIGURATION:
-		*desc_len = hba->desc_size.conf_desc;
-		break;
-	case QUERY_DESC_IDN_UNIT:
-		*desc_len = hba->desc_size.unit_desc;
-		break;
-	case QUERY_DESC_IDN_INTERCONNECT:
-		*desc_len = hba->desc_size.interc_desc;
-		break;
-	case QUERY_DESC_IDN_STRING:
-		*desc_len = QUERY_DESC_MAX_SIZE;
-		break;
-	case QUERY_DESC_IDN_HEALTH:
-		*desc_len = hba->desc_size.hlth_desc;
-		break;
-	case QUERY_DESC_IDN_RFU_0:
-	case QUERY_DESC_IDN_RFU_1:
-		*desc_len = 0;
-		break;
-	default:
+	if (desc_id >= QUERY_DESC_IDN_MAX || desc_id == QUERY_DESC_IDN_RFU_0 ||
+	    desc_id == QUERY_DESC_IDN_RFU_1)
 		*desc_len = 0;
-		return -EINVAL;
-	}
-	return 0;
+	else
+		*desc_len = hba->desc_size[desc_id];
 }
 EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
 
+static void ufshcd_update_desc_length(struct ufs_hba *hba,
+				      enum desc_idn desc_id,
+				      unsigned char desc_len)
+{
+	if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
+	    desc_id != QUERY_DESC_IDN_STRING)
+		hba->desc_size[desc_id] = desc_len;
+}
+
 /**
  * ufshcd_read_desc_param - read the specified descriptor parameter
  * @hba: Pointer to adapter instance
@@ -3168,16 +3105,11 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 	if (desc_id >= QUERY_DESC_IDN_MAX || !param_size)
 		return -EINVAL;
 
-	/* Get the max length of descriptor from structure filled up at probe
-	 * time.
-	 */
-	ret = ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
-
-	/* Sanity checks */
-	if (ret || !buff_len) {
-		dev_err(hba->dev, "%s: Failed to get full descriptor length",
-			__func__);
-		return ret;
+	/* Get the length of descriptor */
+	ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
+	if (!buff_len) {
+		dev_err(hba->dev, "%s: Failed to get desc length", __func__);
+		return -EINVAL;
 	}
 
 	/* Check whether we need temp memory */
@@ -3209,6 +3141,10 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 		goto out;
 	}
 
+	/* Update descriptor length */
+	buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];
+	ufshcd_update_desc_length(hba, desc_id, buff_len);
+
 	/* Check wherher we will not copy more data, than available */
 	if (is_kmalloc && param_size > buff_len)
 		param_size = buff_len - param_offset;
@@ -6665,7 +6601,7 @@ static u32 ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
 static void ufshcd_set_active_icc_lvl(struct ufs_hba *hba)
 {
 	int ret;
-	int buff_len = hba->desc_size.pwr_desc;
+	int buff_len = hba->desc_size[QUERY_DESC_IDN_POWER];
 	u8 *desc_buf;
 	u32 icc_level;
 
@@ -6783,7 +6719,8 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
 	if (!ufshcd_is_wb_allowed(hba))
 		return;
 
-	if (hba->desc_size.dev_desc < DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
+	if (hba->desc_size[QUERY_DESC_IDN_DEVICE] <
+	    DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
 		goto wb_disabled;
 
 	hba->dev_info.d_ext_ufs_feature_sup =
@@ -6876,7 +6813,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
 	}
 
 	err = ufshcd_read_desc_param(hba, QUERY_DESC_IDN_DEVICE, 0, 0, desc_buf,
-				     hba->desc_size.dev_desc);
+				     hba->desc_size[QUERY_DESC_IDN_DEVICE]);
 	if (err) {
 		dev_err(hba->dev, "%s: Failed reading Device Desc. err = %d\n",
 			__func__, err);
@@ -7104,53 +7041,13 @@ static void ufshcd_clear_dbg_ufs_stats(struct ufs_hba *hba)
 	hba->req_abort_count = 0;
 }
 
-static void ufshcd_init_desc_sizes(struct ufs_hba *hba)
-{
-	int err;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_DEVICE, 0,
-		&hba->desc_size.dev_desc);
-	if (err)
-		hba->desc_size.dev_desc = QUERY_DESC_DEVICE_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_POWER, 0,
-		&hba->desc_size.pwr_desc);
-	if (err)
-		hba->desc_size.pwr_desc = QUERY_DESC_POWER_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_INTERCONNECT, 0,
-		&hba->desc_size.interc_desc);
-	if (err)
-		hba->desc_size.interc_desc = QUERY_DESC_INTERCONNECT_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_CONFIGURATION, 0,
-		&hba->desc_size.conf_desc);
-	if (err)
-		hba->desc_size.conf_desc = QUERY_DESC_CONFIGURATION_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_UNIT, 0,
-		&hba->desc_size.unit_desc);
-	if (err)
-		hba->desc_size.unit_desc = QUERY_DESC_UNIT_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_GEOMETRY, 0,
-		&hba->desc_size.geom_desc);
-	if (err)
-		hba->desc_size.geom_desc = QUERY_DESC_GEOMETRY_DEF_SIZE;
-
-	err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_HEALTH, 0,
-		&hba->desc_size.hlth_desc);
-	if (err)
-		hba->desc_size.hlth_desc = QUERY_DESC_HEALTH_DEF_SIZE;
-}
-
 static int ufshcd_device_geo_params_init(struct ufs_hba *hba)
 {
 	int err;
 	size_t buff_len;
 	u8 *desc_buf;
 
-	buff_len = hba->desc_size.geom_desc;
+	buff_len = hba->desc_size[QUERY_DESC_IDN_GEOMETRY];
 	desc_buf = kmalloc(buff_len, GFP_KERNEL);
 	if (!desc_buf) {
 		err = -ENOMEM;
@@ -7246,13 +7143,14 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
 static int ufshcd_device_params_init(struct ufs_hba *hba)
 {
 	bool flag;
-	int ret;
+	int ret, i;
 
 	/* Clear any previous UFS device information */
 	memset(&hba->dev_info, 0, sizeof(hba->dev_info));
 
-	/* Init check for device descriptor sizes */
-	ufshcd_init_desc_sizes(hba);
+	/* Init device descriptor sizes */
+	for (i = 0; i < QUERY_DESC_IDN_MAX; i++)
+		hba->desc_size[i] = QUERY_DESC_MAX_SIZE;
 
 	/* Init UFS geometry descriptor related parameters */
 	ret = ufshcd_device_geo_params_init(hba);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e3dfb48e669e..5ea090d82ddc 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -236,16 +236,6 @@ struct ufs_dev_cmd {
 	struct ufs_query query;
 };
 
-struct ufs_desc_size {
-	int dev_desc;
-	int pwr_desc;
-	int geom_desc;
-	int interc_desc;
-	int unit_desc;
-	int conf_desc;
-	int hlth_desc;
-};
-
 /**
  * struct ufs_clk_info - UFS clock related info
  * @list: list headed by hba->clk_list_head
@@ -738,7 +728,7 @@ struct ufs_hba {
 	bool is_urgent_bkops_lvl_checked;
 
 	struct rw_semaphore clk_scaling_lock;
-	struct ufs_desc_size desc_size;
+	unsigned char desc_size[QUERY_DESC_IDN_MAX];
 	atomic_t scsi_block_reqs_cnt;
 
 	struct device		bsg_dev;
@@ -975,8 +965,8 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index,
 int ufshcd_hold(struct ufs_hba *hba, bool async);
 void ufshcd_release(struct ufs_hba *hba);
 
-int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
-	int *desc_length);
+void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
+				  int *desc_length);
 
 u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba);
 
-- 
2.17.1

[PATCH v4 5/5] scsi: ufs: add compatibility with 3.1 UFS unit descriptor length

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

For UFS 3.1, the normal unit descriptor is 10 bytes larger
than the RPMB unit, however, both descriptors share the same
desc_idn, to cover both unit descriptors with one length, we
choose the normal unit descriptor length by desc_index.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufs.h    |  1 +
 drivers/scsi/ufs/ufshcd.c | 11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 6548ef102eb9..332ae09e6238 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -63,6 +63,7 @@
 #define UFS_UPIU_MAX_UNIT_NUM_ID	0x7F
 #define UFS_MAX_LUNS		(SCSI_W_LUN_BASE + UFS_UPIU_MAX_UNIT_NUM_ID)
 #define UFS_UPIU_WLUN_ID	(1 << 7)
+#define UFS_RPMB_UNIT		0xC4
 
 /* WriteBooster buffer is available only for the logical unit from 0 to 7 */
 #define UFS_UPIU_MAX_WB_LUN_ID	8
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 45d1fef982a9..a64718bfe2bc 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3070,11 +3070,16 @@ void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
 EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
 
 static void ufshcd_update_desc_length(struct ufs_hba *hba,
-				      enum desc_idn desc_id,
+				      enum desc_idn desc_id, int desc_index,
 				      unsigned char desc_len)
 {
 	if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
-	    desc_id != QUERY_DESC_IDN_STRING)
+	    desc_id != QUERY_DESC_IDN_STRING && desc_index != UFS_RPMB_UNIT)
+		/* For UFS 3.1, the normal unit descriptor is 10 bytes larger
+		 * than the RPMB unit, however, both descriptors share the same
+		 * desc_idn, to cover both unit descriptors with one length, we
+		 * choose the normal unit descriptor length by desc_index.
+		 */
 		hba->desc_size[desc_id] = desc_len;
 }
 
@@ -3143,7 +3148,7 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 
 	/* Update descriptor length */
 	buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];
-	ufshcd_update_desc_length(hba, desc_id, buff_len);
+	ufshcd_update_desc_length(hba, desc_id, desc_index, buff_len);
 
 	/* Check wherher we will not copy more data, than available */
 	if (is_kmalloc && param_size > buff_len)
-- 
2.17.1

RE: [PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer while memcpy

[Avri Altman]

Hi,

> If param_offset is not 0, the memcpy length shouldn't be the
> true descriptor length.
> 
> Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out ufshcd_read_desc_param")
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f7e8bfefe3d4..bc52a0e89cd3 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3211,7 +3211,7 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
> 
>         /* Check wherher we will not copy more data, than available */
>         if (is_kmalloc && param_size > buff_len)
> -               param_size = buff_len;
> +               param_size = buff_len - param_offset;
But Is_kmalloc is true if (param_offset != 0 || param_size < buff_len)
So  if (is_kmalloc && param_size > buff_len) implies that param_offset is 0,
Or did I get it wrong?

Still, I think that there is a problem here because nowhere we are checking that  
param_offset + param_size < buff_len, which now can happen because of ufs-bsg.

Maybe you can add it and get rid of that is_kmalloc which is an awkward way to test for valid values?

Thanks,
Avri
> 
>         if (is_kmalloc)
>                 memcpy(param_read_buf, &desc_buf[param_offset], param_size);
> --
> 2.17.1

Re: [PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer while memcpy

[Bean Huo]

hi Avri
thanks review.


On Mon, 2020-06-01 at 06:25 +0000, Avri Altman wrote:
> Hi,
> 
> > If param_offset is not 0, the memcpy length shouldn't be the
> > true descriptor length.
> > 
> > Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out
> > ufshcd_read_desc_param")
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index f7e8bfefe3d4..bc52a0e89cd3 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -3211,7 +3211,7 @@ int ufshcd_read_desc_param(struct ufs_hba
> > *hba,
> > 
> >         /* Check wherher we will not copy more data, than available
> > */
> >         if (is_kmalloc && param_size > buff_len)
> > -               param_size = buff_len;
> > +               param_size = buff_len - param_offset;
> 
> But Is_kmalloc is true if (param_offset != 0 || param_size <
> buff_len)
> So  if (is_kmalloc && param_size > buff_len) implies that
> param_offset is 0,
> Or did I get it wrong?

If param_offset is 0, This willn't get any wrong, after this patch, it
is the same since offset is 0. As mentioned in the commit message, this
patch is only for the case of param_offset is not 0.

> 
> Still, I think that there is a problem here because nowhere we are
> checking that  
> param_offset + param_size < buff_len, which now can happen because of
> ufs-bsg.
> Maybe you can add it and get rid of that is_kmalloc which is an
> awkward way to test for valid values?

let me explain further:
we have these conditinos:

1) param_offset == 0, param_size >= buff_len;//no problem, 
ufshcd_query_descriptor_retry() will read descripor with true
descriptor length, and no memcpy() called.


2) param_offset == 0, param_size < buff_len;// no problem, 
ufshcd_query_descriptor_retry() will read descripor with true
descriptor length buff_len, and memcpy() with param_size length.


3) param_offset != 0, param_offset + param_size <= buff_len;// no
problem, ufshcd_query_descriptor_retry() will read descripor with true
descriptor length, and memcpy() with param_size length.


4) param_offset != 0, param_offset + param_size > buff_len;// NULL
pointer reference problem, since ufshcd_query_descriptor_retry() will
read descripor with true descriptor length, and memcpy() with buff_len
length. correct memcpy length should be (buff_len - param_offset)

param_offset + param_size < buff_len doesn't need to add, and
is_kmalloc is very hard to be removed based on current flow.

so, the correct fixup patch shoulbe be like this:


-if (is_kmalloc && param_size > buff_len)
-	param_size = buff_len
+if (is_kmalloc && (param_size + param_offset) > buff_len) 
+	param_size = buff_len - param_offset;


how do you think about it? if no problem, I will update it in next
version patch.

thanks,

Bean

RE: [PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer while memcpy

[Avri Altman]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

How about something like the untested attached?

Thanks,
Avri

> -----Original Message-----
> From: Bean Huo <huobean@gmail.com>
> Sent: Tuesday, June 2, 2020 2:36 PM
> To: Avri Altman <Avri.Altman@wdc.com>; alim.akhtar@samsung.com;
> asutoshd@codeaurora.org; jejb@linux.ibm.com;
> martin.petersen@oracle.com; stanley.chu@mediatek.com;
> beanhuo@micron.com; bvanassche@acm.org; tomas.winkler@intel.com;
> cang@codeaurora.org
> Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer while
> memcpy
> 
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
> 
> 
> hi Avri
> thanks review.
> 
> 
> On Mon, 2020-06-01 at 06:25 +0000, Avri Altman wrote:
> > Hi,
> >
> > > If param_offset is not 0, the memcpy length shouldn't be the
> > > true descriptor length.
> > >
> > > Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out
> > > ufshcd_read_desc_param")
> > > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > > ---
> > >  drivers/scsi/ufs/ufshcd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index f7e8bfefe3d4..bc52a0e89cd3 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -3211,7 +3211,7 @@ int ufshcd_read_desc_param(struct ufs_hba
> > > *hba,
> > >
> > >         /* Check wherher we will not copy more data, than available
> > > */
> > >         if (is_kmalloc && param_size > buff_len)
> > > -               param_size = buff_len;
> > > +               param_size = buff_len - param_offset;
> >
> > But Is_kmalloc is true if (param_offset != 0 || param_size <
> > buff_len)
> > So  if (is_kmalloc && param_size > buff_len) implies that
> > param_offset is 0,
> > Or did I get it wrong?
> 
> If param_offset is 0, This willn't get any wrong, after this patch, it
> is the same since offset is 0. As mentioned in the commit message, this
> patch is only for the case of param_offset is not 0.
> 
> >
> > Still, I think that there is a problem here because nowhere we are
> > checking that
> > param_offset + param_size < buff_len, which now can happen because of
> > ufs-bsg.
> > Maybe you can add it and get rid of that is_kmalloc which is an
> > awkward way to test for valid values?
> 
> let me explain further:
> we have these conditinos:
> 
> 1) param_offset == 0, param_size >= buff_len;//no problem,
> ufshcd_query_descriptor_retry() will read descripor with true
> descriptor length, and no memcpy() called.
> 
> 
> 2) param_offset == 0, param_size < buff_len;// no problem,
> ufshcd_query_descriptor_retry() will read descripor with true
> descriptor length buff_len, and memcpy() with param_size length.
> 
> 
> 3) param_offset != 0, param_offset + param_size <= buff_len;// no
> problem, ufshcd_query_descriptor_retry() will read descripor with true
> descriptor length, and memcpy() with param_size length.
> 
> 
> 4) param_offset != 0, param_offset + param_size > buff_len;// NULL
> pointer reference problem, since ufshcd_query_descriptor_retry() will
> read descripor with true descriptor length, and memcpy() with buff_len
> length. correct memcpy length should be (buff_len - param_offset)
> 
> param_offset + param_size < buff_len doesn't need to add, and
> is_kmalloc is very hard to be removed based on current flow.
> 
> so, the correct fixup patch shoulbe be like this:
> 
> 
> -if (is_kmalloc && param_size > buff_len)
> -       param_size = buff_len
> +if (is_kmalloc && (param_size + param_offset) > buff_len)
> +       param_size = buff_len - param_offset;
> 
> 
> how do you think about it? if no problem, I will update it in next
> version patch.
> 
> thanks,
> 
> Bean


[-- Attachment #2: 0001-scsi-ufshcd-Simplify-ufshcd_read_desc_param.patch --]
[-- Type: application/octet-stream, Size: 2782 bytes --]

From ccb21474171a2ffc67b7a229538288fd39891181 Mon Sep 17 00:00:00 2001
From: Avri Altman <avri.altman@wdc.com>
Date: Tue, 2 Jun 2020 15:14:56 +0300
Subject: [PATCH] scsi: ufshcd: Simplify ufshcd_read_desc_param

Always use a locally allocated buffer, and verify that the requested
parameters are indeed valid.

Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out ufshcd_read_desc_param")
Signed-off-by: Avri Altman <avri.altman@wdc.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index af14e52..f036c59 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3152,6 +3152,8 @@ EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
  * @param_size: sizeof(param_read_buf)
  *
  * Return 0 in case of success, non-zero otherwise
+ * The caller must ensure that param_read_buf points to at least param_size
+ * available bytes.
  */
 int ufshcd_read_desc_param(struct ufs_hba *hba,
 			   enum desc_idn desc_id,
@@ -3163,7 +3165,6 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 	int ret;
 	u8 *desc_buf;
 	int buff_len;
-	bool is_kmalloc = true;
 
 	/* Safety check */
 	if (desc_id >= QUERY_DESC_IDN_MAX || !param_size)
@@ -3175,26 +3176,19 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 	ret = ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
 
 	/* Sanity checks */
-	if (ret || !buff_len) {
+	if (ret || (param_offset + param_size >= buff_len)) {
 		dev_err(hba->dev, "%s: Failed to get full descriptor length",
 			__func__);
 		return ret;
 	}
 
-	/* Check whether we need temp memory */
-	if (param_offset != 0 || param_size < buff_len) {
-		desc_buf = kmalloc(buff_len, GFP_KERNEL);
-		if (!desc_buf)
-			return -ENOMEM;
-	} else {
-		desc_buf = param_read_buf;
-		is_kmalloc = false;
-	}
+	desc_buf = kmalloc(buff_len, GFP_KERNEL);
+	if (!desc_buf)
+		return -ENOMEM;
 
 	/* Request for full descriptor */
 	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
-					desc_id, desc_index, 0,
-					desc_buf, &buff_len);
+			desc_id, desc_index, 0, desc_buf, &buff_len);
 
 	if (ret) {
 		dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d",
@@ -3210,15 +3204,10 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 		goto out;
 	}
 
-	/* Check wherher we will not copy more data, than available */
-	if (is_kmalloc && param_size > buff_len)
-		param_size = buff_len;
+	memcpy(param_read_buf, &desc_buf[param_offset], param_size);
 
-	if (is_kmalloc)
-		memcpy(param_read_buf, &desc_buf[param_offset], param_size);
 out:
-	if (is_kmalloc)
-		kfree(desc_buf);
+	kfree(desc_buf);
 	return ret;
 }
 
-- 
2.7.4

RE: [PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer while memcpy

[Avri Altman]

But this is just a suggestion.
Your way is fine too.

Thanks,
Avri

> 
> How about something like the untested attached?
> 
> Thanks,
> Avri
> 
> > -----Original Message-----
> > From: Bean Huo <huobean@gmail.com>
> > Sent: Tuesday, June 2, 2020 2:36 PM
> > To: Avri Altman <Avri.Altman@wdc.com>; alim.akhtar@samsung.com;
> > asutoshd@codeaurora.org; jejb@linux.ibm.com;
> > martin.petersen@oracle.com; stanley.chu@mediatek.com;
> > beanhuo@micron.com; bvanassche@acm.org; tomas.winkler@intel.com;
> > cang@codeaurora.org
> > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer while
> > memcpy
> >
> > CAUTION: This email originated from outside of Western Digital. Do not click
> > on links or open attachments unless you recognize the sender and know
> that
> > the content is safe.
> >
> >
> > hi Avri
> > thanks review.
> >
> >
> > On Mon, 2020-06-01 at 06:25 +0000, Avri Altman wrote:
> > > Hi,
> > >
> > > > If param_offset is not 0, the memcpy length shouldn't be the
> > > > true descriptor length.
> > > >
> > > > Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out
> > > > ufshcd_read_desc_param")
> > > > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > > > ---
> > > >  drivers/scsi/ufs/ufshcd.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > > index f7e8bfefe3d4..bc52a0e89cd3 100644
> > > > --- a/drivers/scsi/ufs/ufshcd.c
> > > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > > @@ -3211,7 +3211,7 @@ int ufshcd_read_desc_param(struct ufs_hba
> > > > *hba,
> > > >
> > > >         /* Check wherher we will not copy more data, than available
> > > > */
> > > >         if (is_kmalloc && param_size > buff_len)
> > > > -               param_size = buff_len;
> > > > +               param_size = buff_len - param_offset;
> > >
> > > But Is_kmalloc is true if (param_offset != 0 || param_size <
> > > buff_len)
> > > So  if (is_kmalloc && param_size > buff_len) implies that
> > > param_offset is 0,
> > > Or did I get it wrong?
> >
> > If param_offset is 0, This willn't get any wrong, after this patch, it
> > is the same since offset is 0. As mentioned in the commit message, this
> > patch is only for the case of param_offset is not 0.
> >
> > >
> > > Still, I think that there is a problem here because nowhere we are
> > > checking that
> > > param_offset + param_size < buff_len, which now can happen because of
> > > ufs-bsg.
> > > Maybe you can add it and get rid of that is_kmalloc which is an
> > > awkward way to test for valid values?
> >
> > let me explain further:
> > we have these conditinos:
> >
> > 1) param_offset == 0, param_size >= buff_len;//no problem,
> > ufshcd_query_descriptor_retry() will read descripor with true
> > descriptor length, and no memcpy() called.
> >
> >
> > 2) param_offset == 0, param_size < buff_len;// no problem,
> > ufshcd_query_descriptor_retry() will read descripor with true
> > descriptor length buff_len, and memcpy() with param_size length.
> >
> >
> > 3) param_offset != 0, param_offset + param_size <= buff_len;// no
> > problem, ufshcd_query_descriptor_retry() will read descripor with true
> > descriptor length, and memcpy() with param_size length.
> >
> >
> > 4) param_offset != 0, param_offset + param_size > buff_len;// NULL
> > pointer reference problem, since ufshcd_query_descriptor_retry() will
> > read descripor with true descriptor length, and memcpy() with buff_len
> > length. correct memcpy length should be (buff_len - param_offset)
> >
> > param_offset + param_size < buff_len doesn't need to add, and
> > is_kmalloc is very hard to be removed based on current flow.
> >
> > so, the correct fixup patch shoulbe be like this:
> >
> >
> > -if (is_kmalloc && param_size > buff_len)
> > -       param_size = buff_len
> > +if (is_kmalloc && (param_size + param_offset) > buff_len)
> > +       param_size = buff_len - param_offset;
> >
> >
> > how do you think about it? if no problem, I will update it in next
> > version patch.
> >
> > thanks,
> >
> > Bean