[PATCH V4 1/3] scsi: ufs: set the device reference clock setting

[PATCH V4 1/3] scsi: ufs: set the device reference clock setting

[Sayali Lokhande]

From: Subhash Jadavani <subhashj@codeaurora.org>

UFS host supplies the reference clock to UFS device and UFS device
specification allows host to provide one of the 4 frequencies (19.2 MHz,
26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the
device reference clock frequency setting in the device based on what
frequency it is supplying to UFS device.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
---
 .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      |  7 +++
 drivers/scsi/ufs/ufs.h                             |  9 +++
 drivers/scsi/ufs/ufshcd-pltfrm.c                   |  2 +
 drivers/scsi/ufs/ufshcd.c                          | 72 ++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h                          |  2 +
 5 files changed, 92 insertions(+)

diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
index c39dfef..4522434 100644
--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
@@ -41,6 +41,12 @@ Optional properties:
 -lanes-per-direction	: number of lanes available per direction - either 1 or 2.
 			  Note that it is assume same number of lanes is used both
 			  directions at once. If not specified, default is 2 lanes per direction.
+- dev-ref-clk-freq	: Specify the device reference clock frequency, must be one of the following:
+			  0: 19.2 MHz
+			  1: 26 MHz
+			  2: 38.4 MHz
+			  3: 52 MHz
+			  Defaults to 26 MHz if not specified.
 
 Note: If above properties are not defined it can be assumed that the supply
 regulators or clocks are always on.
@@ -66,4 +72,5 @@ Example:
 		freq-table-hz = <100000000 200000000>, <0 0>, <0 0>;
 		phys = <&ufsphy1>;
 		phy-names = "ufsphy";
+		dev-ref-clk-freq = <0>; /* reference clock freq: 19.2 MHz */
 	};
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 14e5bf7..e15deb0 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -378,6 +378,15 @@ enum query_opcode {
 	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
 };
 
+/* bRefClkFreq attribute values */
+enum ref_clk_freq {
+	REF_CLK_FREQ_19_2_MHZ	= 0x0,
+	REF_CLK_FREQ_26_MHZ	= 0x1,
+	REF_CLK_FREQ_38_4_MHZ	= 0x2,
+	REF_CLK_FREQ_52_MHZ	= 0x3,
+	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
+};
+
 /* Query response result code */
 enum {
 	QUERY_RESULT_SUCCESS                    = 0x00,
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index e82bde0..0953563 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -343,6 +343,8 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
+	ufshcd_parse_dev_ref_clk_freq(hba);
+
 	ufshcd_init_lanes_per_dir(hba);
 
 	err = ufshcd_init(hba, mmio_base, irq);
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c5b1bf1..351f874 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6296,6 +6296,72 @@ static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
 	hba->desc_size.hlth_desc = QUERY_DESC_HEALTH_DEF_SIZE;
 }
 
+void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	int ret;
+
+	if (!dev)
+		return;
+
+	ret = device_property_read_u32(dev, "dev-ref-clk-freq",
+				   &hba->dev_ref_clk_freq);
+	if (ret ||
+	    (hba->dev_ref_clk_freq > REF_CLK_FREQ_52_MHZ)) {
+		dev_err(hba->dev,
+		"%s: invalid ref_clk setting = %d\n",
+		__func__, hba->dev_ref_clk_freq);
+		hba->dev_ref_clk_freq = REF_CLK_FREQ_MAX + 1;
+	}
+}
+
+/**
+ * ufshcd_set_dev_ref_clk - set the device bRefClkFreq
+ * @hba: per-adapter instance
+ *
+ * Read the current value of the bRefClkFreq attribute from device and update it
+ * if host is supplying different reference clock frequency than one mentioned
+ * in bRefClkFreq attribute.
+ *
+ * Returns zero on success, non-zero error value on failure.
+ */
+static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
+{
+	int err = 0;
+	int ref_clk = -1;
+	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
+						     "38.4 MHz", "52 MHz"};
+
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
+
+	if (err) {
+		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
+			 __func__, err);
+		goto out;
+	}
+
+	if (ref_clk == hba->dev_ref_clk_freq)
+		goto out; /* nothing to update */
+
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
+			&hba->dev_ref_clk_freq);
+
+	if (err)
+		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
+			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
+	/*
+	 * It is good to print this out here to debug any later failures
+	 * related to gear switch.
+	 */
+	dev_dbg(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
+			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
+
+out:
+	return err;
+}
+
 /**
  * ufshcd_probe_hba - probe hba to detect device and initialize
  * @hba: per-adapter instance
@@ -6361,6 +6427,12 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 			"%s: Failed getting max supported power mode\n",
 			__func__);
 	} else {
+		/*
+		 * Set the right value to bRefClkFreq before attempting to
+		 * switch to HS gears.
+		 */
+		if (hba->dev_ref_clk_freq < REF_CLK_FREQ_MAX + 1)
+			ufshcd_set_dev_ref_clk(hba);
 		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
 		if (ret) {
 			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n",
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 8110dcd..101a75c 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -548,6 +548,7 @@ struct ufs_hba {
 	void *priv;
 	unsigned int irq;
 	bool is_irq_enabled;
+	u32 dev_ref_clk_freq;
 
 	/* Interrupt aggregation support is broken */
 	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
@@ -746,6 +747,7 @@ static inline void ufshcd_rmwl(struct ufs_hba *hba, u32 mask, u32 val, u32 reg)
 int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
 				u32 val, unsigned long interval_us,
 				unsigned long timeout_ms, bool can_sleep);
+void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba);
 
 static inline void check_upiu_size(void)
 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH V4 2/3] scsi: ufs: Add ufs provisioning support

[Sayali Lokhande]

A new api ufshcd_do_config_device() is added in driver
to support UFS provisioning at runtime. Configfs support
is added to trigger provisioning.
Device and Unit descriptor configurable parameters are
parsed from vendor specific provisioning data or file and
passed via configfs node at runtime to provision ufs device.

Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
---
 drivers/scsi/ufs/ufs.h    |  28 +++++++
 drivers/scsi/ufs/ufshcd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h |   2 +
 3 files changed, 228 insertions(+)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index e15deb0..1f99904 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -333,6 +333,7 @@ enum {
 	UFSHCD_AMP		= 3,
 };
 
+#define UFS_BLOCK_SIZE	4096
 #define POWER_DESC_MAX_SIZE			0x62
 #define POWER_DESC_MAX_ACTV_ICC_LVLS		16
 
@@ -425,6 +426,33 @@ enum {
 	MASK_TM_SERVICE_RESP		= 0xFF,
 };
 
+struct ufs_unit_desc {
+	u8     bLUEnable;              /* 1 for enabled LU */
+	u8     bBootLunID;             /* 0 for using this LU for boot */
+	u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
+	u8     bMemoryType;            /* 0 for enhanced memory type */
+	u32    dNumAllocUnits;         /* Number of alloc unit for this LU */
+	u8     bDataReliability;       /* 0 for reliable write support */
+	u8     bLogicalBlockSize;      /* See section 13.2.3 of UFS standard */
+	u8     bProvisioningType;      /* 0 for thin provisioning */
+	u16    wContextCapabilities;   /* refer Unit Descriptor Description */
+};
+
+struct ufs_config_descr {
+	u8     bNumberLU;              /* Total number of active LUs */
+	u8     bBootEnable;            /* enabling device for partial init */
+	u8     bDescrAccessEn;         /* desc access during partial init */
+	u8     bInitPowerMode;         /* Initial device power mode */
+	u8     bHighPriorityLUN;       /* LUN of the high priority LU */
+	u8     bSecureRemovalType;     /* Erase config for data removal */
+	u8     bInitActiveICCLevel;    /* ICC level after reset */
+	u16    wPeriodicRTCUpdate;     /* 0 to set a priodic RTC update rate */
+	u32     bConfigDescrLock;      /* 1 to lock Configation Descriptor */
+	u32    qVendorConfigCode;      /* Vendor specific configuration code */
+	struct ufs_unit_desc unit[8];
+	u8	lun_to_grow;
+};
+
 /* Task management service response */
 enum {
 	UPIU_TASK_MANAGEMENT_FUNC_COMPL		= 0x00,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 351f874..370a7c6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -42,6 +42,7 @@
 #include <linux/nls.h>
 #include <linux/of.h>
 #include <linux/bitfield.h>
+#include <asm/unaligned.h>
 #include "ufshcd.h"
 #include "ufs_quirks.h"
 #include "unipro.h"
@@ -3063,6 +3064,14 @@ static inline int ufshcd_read_power_desc(struct ufs_hba *hba,
 	return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);
 }
 
+static inline int ufshcd_read_geometry_desc(struct ufs_hba *hba,
+					 u8 *buf,
+					 u32 size)
+{
+	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
+}
+
+
 static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
 {
 	return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
@@ -6363,6 +6372,195 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
 }
 
 /**
+ * ufshcd_do_config_device - API function for UFS provisioning
+ * hba: per-adapter instance
+ * Returns 0 for success, non-zero in case of failure.
+ */
+int ufshcd_do_config_device(struct ufs_hba *hba)
+{
+	struct ufs_config_descr *cfg = &hba->cfgs;
+	int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
+	u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
+	int i, ret = 0;
+	int lun_to_grow = -1;
+	u64 qTotalRawDeviceCapacity;
+	u16 wEnhanced1CapAdjFac, wEnhanced2CapAdjFac;
+	u32 dEnhanced1MaxNAllocU, dEnhanced2MaxNAllocU;
+	size_t alloc_units, units_to_create = 0;
+	size_t capacity_to_alloc_factor;
+	size_t enhanced1_units = 0, enhanced2_units = 0;
+	size_t conversion_ratio = 1;
+	u8 *pt;
+	u32 blocks_per_alloc_unit = 1024;
+	int geo_len = hba->desc_size.geom_desc;
+	u8 geo_buf[hba->desc_size.geom_desc];
+	unsigned int max_partitions = 8;
+
+	WARN_ON(!hba || !cfg);
+
+	pm_runtime_get_sync(hba->dev);
+	scsi_block_requests(hba->host);
+	if (ufshcd_wait_for_doorbell_clr(hba, U64_MAX)) {
+		dev_err(hba->dev, "%s: Timed out waitig for DB to clear\n",
+			__func__);
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = ufshcd_read_geometry_desc(hba, geo_buf, geo_len);
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed getting geometry_desc %d\n",
+			__func__, ret);
+		goto out;
+	}
+
+	/*
+	 * Get Geomtric parameters like total configurable memory
+	 * quantity (Offset 0x04 to 0x0b), Capacity Adjustment
+	 * Factors (Offset 0x30, 0x31, 0x36, 0x37), Max allocation
+	 * units (Offset 0x2c to 0x2f, 0x32 to 0x35) used to configure
+	 * the device logical units.
+	 */
+	qTotalRawDeviceCapacity = get_unaligned_be64(&geo_buf[0x04]);
+	wEnhanced1CapAdjFac = get_unaligned_be16(&geo_buf[0x30]);
+	wEnhanced2CapAdjFac = get_unaligned_be16(&geo_buf[0x36]);
+	dEnhanced1MaxNAllocU = get_unaligned_be32(&geo_buf[0x2c]);
+	dEnhanced2MaxNAllocU = get_unaligned_be32(&geo_buf[0x32]);
+
+	capacity_to_alloc_factor =
+		(blocks_per_alloc_unit * UFS_BLOCK_SIZE) / 512;
+
+	if (qTotalRawDeviceCapacity % capacity_to_alloc_factor != 0) {
+		dev_err(hba->dev,
+			"%s: Raw capacity(%llu) not multiple of alloc factor(%zu)\n",
+			__func__, qTotalRawDeviceCapacity,
+			capacity_to_alloc_factor);
+		ret = -EINVAL;
+		goto out;
+	}
+	alloc_units = (qTotalRawDeviceCapacity / capacity_to_alloc_factor);
+	units_to_create = 0;
+	enhanced1_units = enhanced2_units = 0;
+
+	/*
+	 * Calculate number of allocation units to be assigned to a logical unit
+	 * considering the capacity adjustment factor of respective memory type.
+	 */
+	for (i = 0; i < (max_partitions - 1) &&
+		units_to_create <= alloc_units; i++) {
+		if ((cfg->unit[i].dNumAllocUnits % blocks_per_alloc_unit) == 0)
+			cfg->unit[i].dNumAllocUnits /= blocks_per_alloc_unit;
+		else
+			cfg->unit[i].dNumAllocUnits =
+			cfg->unit[i].dNumAllocUnits / blocks_per_alloc_unit + 1;
+
+		if (cfg->unit[i].bMemoryType == 0)
+			units_to_create += cfg->unit[i].dNumAllocUnits;
+		else if (cfg->unit[i].bMemoryType == 3) {
+			enhanced1_units += cfg->unit[i].dNumAllocUnits;
+			cfg->unit[i].dNumAllocUnits *=
+				(wEnhanced1CapAdjFac / 0x100);
+			units_to_create += cfg->unit[i].dNumAllocUnits;
+		} else if (cfg->unit[i].bMemoryType == 4) {
+			enhanced2_units += cfg->unit[i].dNumAllocUnits;
+			cfg->unit[i].dNumAllocUnits *=
+				(wEnhanced1CapAdjFac / 0x100);
+			units_to_create += cfg->unit[i].dNumAllocUnits;
+		} else {
+			dev_err(hba->dev, "%s: Unsupported memory type %d\n",
+				__func__, cfg->unit[i].bMemoryType);
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+	if (enhanced1_units > dEnhanced1MaxNAllocU) {
+		dev_err(hba->dev, "%s: size %zu exceeds max enhanced1 area size %u\n",
+			__func__, enhanced1_units, dEnhanced1MaxNAllocU);
+		ret = -ERANGE;
+		goto out;
+	}
+	if (enhanced2_units > dEnhanced2MaxNAllocU) {
+		dev_err(hba->dev, "%s: size %zu exceeds max enhanced2 area size %u\n",
+			__func__, enhanced2_units, dEnhanced2MaxNAllocU);
+		ret = -ERANGE;
+		goto out;
+	}
+	if (units_to_create > alloc_units) {
+		dev_err(hba->dev, "%s: Specified size %zu exceeds device size %zu\n",
+			__func__, units_to_create, alloc_units);
+		ret = -ERANGE;
+		goto out;
+	}
+	lun_to_grow = cfg->lun_to_grow;
+	if (lun_to_grow != -1) {
+		if (cfg->unit[i].bMemoryType == 0)
+			conversion_ratio = 1;
+		else if (cfg->unit[i].bMemoryType == 3)
+			conversion_ratio = (wEnhanced1CapAdjFac / 0x100);
+		else if (cfg->unit[i].bMemoryType == 4)
+			conversion_ratio = (wEnhanced2CapAdjFac / 0x100);
+
+		cfg->unit[lun_to_grow].dNumAllocUnits +=
+		((alloc_units - units_to_create) / conversion_ratio);
+		dev_dbg(hba->dev, "%s: conversion_ratio %zu for lun %d\n",
+			__func__, conversion_ratio, i);
+	}
+
+	/* Fill in the buffer with configuration data */
+	pt = desc_buf;
+	*pt++ = 0x90;        // bLength
+	*pt++ = 0x01;        // bDescriptorType
+	*pt++ = 0;           // Reserved in UFS2.0 and onward
+	*pt++ = cfg->bBootEnable;
+	*pt++ = cfg->bDescrAccessEn;
+	*pt++ = cfg->bInitPowerMode;
+	*pt++ = cfg->bHighPriorityLUN;
+	*pt++ = cfg->bSecureRemovalType;
+	*pt++ = cfg->bInitActiveICCLevel;
+	put_unaligned_be16(cfg->wPeriodicRTCUpdate, pt);
+	pt = pt + 7; // Reserved fields set to 0
+
+	/* Fill in the buffer with per logical unit data */
+	for (i = 0; i < UFS_UPIU_MAX_GENERAL_LUN; i++) {
+		*pt++ = cfg->unit[i].bLUEnable;
+		*pt++ = cfg->unit[i].bBootLunID;
+		*pt++ = cfg->unit[i].bLUWriteProtect;
+		*pt++ = cfg->unit[i].bMemoryType;
+		put_unaligned_be32(cfg->unit[i].dNumAllocUnits, pt);
+		pt = pt + 4;
+		*pt++ = cfg->unit[i].bDataReliability;
+		*pt++ = cfg->unit[i].bLogicalBlockSize;
+		*pt++ = cfg->unit[i].bProvisioningType;
+		put_unaligned_be16(cfg->unit[i].wContextCapabilities, pt);
+		pt = pt + 5; // Reserved fields set to 0
+	}
+
+	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
+		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
+
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x ret %d\n",
+		__func__, QUERY_DESC_IDN_CONFIGURATION,
+		UPIU_QUERY_OPCODE_WRITE_DESC, ret);
+		goto out;
+	}
+
+	if (cfg->bConfigDescrLock == 1) {
+		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
+		if (ret)
+			dev_err(hba->dev, "%s: Failed writing bConfigDescrLock %d\n",
+				__func__, ret);
+	}
+
+out:
+	scsi_unblock_requests(hba->host);
+	pm_runtime_put_sync(hba->dev);
+
+	return ret;
+}
+
+/**
  * ufshcd_probe_hba - probe hba to detect device and initialize
  * @hba: per-adapter instance
  *
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 101a75c..0e6bf30 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -549,6 +549,7 @@ struct ufs_hba {
 	unsigned int irq;
 	bool is_irq_enabled;
 	u32 dev_ref_clk_freq;
+	struct ufs_config_descr cfgs;
 
 	/* Interrupt aggregation support is broken */
 	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
@@ -867,6 +868,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index,
 
 int ufshcd_hold(struct ufs_hba *hba, bool async);
 void ufshcd_release(struct ufs_hba *hba);
+int ufshcd_do_config_device(struct ufs_hba *hba);
 
 int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
 	int *desc_length);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V4 2/3] scsi: ufs: Add ufs provisioning support

[Asutosh Das (asd)]

On 6/26/2018 9:33 AM, Sayali Lokhande wrote:
> A new api ufshcd_do_config_device() is added in driver
> to support UFS provisioning at runtime. Configfs support
> is added to trigger provisioning.
> Device and Unit descriptor configurable parameters are
> parsed from vendor specific provisioning data or file and
> passed via configfs node at runtime to provision ufs device.
> 
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>   drivers/scsi/ufs/ufs.h    |  28 +++++++
>   drivers/scsi/ufs/ufshcd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/scsi/ufs/ufshcd.h |   2 +
>   3 files changed, 228 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index e15deb0..1f99904 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -333,6 +333,7 @@ enum {
>   	UFSHCD_AMP		= 3,
>   };
>   
> +#define UFS_BLOCK_SIZE	4096
>   #define POWER_DESC_MAX_SIZE			0x62
>   #define POWER_DESC_MAX_ACTV_ICC_LVLS		16
>   
> @@ -425,6 +426,33 @@ enum {
>   	MASK_TM_SERVICE_RESP		= 0xFF,
>   };
>   
> +struct ufs_unit_desc {
> +	u8     bLUEnable;              /* 1 for enabled LU */
> +	u8     bBootLunID;             /* 0 for using this LU for boot */
> +	u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
> +	u8     bMemoryType;            /* 0 for enhanced memory type */
> +	u32    dNumAllocUnits;         /* Number of alloc unit for this LU */
> +	u8     bDataReliability;       /* 0 for reliable write support */
> +	u8     bLogicalBlockSize;      /* See section 13.2.3 of UFS standard */
> +	u8     bProvisioningType;      /* 0 for thin provisioning */
> +	u16    wContextCapabilities;   /* refer Unit Descriptor Description */
> +};
> +
> +struct ufs_config_descr {
> +	u8     bNumberLU;              /* Total number of active LUs */
> +	u8     bBootEnable;            /* enabling device for partial init */
> +	u8     bDescrAccessEn;         /* desc access during partial init */
> +	u8     bInitPowerMode;         /* Initial device power mode */
> +	u8     bHighPriorityLUN;       /* LUN of the high priority LU */
> +	u8     bSecureRemovalType;     /* Erase config for data removal */
> +	u8     bInitActiveICCLevel;    /* ICC level after reset */
> +	u16    wPeriodicRTCUpdate;     /* 0 to set a priodic RTC update rate */
> +	u32     bConfigDescrLock;      /* 1 to lock Configation Descriptor */
> +	u32    qVendorConfigCode;      /* Vendor specific configuration code */
> +	struct ufs_unit_desc unit[8];
> +	u8	lun_to_grow;
> +};
> +
>   /* Task management service response */
>   enum {
>   	UPIU_TASK_MANAGEMENT_FUNC_COMPL		= 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 351f874..370a7c6 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -42,6 +42,7 @@
>   #include <linux/nls.h>
>   #include <linux/of.h>
>   #include <linux/bitfield.h>
> +#include <asm/unaligned.h>
>   #include "ufshcd.h"
>   #include "ufs_quirks.h"
>   #include "unipro.h"
> @@ -3063,6 +3064,14 @@ static inline int ufshcd_read_power_desc(struct ufs_hba *hba,
>   	return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);
>   }
>   
> +static inline int ufshcd_read_geometry_desc(struct ufs_hba *hba,
> +					 u8 *buf,
> +					 u32 size)
> +{
> +	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
> +}
> +
> +
>   static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
>   {
>   	return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
> @@ -6363,6 +6372,195 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
>   }
>   
>   /**
> + * ufshcd_do_config_device - API function for UFS provisioning
> + * hba: per-adapter instance
> + * Returns 0 for success, non-zero in case of failure.
> + */
> +int ufshcd_do_config_device(struct ufs_hba *hba)
> +{
> +	struct ufs_config_descr *cfg = &hba->cfgs;
> +	int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
> +	u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
> +	int i, ret = 0;
> +	int lun_to_grow = -1;
> +	u64 qTotalRawDeviceCapacity;
> +	u16 wEnhanced1CapAdjFac, wEnhanced2CapAdjFac;
> +	u32 dEnhanced1MaxNAllocU, dEnhanced2MaxNAllocU;
> +	size_t alloc_units, units_to_create = 0;
> +	size_t capacity_to_alloc_factor;
> +	size_t enhanced1_units = 0, enhanced2_units = 0;
> +	size_t conversion_ratio = 1;
> +	u8 *pt;
> +	u32 blocks_per_alloc_unit = 1024;
> +	int geo_len = hba->desc_size.geom_desc;
> +	u8 geo_buf[hba->desc_size.geom_desc];
> +	unsigned int max_partitions = 8;
> +
> +	WARN_ON(!hba || !cfg);
There's no point of null checks here. It's being dereferenced at the 
declaration above.
Also, I think it should return -EINVAL if these're null.
> +
> +	pm_runtime_get_sync(hba->dev);
> +	scsi_block_requests(hba->host);
> +	if (ufshcd_wait_for_doorbell_clr(hba, U64_MAX)) {
> +		dev_err(hba->dev, "%s: Timed out waitig for DB to clear\n",
> +			__func__);
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = ufshcd_read_geometry_desc(hba, geo_buf, geo_len);
> +	if (ret) {
> +		dev_err(hba->dev, "%s: Failed getting geometry_desc %d\n",
> +			__func__, ret);
> +		goto out;
> +	}
> +
> +	/*
> +	 * Get Geomtric parameters like total configurable memory
> +	 * quantity (Offset 0x04 to 0x0b), Capacity Adjustment
> +	 * Factors (Offset 0x30, 0x31, 0x36, 0x37), Max allocation
> +	 * units (Offset 0x2c to 0x2f, 0x32 to 0x35) used to configure
> +	 * the device logical units.
> +	 */
> +	qTotalRawDeviceCapacity = get_unaligned_be64(&geo_buf[0x04]);
> +	wEnhanced1CapAdjFac = get_unaligned_be16(&geo_buf[0x30]);
> +	wEnhanced2CapAdjFac = get_unaligned_be16(&geo_buf[0x36]);
> +	dEnhanced1MaxNAllocU = get_unaligned_be32(&geo_buf[0x2c]);
> +	dEnhanced2MaxNAllocU = get_unaligned_be32(&geo_buf[0x32]);
> +
> +	capacity_to_alloc_factor =
> +		(blocks_per_alloc_unit * UFS_BLOCK_SIZE) / 512;
> +
> +	if (qTotalRawDeviceCapacity % capacity_to_alloc_factor != 0) {
> +		dev_err(hba->dev,
> +			"%s: Raw capacity(%llu) not multiple of alloc factor(%zu)\n",
> +			__func__, qTotalRawDeviceCapacity,
> +			capacity_to_alloc_factor);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	alloc_units = (qTotalRawDeviceCapacity / capacity_to_alloc_factor);
> +	units_to_create = 0;
> +	enhanced1_units = enhanced2_units = 0;
> +
> +	/*
> +	 * Calculate number of allocation units to be assigned to a logical unit
> +	 * considering the capacity adjustment factor of respective memory type.
> +	 */
> +	for (i = 0; i < (max_partitions - 1) &&
> +		units_to_create <= alloc_units; i++) {
> +		if ((cfg->unit[i].dNumAllocUnits % blocks_per_alloc_unit) == 0)
> +			cfg->unit[i].dNumAllocUnits /= blocks_per_alloc_unit;
> +		else
> +			cfg->unit[i].dNumAllocUnits =
> +			cfg->unit[i].dNumAllocUnits / blocks_per_alloc_unit + 1;
> +
> +		if (cfg->unit[i].bMemoryType == 0)
> +			units_to_create += cfg->unit[i].dNumAllocUnits;
> +		else if (cfg->unit[i].bMemoryType == 3) {
> +			enhanced1_units += cfg->unit[i].dNumAllocUnits;
> +			cfg->unit[i].dNumAllocUnits *=
> +				(wEnhanced1CapAdjFac / 0x100);
> +			units_to_create += cfg->unit[i].dNumAllocUnits;
> +		} else if (cfg->unit[i].bMemoryType == 4) {
> +			enhanced2_units += cfg->unit[i].dNumAllocUnits;
> +			cfg->unit[i].dNumAllocUnits *=
> +				(wEnhanced1CapAdjFac / 0x100);
> +			units_to_create += cfg->unit[i].dNumAllocUnits;
> +		} else {
> +			dev_err(hba->dev, "%s: Unsupported memory type %d\n",
> +				__func__, cfg->unit[i].bMemoryType);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +	}
> +	if (enhanced1_units > dEnhanced1MaxNAllocU) {
> +		dev_err(hba->dev, "%s: size %zu exceeds max enhanced1 area size %u\n",
> +			__func__, enhanced1_units, dEnhanced1MaxNAllocU);
> +		ret = -ERANGE;
> +		goto out;
> +	}
> +	if (enhanced2_units > dEnhanced2MaxNAllocU) {
> +		dev_err(hba->dev, "%s: size %zu exceeds max enhanced2 area size %u\n",
> +			__func__, enhanced2_units, dEnhanced2MaxNAllocU);
> +		ret = -ERANGE;
> +		goto out;
> +	}
> +	if (units_to_create > alloc_units) {
> +		dev_err(hba->dev, "%s: Specified size %zu exceeds device size %zu\n",
> +			__func__, units_to_create, alloc_units);
> +		ret = -ERANGE;
> +		goto out;
> +	}
> +	lun_to_grow = cfg->lun_to_grow;
> +	if (lun_to_grow != -1) {
> +		if (cfg->unit[i].bMemoryType == 0)
> +			conversion_ratio = 1;
> +		else if (cfg->unit[i].bMemoryType == 3)
> +			conversion_ratio = (wEnhanced1CapAdjFac / 0x100);
> +		else if (cfg->unit[i].bMemoryType == 4)
> +			conversion_ratio = (wEnhanced2CapAdjFac / 0x100);
> +
> +		cfg->unit[lun_to_grow].dNumAllocUnits +=
> +		((alloc_units - units_to_create) / conversion_ratio);
> +		dev_dbg(hba->dev, "%s: conversion_ratio %zu for lun %d\n",
> +			__func__, conversion_ratio, i);
> +	}
> +
> +	/* Fill in the buffer with configuration data */
> +	pt = desc_buf;
> +	*pt++ = 0x90;        // bLength
> +	*pt++ = 0x01;        // bDescriptorType
> +	*pt++ = 0;           // Reserved in UFS2.0 and onward
> +	*pt++ = cfg->bBootEnable;
> +	*pt++ = cfg->bDescrAccessEn;
> +	*pt++ = cfg->bInitPowerMode;
> +	*pt++ = cfg->bHighPriorityLUN;
> +	*pt++ = cfg->bSecureRemovalType;
> +	*pt++ = cfg->bInitActiveICCLevel;
> +	put_unaligned_be16(cfg->wPeriodicRTCUpdate, pt);
> +	pt = pt + 7; // Reserved fields set to 0
> +
> +	/* Fill in the buffer with per logical unit data */
> +	for (i = 0; i < UFS_UPIU_MAX_GENERAL_LUN; i++) {
> +		*pt++ = cfg->unit[i].bLUEnable;
> +		*pt++ = cfg->unit[i].bBootLunID;
> +		*pt++ = cfg->unit[i].bLUWriteProtect;
> +		*pt++ = cfg->unit[i].bMemoryType;
> +		put_unaligned_be32(cfg->unit[i].dNumAllocUnits, pt);
> +		pt = pt + 4;
> +		*pt++ = cfg->unit[i].bDataReliability;
> +		*pt++ = cfg->unit[i].bLogicalBlockSize;
> +		*pt++ = cfg->unit[i].bProvisioningType;
> +		put_unaligned_be16(cfg->unit[i].wContextCapabilities, pt);
> +		pt = pt + 5; // Reserved fields set to 0
> +	}
> +
> +	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
> +		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
> +
> +	if (ret) {
> +		dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x ret %d\n",
> +		__func__, QUERY_DESC_IDN_CONFIGURATION,
> +		UPIU_QUERY_OPCODE_WRITE_DESC, ret);
> +		goto out;
> +	}
> +
> +	if (cfg->bConfigDescrLock == 1) {
> +		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
> +		if (ret)
> +			dev_err(hba->dev, "%s: Failed writing bConfigDescrLock %d\n",
> +				__func__, ret);
> +	}
> +
> +out:
> +	scsi_unblock_requests(hba->host);
> +	pm_runtime_put_sync(hba->dev);
> +
> +	return ret;
> +}
> +
> +/**
>    * ufshcd_probe_hba - probe hba to detect device and initialize
>    * @hba: per-adapter instance
>    *
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 101a75c..0e6bf30 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -549,6 +549,7 @@ struct ufs_hba {
>   	unsigned int irq;
>   	bool is_irq_enabled;
>   	u32 dev_ref_clk_freq;
> +	struct ufs_config_descr cfgs;
>   
>   	/* Interrupt aggregation support is broken */
>   	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
> @@ -867,6 +868,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index,
>   
>   int ufshcd_hold(struct ufs_hba *hba, bool async);
>   void ufshcd_release(struct ufs_hba *hba);
> +int ufshcd_do_config_device(struct ufs_hba *hba);
>   
>   int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
>   	int *desc_length);
> 


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [PATCH V4 2/3] scsi: ufs: Add ufs provisioning support

[Evan Green]

Hi Sayali,

On Mon, Jun 25, 2018 at 9:04 PM Sayali Lokhande <sayalil@codeaurora.org> wrote:
>
> A new api ufshcd_do_config_device() is added in driver
> to support UFS provisioning at runtime. Configfs support
> is added to trigger provisioning.
> Device and Unit descriptor configurable parameters are
> parsed from vendor specific provisioning data or file and
> passed via configfs node at runtime to provision ufs device.
>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  28 +++++++
>  drivers/scsi/ufs/ufshcd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |   2 +
>  3 files changed, 228 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index e15deb0..1f99904 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -333,6 +333,7 @@ enum {
>         UFSHCD_AMP              = 3,
>  };
>
> +#define UFS_BLOCK_SIZE 4096

Is this not prescribed by UFS. You need to look at bMinAddrBlockSize,
whose minimum value is 8, representing 4k blocks. But you can't assume
they're all going to be that, the spec allows for larger block sizes.

>  #define POWER_DESC_MAX_SIZE                    0x62
>  #define POWER_DESC_MAX_ACTV_ICC_LVLS           16
>
> @@ -425,6 +426,33 @@ enum {
>         MASK_TM_SERVICE_RESP            = 0xFF,
>  };
>
> +struct ufs_unit_desc {
> +       u8     bLUEnable;              /* 1 for enabled LU */
> +       u8     bBootLunID;             /* 0 for using this LU for boot */
> +       u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
> +       u8     bMemoryType;            /* 0 for enhanced memory type */
> +       u32    dNumAllocUnits;         /* Number of alloc unit for this LU */
> +       u8     bDataReliability;       /* 0 for reliable write support */
> +       u8     bLogicalBlockSize;      /* See section 13.2.3 of UFS standard */
> +       u8     bProvisioningType;      /* 0 for thin provisioning */
> +       u16    wContextCapabilities;   /* refer Unit Descriptor Description */
> +};
> +
> +struct ufs_config_descr {
> +       u8     bNumberLU;              /* Total number of active LUs */
> +       u8     bBootEnable;            /* enabling device for partial init */
> +       u8     bDescrAccessEn;         /* desc access during partial init */
> +       u8     bInitPowerMode;         /* Initial device power mode */
> +       u8     bHighPriorityLUN;       /* LUN of the high priority LU */
> +       u8     bSecureRemovalType;     /* Erase config for data removal */
> +       u8     bInitActiveICCLevel;    /* ICC level after reset */
> +       u16    wPeriodicRTCUpdate;     /* 0 to set a priodic RTC update rate */
> +       u32     bConfigDescrLock;      /* 1 to lock Configation Descriptor */
> +       u32    qVendorConfigCode;      /* Vendor specific configuration code */
> +       struct ufs_unit_desc unit[8];
> +       u8      lun_to_grow;
> +};

You've created a structure whose naming and members suggest that it
matches the hardware, but it does not. I think you should make this
structure match the hardware, and remove software members like
lun_to_grow. It's okay with me if 8 is hardcoded for now, but bear in
mind I think the spec allows for this to be a different number.

> +
>  /* Task management service response */
>  enum {
>         UPIU_TASK_MANAGEMENT_FUNC_COMPL         = 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 351f874..370a7c6 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -42,6 +42,7 @@
>  #include <linux/nls.h>
>  #include <linux/of.h>
>  #include <linux/bitfield.h>
> +#include <asm/unaligned.h>

Alphabetize includes, though I think if you follow my other
suggestions this will go away.

>  #include "ufshcd.h"
>  #include "ufs_quirks.h"
>  #include "unipro.h"
> @@ -3063,6 +3064,14 @@ static inline int ufshcd_read_power_desc(struct ufs_hba *hba,
>         return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);
>  }
>
> +static inline int ufshcd_read_geometry_desc(struct ufs_hba *hba,
> +                                        u8 *buf,
> +                                        u32 size)
> +{
> +       return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
> +}
> +
> +

Remove extra blank line.

>  static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
>  {
>         return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
> @@ -6363,6 +6372,195 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
>  }
>
>  /**
> + * ufshcd_do_config_device - API function for UFS provisioning
> + * hba: per-adapter instance
> + * Returns 0 for success, non-zero in case of failure.
> + */
> +int ufshcd_do_config_device(struct ufs_hba *hba)
> +{
> +       struct ufs_config_descr *cfg = &hba->cfgs;
> +       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
> +       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
> +       int i, ret = 0;
> +       int lun_to_grow = -1;
> +       u64 qTotalRawDeviceCapacity;
> +       u16 wEnhanced1CapAdjFac, wEnhanced2CapAdjFac;
> +       u32 dEnhanced1MaxNAllocU, dEnhanced2MaxNAllocU;
> +       size_t alloc_units, units_to_create = 0;
> +       size_t capacity_to_alloc_factor;
> +       size_t enhanced1_units = 0, enhanced2_units = 0;
> +       size_t conversion_ratio = 1;
> +       u8 *pt;
> +       u32 blocks_per_alloc_unit = 1024;

Is this really hardcoded by the spec? I think it probably isn't, but
if it is, make it a define.

> +       int geo_len = hba->desc_size.geom_desc;
> +       u8 geo_buf[hba->desc_size.geom_desc];
> +       unsigned int max_partitions = 8;

Magic value. Perhaps a define here as well.

> +
> +       WARN_ON(!hba || !cfg);
> +
> +       pm_runtime_get_sync(hba->dev);
> +       scsi_block_requests(hba->host);

Is this needed? Why is it bad to have requests going through during
this function, given that you re-enable them at the end of this
function?

> +       if (ufshcd_wait_for_doorbell_clr(hba, U64_MAX)) {

Why is this needed?

> +               dev_err(hba->dev, "%s: Timed out waitig for DB to clear\n",
> +                       __func__);
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +       ret = ufshcd_read_geometry_desc(hba, geo_buf, geo_len);
> +       if (ret) {
> +               dev_err(hba->dev, "%s: Failed getting geometry_desc %d\n",
> +                       __func__, ret);
> +               goto out;
> +       }
> +
> +       /*
> +        * Get Geomtric parameters like total configurable memory
> +        * quantity (Offset 0x04 to 0x0b), Capacity Adjustment
> +        * Factors (Offset 0x30, 0x31, 0x36, 0x37), Max allocation
> +        * units (Offset 0x2c to 0x2f, 0x32 to 0x35) used to configure
> +        * the device logical units.
> +        */
> +       qTotalRawDeviceCapacity = get_unaligned_be64(&geo_buf[0x04]);
> +       wEnhanced1CapAdjFac = get_unaligned_be16(&geo_buf[0x30]);
> +       wEnhanced2CapAdjFac = get_unaligned_be16(&geo_buf[0x36]);
> +       dEnhanced1MaxNAllocU = get_unaligned_be32(&geo_buf[0x2c]);
> +       dEnhanced2MaxNAllocU = get_unaligned_be32(&geo_buf[0x32]);

Magic values, these should be done as other descriptors do it (see
enum device_desc_param).

> +
> +       capacity_to_alloc_factor =
> +               (blocks_per_alloc_unit * UFS_BLOCK_SIZE) / 512;
> +
> +       if (qTotalRawDeviceCapacity % capacity_to_alloc_factor != 0) {
> +               dev_err(hba->dev,
> +                       "%s: Raw capacity(%llu) not multiple of alloc factor(%zu)\n",
> +                       __func__, qTotalRawDeviceCapacity,
> +                       capacity_to_alloc_factor);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +       alloc_units = (qTotalRawDeviceCapacity / capacity_to_alloc_factor);
> +       units_to_create = 0;
> +       enhanced1_units = enhanced2_units = 0;
> +
> +       /*
> +        * Calculate number of allocation units to be assigned to a logical unit
> +        * considering the capacity adjustment factor of respective memory type.
> +        */
> +       for (i = 0; i < (max_partitions - 1) &&
> +               units_to_create <= alloc_units; i++) {
> +               if ((cfg->unit[i].dNumAllocUnits % blocks_per_alloc_unit) == 0)
> +                       cfg->unit[i].dNumAllocUnits /= blocks_per_alloc_unit;
> +               else
> +                       cfg->unit[i].dNumAllocUnits =
> +                       cfg->unit[i].dNumAllocUnits / blocks_per_alloc_unit + 1;
> +
> +               if (cfg->unit[i].bMemoryType == 0)
> +                       units_to_create += cfg->unit[i].dNumAllocUnits;
> +               else if (cfg->unit[i].bMemoryType == 3) {
> +                       enhanced1_units += cfg->unit[i].dNumAllocUnits;
> +                       cfg->unit[i].dNumAllocUnits *=
> +                               (wEnhanced1CapAdjFac / 0x100);
> +                       units_to_create += cfg->unit[i].dNumAllocUnits;
> +               } else if (cfg->unit[i].bMemoryType == 4) {
> +                       enhanced2_units += cfg->unit[i].dNumAllocUnits;
> +                       cfg->unit[i].dNumAllocUnits *=
> +                               (wEnhanced1CapAdjFac / 0x100);
> +                       units_to_create += cfg->unit[i].dNumAllocUnits;
> +               } else {
> +                       dev_err(hba->dev, "%s: Unsupported memory type %d\n",
> +                               __func__, cfg->unit[i].bMemoryType);
> +                       ret = -EINVAL;
> +                       goto out;
> +               }
> +       }
> +       if (enhanced1_units > dEnhanced1MaxNAllocU) {
> +               dev_err(hba->dev, "%s: size %zu exceeds max enhanced1 area size %u\n",
> +                       __func__, enhanced1_units, dEnhanced1MaxNAllocU);
> +               ret = -ERANGE;
> +               goto out;
> +       }
> +       if (enhanced2_units > dEnhanced2MaxNAllocU) {
> +               dev_err(hba->dev, "%s: size %zu exceeds max enhanced2 area size %u\n",
> +                       __func__, enhanced2_units, dEnhanced2MaxNAllocU);
> +               ret = -ERANGE;
> +               goto out;
> +       }
> +       if (units_to_create > alloc_units) {
> +               dev_err(hba->dev, "%s: Specified size %zu exceeds device size %zu\n",
> +                       __func__, units_to_create, alloc_units);
> +               ret = -ERANGE;
> +               goto out;
> +       }
> +       lun_to_grow = cfg->lun_to_grow;
> +       if (lun_to_grow != -1) {
> +               if (cfg->unit[i].bMemoryType == 0)
> +                       conversion_ratio = 1;
> +               else if (cfg->unit[i].bMemoryType == 3)
> +                       conversion_ratio = (wEnhanced1CapAdjFac / 0x100);
> +               else if (cfg->unit[i].bMemoryType == 4)
> +                       conversion_ratio = (wEnhanced2CapAdjFac / 0x100);
> +
> +               cfg->unit[lun_to_grow].dNumAllocUnits +=
> +               ((alloc_units - units_to_create) / conversion_ratio);
> +               dev_dbg(hba->dev, "%s: conversion_ratio %zu for lun %d\n",
> +                       __func__, conversion_ratio, i);
> +       }

I really don't like all this wizardry in here. I think the kernel
should just accept config descriptor bytes through configfs that it
more or less passes along. These calculations could all be done in
user mode.

> +
> +       /* Fill in the buffer with configuration data */
> +       pt = desc_buf;
> +       *pt++ = 0x90;        // bLength
> +       *pt++ = 0x01;        // bDescriptorType
> +       *pt++ = 0;           // Reserved in UFS2.0 and onward
> +       *pt++ = cfg->bBootEnable;
> +       *pt++ = cfg->bDescrAccessEn;
> +       *pt++ = cfg->bInitPowerMode;
> +       *pt++ = cfg->bHighPriorityLUN;
> +       *pt++ = cfg->bSecureRemovalType;
> +       *pt++ = cfg->bInitActiveICCLevel;
> +       put_unaligned_be16(cfg->wPeriodicRTCUpdate, pt);
> +       pt = pt + 7; // Reserved fields set to 0
> +
> +       /* Fill in the buffer with per logical unit data */
> +       for (i = 0; i < UFS_UPIU_MAX_GENERAL_LUN; i++) {
> +               *pt++ = cfg->unit[i].bLUEnable;
> +               *pt++ = cfg->unit[i].bBootLunID;
> +               *pt++ = cfg->unit[i].bLUWriteProtect;
> +               *pt++ = cfg->unit[i].bMemoryType;
> +               put_unaligned_be32(cfg->unit[i].dNumAllocUnits, pt);
> +               pt = pt + 4;
> +               *pt++ = cfg->unit[i].bDataReliability;
> +               *pt++ = cfg->unit[i].bLogicalBlockSize;
> +               *pt++ = cfg->unit[i].bProvisioningType;
> +               put_unaligned_be16(cfg->unit[i].wContextCapabilities, pt);
> +               pt = pt + 5; // Reserved fields set to 0
> +       }

Yikes. If your structure reflected the hardware, then you could use
actual members. Barring that, you could create and use enum values for
the descriptor members. The way it stands this is very brittle and
full of magic values and offsets.

> +
> +       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
> +               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);

I'm not sure this works out of the box, especially if what you're
writing isn't exactly the descriptor size (which it might be in your
tests, but might suddenly start failing later). Please consider
absorbing my change [1] which properly refactors the function for
reading and writing.

> +
> +       if (ret) {
> +               dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x ret %d\n",
> +               __func__, QUERY_DESC_IDN_CONFIGURATION,
> +               UPIU_QUERY_OPCODE_WRITE_DESC, ret);
> +               goto out;
> +       }
> +
> +       if (cfg->bConfigDescrLock == 1) {
> +               ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +               QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
> +               if (ret)
> +                       dev_err(hba->dev, "%s: Failed writing bConfigDescrLock %d\n",
> +                               __func__, ret);
> +       }

It might be better not to merge this functionality in with the act of
writing provisioning data, and instead make the sysfs attributes
writable like [2].

> +
> +out:
> +       scsi_unblock_requests(hba->host);
> +       pm_runtime_put_sync(hba->dev);
> +
> +       return ret;
> +}
> +
> +/**
>   * ufshcd_probe_hba - probe hba to detect device and initialize
>   * @hba: per-adapter instance
>   *
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 101a75c..0e6bf30 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -549,6 +549,7 @@ struct ufs_hba {
>         unsigned int irq;
>         bool is_irq_enabled;
>         u32 dev_ref_clk_freq;
> +       struct ufs_config_descr cfgs;

How come you store this here, rather than as a local in
ufshcd_do_config_device()?

>
>         /* Interrupt aggregation support is broken */
>         #define UFSHCD_QUIRK_BROKEN_INTR_AGGR                   0x1
> @@ -867,6 +868,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index,
>
>  int ufshcd_hold(struct ufs_hba *hba, bool async);
>  void ufshcd_release(struct ufs_hba *hba);
> +int ufshcd_do_config_device(struct ufs_hba *hba);
>
>  int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
>         int *desc_length);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

[1] https://patchwork.kernel.org/patch/10467669/
[2] https://patchwork.kernel.org/patch/10467665/

Re: [PATCH V4 2/3] scsi: ufs: Add ufs provisioning support

[Sayali Lokhande]

Hi Evan,


On 7/3/2018 5:12 AM, Evan Green wrote:
> Hi Sayali,
>
> On Mon, Jun 25, 2018 at 9:04 PM Sayali Lokhande <sayalil@codeaurora.org> wrote:
>> A new api ufshcd_do_config_device() is added in driver
>> to support UFS provisioning at runtime. Configfs support
>> is added to trigger provisioning.
>> Device and Unit descriptor configurable parameters are
>> parsed from vendor specific provisioning data or file and
>> passed via configfs node at runtime to provision ufs device.
>>
>> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
>> ---
>>   drivers/scsi/ufs/ufs.h    |  28 +++++++
>>   drivers/scsi/ufs/ufshcd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/scsi/ufs/ufshcd.h |   2 +
>>   3 files changed, 228 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index e15deb0..1f99904 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -333,6 +333,7 @@ enum {
>>          UFSHCD_AMP              = 3,
>>   };
>>
>> +#define UFS_BLOCK_SIZE 4096
> Is this not prescribed by UFS. You need to look at bMinAddrBlockSize,
> whose minimum value is 8, representing 4k blocks. But you can't assume
> they're all going to be that, the spec allows for larger block sizes.
Will get rid of it and also remove all the calculations that used it.
>
>>   #define POWER_DESC_MAX_SIZE                    0x62
>>   #define POWER_DESC_MAX_ACTV_ICC_LVLS           16
>>
>> @@ -425,6 +426,33 @@ enum {
>>          MASK_TM_SERVICE_RESP            = 0xFF,
>>   };
>>
>> +struct ufs_unit_desc {
>> +       u8     bLUEnable;              /* 1 for enabled LU */
>> +       u8     bBootLunID;             /* 0 for using this LU for boot */
>> +       u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
>> +       u8     bMemoryType;            /* 0 for enhanced memory type */
>> +       u32    dNumAllocUnits;         /* Number of alloc unit for this LU */
>> +       u8     bDataReliability;       /* 0 for reliable write support */
>> +       u8     bLogicalBlockSize;      /* See section 13.2.3 of UFS standard */
>> +       u8     bProvisioningType;      /* 0 for thin provisioning */
>> +       u16    wContextCapabilities;   /* refer Unit Descriptor Description */
>> +};
>> +
>> +struct ufs_config_descr {
>> +       u8     bNumberLU;              /* Total number of active LUs */
>> +       u8     bBootEnable;            /* enabling device for partial init */
>> +       u8     bDescrAccessEn;         /* desc access during partial init */
>> +       u8     bInitPowerMode;         /* Initial device power mode */
>> +       u8     bHighPriorityLUN;       /* LUN of the high priority LU */
>> +       u8     bSecureRemovalType;     /* Erase config for data removal */
>> +       u8     bInitActiveICCLevel;    /* ICC level after reset */
>> +       u16    wPeriodicRTCUpdate;     /* 0 to set a priodic RTC update rate */
>> +       u32     bConfigDescrLock;      /* 1 to lock Configation Descriptor */
>> +       u32    qVendorConfigCode;      /* Vendor specific configuration code */
>> +       struct ufs_unit_desc unit[8];
>> +       u8      lun_to_grow;
>> +};
> You've created a structure whose naming and members suggest that it
> matches the hardware, but it does not. I think you should make this
> structure match the hardware, and remove software members like
> lun_to_grow. It's okay with me if 8 is hardcoded for now, but bear in
> mind I think the spec allows for this to be a different number.
Will get rid of these structs.
>> +
>>   /* Task management service response */
>>   enum {
>>          UPIU_TASK_MANAGEMENT_FUNC_COMPL         = 0x00,
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 351f874..370a7c6 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -42,6 +42,7 @@
>>   #include <linux/nls.h>
>>   #include <linux/of.h>
>>   #include <linux/bitfield.h>
>> +#include <asm/unaligned.h>
> Alphabetize includes, though I think if you follow my other
> suggestions this will go away.
Agreed.
>>   #include "ufshcd.h"
>>   #include "ufs_quirks.h"
>>   #include "unipro.h"
>> @@ -3063,6 +3064,14 @@ static inline int ufshcd_read_power_desc(struct ufs_hba *hba,
>>          return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);
>>   }
>>
>> +static inline int ufshcd_read_geometry_desc(struct ufs_hba *hba,
>> +                                        u8 *buf,
>> +                                        u32 size)
>> +{
>> +       return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
>> +}
>> +
>> +
> Remove extra blank line.
>
>>   static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
>>   {
>>          return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
>> @@ -6363,6 +6372,195 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
>>   }
>>
>>   /**
>> + * ufshcd_do_config_device - API function for UFS provisioning
>> + * hba: per-adapter instance
>> + * Returns 0 for success, non-zero in case of failure.
>> + */
>> +int ufshcd_do_config_device(struct ufs_hba *hba)
>> +{
>> +       struct ufs_config_descr *cfg = &hba->cfgs;
>> +       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
>> +       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
>> +       int i, ret = 0;
>> +       int lun_to_grow = -1;
>> +       u64 qTotalRawDeviceCapacity;
>> +       u16 wEnhanced1CapAdjFac, wEnhanced2CapAdjFac;
>> +       u32 dEnhanced1MaxNAllocU, dEnhanced2MaxNAllocU;
>> +       size_t alloc_units, units_to_create = 0;
>> +       size_t capacity_to_alloc_factor;
>> +       size_t enhanced1_units = 0, enhanced2_units = 0;
>> +       size_t conversion_ratio = 1;
>> +       u8 *pt;
>> +       u32 blocks_per_alloc_unit = 1024;
> Is this really hardcoded by the spec? I think it probably isn't, but
> if it is, make it a define.
Will get rid of these calculations here.
>> +       int geo_len = hba->desc_size.geom_desc;
>> +       u8 geo_buf[hba->desc_size.geom_desc];
>> +       unsigned int max_partitions = 8;
> Magic value. Perhaps a define here as well.
Will get rid of these calculations here.
>> +
>> +       WARN_ON(!hba || !cfg);
>> +
>> +       pm_runtime_get_sync(hba->dev);
>> +       scsi_block_requests(hba->host);
> Is this needed? Why is it bad to have requests going through during
> this function, given that you re-enable them at the end of this
> function?
>
>> +       if (ufshcd_wait_for_doorbell_clr(hba, U64_MAX)) {
> Why is this needed?
This is just to ensure stable operation while doing runtime provisioning.
>> +               dev_err(hba->dev, "%s: Timed out waitig for DB to clear\n",
>> +                       __func__);
>> +               ret = -EBUSY;
>> +               goto out;
>> +       }
>> +
>> +       ret = ufshcd_read_geometry_desc(hba, geo_buf, geo_len);
>> +       if (ret) {
>> +               dev_err(hba->dev, "%s: Failed getting geometry_desc %d\n",
>> +                       __func__, ret);
>> +               goto out;
>> +       }
>> +
>> +       /*
>> +        * Get Geomtric parameters like total configurable memory
>> +        * quantity (Offset 0x04 to 0x0b), Capacity Adjustment
>> +        * Factors (Offset 0x30, 0x31, 0x36, 0x37), Max allocation
>> +        * units (Offset 0x2c to 0x2f, 0x32 to 0x35) used to configure
>> +        * the device logical units.
>> +        */
>> +       qTotalRawDeviceCapacity = get_unaligned_be64(&geo_buf[0x04]);
>> +       wEnhanced1CapAdjFac = get_unaligned_be16(&geo_buf[0x30]);
>> +       wEnhanced2CapAdjFac = get_unaligned_be16(&geo_buf[0x36]);
>> +       dEnhanced1MaxNAllocU = get_unaligned_be32(&geo_buf[0x2c]);
>> +       dEnhanced2MaxNAllocU = get_unaligned_be32(&geo_buf[0x32]);
> Magic values, these should be done as other descriptors do it (see
> enum device_desc_param).
Will get rid of these calculations here.
>> +
>> +       capacity_to_alloc_factor =
>> +               (blocks_per_alloc_unit * UFS_BLOCK_SIZE) / 512;
>> +
>> +       if (qTotalRawDeviceCapacity % capacity_to_alloc_factor != 0) {
>> +               dev_err(hba->dev,
>> +                       "%s: Raw capacity(%llu) not multiple of alloc factor(%zu)\n",
>> +                       __func__, qTotalRawDeviceCapacity,
>> +                       capacity_to_alloc_factor);
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>> +       alloc_units = (qTotalRawDeviceCapacity / capacity_to_alloc_factor);
>> +       units_to_create = 0;
>> +       enhanced1_units = enhanced2_units = 0;
>> +
>> +       /*
>> +        * Calculate number of allocation units to be assigned to a logical unit
>> +        * considering the capacity adjustment factor of respective memory type.
>> +        */
>> +       for (i = 0; i < (max_partitions - 1) &&
>> +               units_to_create <= alloc_units; i++) {
>> +               if ((cfg->unit[i].dNumAllocUnits % blocks_per_alloc_unit) == 0)
>> +                       cfg->unit[i].dNumAllocUnits /= blocks_per_alloc_unit;
>> +               else
>> +                       cfg->unit[i].dNumAllocUnits =
>> +                       cfg->unit[i].dNumAllocUnits / blocks_per_alloc_unit + 1;
>> +
>> +               if (cfg->unit[i].bMemoryType == 0)
>> +                       units_to_create += cfg->unit[i].dNumAllocUnits;
>> +               else if (cfg->unit[i].bMemoryType == 3) {
>> +                       enhanced1_units += cfg->unit[i].dNumAllocUnits;
>> +                       cfg->unit[i].dNumAllocUnits *=
>> +                               (wEnhanced1CapAdjFac / 0x100);
>> +                       units_to_create += cfg->unit[i].dNumAllocUnits;
>> +               } else if (cfg->unit[i].bMemoryType == 4) {
>> +                       enhanced2_units += cfg->unit[i].dNumAllocUnits;
>> +                       cfg->unit[i].dNumAllocUnits *=
>> +                               (wEnhanced1CapAdjFac / 0x100);
>> +                       units_to_create += cfg->unit[i].dNumAllocUnits;
>> +               } else {
>> +                       dev_err(hba->dev, "%s: Unsupported memory type %d\n",
>> +                               __func__, cfg->unit[i].bMemoryType);
>> +                       ret = -EINVAL;
>> +                       goto out;
>> +               }
>> +       }
>> +       if (enhanced1_units > dEnhanced1MaxNAllocU) {
>> +               dev_err(hba->dev, "%s: size %zu exceeds max enhanced1 area size %u\n",
>> +                       __func__, enhanced1_units, dEnhanced1MaxNAllocU);
>> +               ret = -ERANGE;
>> +               goto out;
>> +       }
>> +       if (enhanced2_units > dEnhanced2MaxNAllocU) {
>> +               dev_err(hba->dev, "%s: size %zu exceeds max enhanced2 area size %u\n",
>> +                       __func__, enhanced2_units, dEnhanced2MaxNAllocU);
>> +               ret = -ERANGE;
>> +               goto out;
>> +       }
>> +       if (units_to_create > alloc_units) {
>> +               dev_err(hba->dev, "%s: Specified size %zu exceeds device size %zu\n",
>> +                       __func__, units_to_create, alloc_units);
>> +               ret = -ERANGE;
>> +               goto out;
>> +       }
>> +       lun_to_grow = cfg->lun_to_grow;
>> +       if (lun_to_grow != -1) {
>> +               if (cfg->unit[i].bMemoryType == 0)
>> +                       conversion_ratio = 1;
>> +               else if (cfg->unit[i].bMemoryType == 3)
>> +                       conversion_ratio = (wEnhanced1CapAdjFac / 0x100);
>> +               else if (cfg->unit[i].bMemoryType == 4)
>> +                       conversion_ratio = (wEnhanced2CapAdjFac / 0x100);
>> +
>> +               cfg->unit[lun_to_grow].dNumAllocUnits +=
>> +               ((alloc_units - units_to_create) / conversion_ratio);
>> +               dev_dbg(hba->dev, "%s: conversion_ratio %zu for lun %d\n",
>> +                       __func__, conversion_ratio, i);
>> +       }
> I really don't like all this wizardry in here. I think the kernel
> should just accept config descriptor bytes through configfs that it
> more or less passes along. These calculations could all be done in
> user mode.
Agreed. Will get rid of these calculations here. We can just accept 
configuaration descriptor params (as in spec)
Calculations (if required) can be done beforehand (in user mode).
>> +
>> +       /* Fill in the buffer with configuration data */
>> +       pt = desc_buf;
>> +       *pt++ = 0x90;        // bLength
>> +       *pt++ = 0x01;        // bDescriptorType
>> +       *pt++ = 0;           // Reserved in UFS2.0 and onward
>> +       *pt++ = cfg->bBootEnable;
>> +       *pt++ = cfg->bDescrAccessEn;
>> +       *pt++ = cfg->bInitPowerMode;
>> +       *pt++ = cfg->bHighPriorityLUN;
>> +       *pt++ = cfg->bSecureRemovalType;
>> +       *pt++ = cfg->bInitActiveICCLevel;
>> +       put_unaligned_be16(cfg->wPeriodicRTCUpdate, pt);
>> +       pt = pt + 7; // Reserved fields set to 0
>> +
>> +       /* Fill in the buffer with per logical unit data */
>> +       for (i = 0; i < UFS_UPIU_MAX_GENERAL_LUN; i++) {
>> +               *pt++ = cfg->unit[i].bLUEnable;
>> +               *pt++ = cfg->unit[i].bBootLunID;
>> +               *pt++ = cfg->unit[i].bLUWriteProtect;
>> +               *pt++ = cfg->unit[i].bMemoryType;
>> +               put_unaligned_be32(cfg->unit[i].dNumAllocUnits, pt);
>> +               pt = pt + 4;
>> +               *pt++ = cfg->unit[i].bDataReliability;
>> +               *pt++ = cfg->unit[i].bLogicalBlockSize;
>> +               *pt++ = cfg->unit[i].bProvisioningType;
>> +               put_unaligned_be16(cfg->unit[i].wContextCapabilities, pt);
>> +               pt = pt + 5; // Reserved fields set to 0
>> +       }
> Yikes. If your structure reflected the hardware, then you could use
> actual members. Barring that, you could create and use enum values for
> the descriptor members. The way it stands this is very brittle and
> full of magic values and offsets.
Will get rid of this code here.
>> +
>> +       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
>> +               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
> I'm not sure this works out of the box, especially if what you're
> writing isn't exactly the descriptor size (which it might be in your
> tests, but might suddenly start failing later). Please consider
> absorbing my change [1] which properly refactors the function for
> reading and writing.
I know the desc size (saved in buff_len). And via configfs we are 
passing same size of configuration descriptor.
So it works fine.
>
>> +
>> +       if (ret) {
>> +               dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x ret %d\n",
>> +               __func__, QUERY_DESC_IDN_CONFIGURATION,
>> +               UPIU_QUERY_OPCODE_WRITE_DESC, ret);
>> +               goto out;
>> +       }
>> +
>> +       if (cfg->bConfigDescrLock == 1) {
>> +               ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> +               QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
>> +               if (ret)
>> +                       dev_err(hba->dev, "%s: Failed writing bConfigDescrLock %d\n",
>> +                               __func__, ret);
>> +       }
> It might be better not to merge this functionality in with the act of
> writing provisioning data, and instead make the sysfs attributes
> writable like [2].
Will remove writing decriptor lock here (as it is not a part of 
configuration descriptor anyway and also we wont be passing it via 
configfs).

>
>> +
>> +out:
>> +       scsi_unblock_requests(hba->host);
>> +       pm_runtime_put_sync(hba->dev);
>> +
>> +       return ret;
>> +}
>> +
>> +/**
>>    * ufshcd_probe_hba - probe hba to detect device and initialize
>>    * @hba: per-adapter instance
>>    *
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 101a75c..0e6bf30 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -549,6 +549,7 @@ struct ufs_hba {
>>          unsigned int irq;
>>          bool is_irq_enabled;
>>          u32 dev_ref_clk_freq;
>> +       struct ufs_config_descr cfgs;
> How come you store this here, rather than as a local in
> ufshcd_do_config_device()?
Will remove this entry.
>>          /* Interrupt aggregation support is broken */
>>          #define UFSHCD_QUIRK_BROKEN_INTR_AGGR                   0x1
>> @@ -867,6 +868,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index,
>>
>>   int ufshcd_hold(struct ufs_hba *hba, bool async);
>>   void ufshcd_release(struct ufs_hba *hba);
>> +int ufshcd_do_config_device(struct ufs_hba *hba);
>>
>>   int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
>>          int *desc_length);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> [1] https://patchwork.kernel.org/patch/10467669/
> [2] https://patchwork.kernel.org/patch/10467665/
Thanks,
Sayali

[PATCH V4 3/3] scsi: ufs: Add configfs support for ufs provisioning

[Sayali Lokhande]

Add configfs support to provision ufs device at runtime.
Usage:
echo <desc_buf> > /config/ufshcd/ufs_provision
To check provisioning status:
cat /config/ufshcd/ufs_provision
1 -> Success (Reboot device to check updated provisioning)

Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
---
 Documentation/ABI/testing/configfs-driver-ufs |  15 ++
 drivers/scsi/ufs/Makefile                     |   1 +
 drivers/scsi/ufs/ufs-configfs.c               | 198 ++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs.h                        |   2 +
 drivers/scsi/ufs/ufshcd.c                     |   2 +
 drivers/scsi/ufs/ufshcd.h                     |  18 +++
 6 files changed, 236 insertions(+)
 create mode 100644 Documentation/ABI/testing/configfs-driver-ufs
 create mode 100644 drivers/scsi/ufs/ufs-configfs.c

diff --git a/Documentation/ABI/testing/configfs-driver-ufs b/Documentation/ABI/testing/configfs-driver-ufs
new file mode 100644
index 0000000..f6ef38e
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-driver-ufs
@@ -0,0 +1,15 @@
+What:		/config/ufshcd/ufs_provision
+Date:		Jun 2018
+KernelVersion:	4.14
+Description:
+		This file shows the status of runtime ufs provisioning.
+		This can be used to provision ufs device if bConfigDescrLock is 0.
+		Configuration buffer needs to be written in space separated format
+		specificied as below:
+		echo <bNumberLU> <bBootEnable> <bDescrAccessEn> <bInitPowerMode>
+		<bHighPriorityLUN> <bSecureRemovalType> <bInitActiveICCLevel>
+		<wPeriodicRTCUpdate> <bConfigDescrLock> <LUNum> <bLUEnable>
+		<bBootLunID> <size_in_kb> <bDataReliability> <bLUWriteProtect>
+		<bMemoryType> <bLogicalBlockSize> <bProvisioningType>
+		<wContextCapabilities> <LUNtoGrow> <commit> <NumofLUNs>
+		> /config/ufshcd/ufs_provision
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 918f579..d438e74 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -5,5 +5,6 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-d
 obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
 obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
 ufshcd-core-objs := ufshcd.o ufs-sysfs.o
+obj-$(CONFIG_CONFIGFS_FS) += ufs-configfs.o
 obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
 obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
diff --git a/drivers/scsi/ufs/ufs-configfs.c b/drivers/scsi/ufs/ufs-configfs.c
new file mode 100644
index 0000000..614b017
--- /dev/null
+++ b/drivers/scsi/ufs/ufs-configfs.c
@@ -0,0 +1,198 @@
+/*
+ * Copyright (c) 2018, Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/string.h>
+#include <asm/unaligned.h>
+#include <linux/configfs.h>
+
+#include "ufs.h"
+#include "ufshcd.h"
+
+struct ufs_hba *hba;
+
+static ssize_t ufs_provision_show(struct config_item *item, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "provision_enabled = %x\n",
+		hba->provision_enabled);
+}
+
+ssize_t ufshcd_desc_configfs_store(const char *buf, size_t count)
+{
+	struct ufs_config_descr *cfg = &hba->cfgs;
+	char *strbuf;
+	char *strbuf_copy;
+	int desc_buf[count];
+	int *pt;
+	char *token;
+	int i, ret;
+	int value, commit = 0;
+	int num_luns = 0;
+	int KB_per_block = 4;
+
+	/* reserve one byte for null termination */
+	strbuf = kmalloc(count + 1, GFP_KERNEL);
+	if (!strbuf)
+		return -ENOMEM;
+
+	strbuf_copy = strbuf;
+	strlcpy(strbuf, buf, count + 1);
+	memset(desc_buf, 0, count);
+
+	/* Just return if bConfigDescrLock is already set */
+	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed reading bConfigDescrLock %d, cannot re-provision device!\n",
+			__func__, ret);
+		hba->provision_enabled = 0;
+		goto out;
+	}
+	if (cfg->bConfigDescrLock == 1) {
+		dev_err(hba->dev, "%s: bConfigDescrLock already set to %u, cannot re-provision device!\n",
+		__func__, cfg->bConfigDescrLock);
+		hba->provision_enabled = 0;
+		goto out;
+	}
+
+	for (i = 0; i < count; i++) {
+		token = strsep(&strbuf, " ");
+		if (!token && i) {
+			num_luns = desc_buf[i-1];
+			dev_dbg(hba->dev, "%s: token %s, num_luns %d\n",
+				__func__, token, num_luns);
+			if (num_luns > 8) {
+				dev_err(hba->dev, "%s: Invalid num_luns %d\n",
+				__func__, num_luns);
+				hba->provision_enabled = 0;
+				goto out;
+			}
+			break;
+		}
+
+		ret = kstrtoint(token, 0, &value);
+		if (ret) {
+			dev_err(hba->dev, "%s: kstrtoint failed %d %s\n",
+				__func__, ret, token);
+			break;
+		}
+		desc_buf[i] = value;
+		dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);
+	}
+
+	/* Fill in the descriptors with parsed configuration data */
+	pt = desc_buf;
+	cfg->bNumberLU = *pt++;
+	cfg->bBootEnable = *pt++;
+	cfg->bDescrAccessEn = *pt++;
+	cfg->bInitPowerMode = *pt++;
+	cfg->bHighPriorityLUN = *pt++;
+	cfg->bSecureRemovalType = *pt++;
+	cfg->bInitActiveICCLevel = *pt++;
+	cfg->wPeriodicRTCUpdate = *pt++;
+	cfg->bConfigDescrLock = *pt++;
+	dev_dbg(hba->dev, "%s: %u %u %u %u %u %u %u %u %u\n", __func__,
+	cfg->bNumberLU, cfg->bBootEnable, cfg->bDescrAccessEn,
+	cfg->bInitPowerMode, cfg->bHighPriorityLUN, cfg->bSecureRemovalType,
+	cfg->bInitActiveICCLevel, cfg->wPeriodicRTCUpdate,
+	cfg->bConfigDescrLock);
+
+	for (i = 0; i < num_luns; i++) {
+		cfg->unit[i].LUNum = *pt++;
+		cfg->unit[i].bLUEnable = *pt++;
+		cfg->unit[i].bBootLunID = *pt++;
+		/* dNumAllocUnits = size_in_kb/KB_per_block */
+		cfg->unit[i].dNumAllocUnits = (u32)(*pt++ / KB_per_block);
+		cfg->unit[i].bDataReliability = *pt++;
+		cfg->unit[i].bLUWriteProtect = *pt++;
+		cfg->unit[i].bMemoryType = *pt++;
+		cfg->unit[i].bLogicalBlockSize = *pt++;
+		cfg->unit[i].bProvisioningType = *pt++;
+		cfg->unit[i].wContextCapabilities = *pt++;
+	}
+
+	cfg->lun_to_grow = *pt++;
+	commit = *pt++;
+	cfg->num_luns = *pt;
+	dev_dbg(hba->dev, "%s: lun_to_grow %u, commit %u num_luns %u\n",
+		__func__, cfg->lun_to_grow, commit, cfg->num_luns);
+	if (commit == 1) {
+		ret = ufshcd_do_config_device(hba);
+		if (!ret) {
+			hba->provision_enabled = 1;
+			dev_err(hba->dev,
+			"%s: UFS Provisioning completed,num_luns %u, reboot now !\n",
+			__func__, cfg->num_luns);
+		}
+	} else
+		dev_err(hba->dev, "%s: Invalid commit %u\n", __func__, commit);
+out:
+	kfree(strbuf_copy);
+	return count;
+}
+
+static ssize_t ufs_provision_store(struct config_item *item,
+		const char *buf, size_t count)
+{
+	return ufshcd_desc_configfs_store(buf, count);
+}
+
+static struct configfs_attribute ufshcd_attr_provision = {
+	.ca_name	= "ufs_provision",
+	.ca_mode	= S_IRUGO | S_IWUGO,
+	.ca_owner	= THIS_MODULE,
+	.show		= ufs_provision_show,
+	.store		= ufs_provision_store,
+};
+
+static struct configfs_attribute *ufshcd_attrs[] = {
+	&ufshcd_attr_provision,
+	NULL,
+};
+
+static struct config_item_type ufscfg_type = {
+	.ct_attrs	= ufshcd_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct configfs_subsystem ufscfg_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "ufshcd",
+			.ci_type = &ufscfg_type,
+		},
+	},
+};
+
+int ufshcd_configfs_init(struct ufs_hba *hba_ufs)
+{
+	int ret;
+	struct configfs_subsystem *subsys = &ufscfg_subsys;
+
+	hba = hba_ufs;
+	config_group_init(&subsys->su_group);
+	mutex_init(&subsys->su_mutex);
+	ret = configfs_register_subsystem(subsys);
+	if (ret) {
+		pr_err("Error %d while registering subsystem %s\n",
+		       ret,
+		       subsys->su_group.cg_item.ci_namebuf);
+	}
+	return ret;
+}
+
+void ufshcd_configfs_exit(void)
+{
+	configfs_unregister_subsystem(&ufscfg_subsys);
+}
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 1f99904..0b497fc 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -427,6 +427,7 @@ enum {
 };
 
 struct ufs_unit_desc {
+	u8	   LUNum;
 	u8     bLUEnable;              /* 1 for enabled LU */
 	u8     bBootLunID;             /* 0 for using this LU for boot */
 	u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
@@ -451,6 +452,7 @@ struct ufs_config_descr {
 	u32    qVendorConfigCode;      /* Vendor specific configuration code */
 	struct ufs_unit_desc unit[8];
 	u8	lun_to_grow;
+	u8 num_luns;
 };
 
 /* Task management service response */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 370a7c6..e980d5a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7892,6 +7892,7 @@ int ufshcd_shutdown(struct ufs_hba *hba)
 void ufshcd_remove(struct ufs_hba *hba)
 {
 	ufs_sysfs_remove_nodes(hba->dev);
+	ufshcd_configfs_exit();
 	scsi_remove_host(hba->host);
 	/* disable interrupts */
 	ufshcd_disable_intr(hba, hba->intr_mask);
@@ -8145,6 +8146,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	async_schedule(ufshcd_async_scan, hba);
 	ufs_sysfs_add_nodes(hba->dev);
+	ufshcd_configfs_init(hba);
 
 	return 0;
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 0e6bf30..7d2fa89 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -41,6 +41,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/configfs.h>
 #include <linux/io.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
@@ -550,6 +551,7 @@ struct ufs_hba {
 	bool is_irq_enabled;
 	u32 dev_ref_clk_freq;
 	struct ufs_config_descr cfgs;
+	bool provision_enabled;
 
 	/* Interrupt aggregation support is broken */
 	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
@@ -869,6 +871,22 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index,
 int ufshcd_hold(struct ufs_hba *hba, bool async);
 void ufshcd_release(struct ufs_hba *hba);
 int ufshcd_do_config_device(struct ufs_hba *hba);
+/* Expose UFS configfs API's */
+ssize_t ufshcd_desc_configfs_store(const char *buf, size_t count);
+
+#ifdef CONFIG_CONFIGFS_FS
+int ufshcd_configfs_init(struct ufs_hba *hba_ufs);
+void ufshcd_configfs_exit(void);
+#else /* CONFIG_CONFIGFS_FS */
+int ufshcd_configfs_init(struct ufs_hba *hba_ufs)
+{
+	return 0;
+}
+
+void ufshcd_configfs_exit(void)
+{
+}
+#endif /* CONFIG_CONFIGFS_FS */
 
 int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
 	int *desc_length);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V4 3/3] scsi: ufs: Add configfs support for ufs provisioning

[kbuild test robot]

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

Hi Sayali,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sayali-Lokhande/Add-ufs-provisioning-support-in-driver/20180626-120644
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-kexec (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "ufshcd_configfs_init" [drivers/scsi/ufs/ufshcd-core.ko] undefined!
>> ERROR: "ufshcd_configfs_exit" [drivers/scsi/ufs/ufshcd-core.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26455 bytes --]

Re: [PATCH V4 3/3] scsi: ufs: Add configfs support for ufs provisioning

[Evan Green]

Hi Sayali,

On Mon, Jun 25, 2018 at 9:04 PM Sayali Lokhande <sayalil@codeaurora.org> wrote:
>
> Add configfs support to provision ufs device at runtime.
> Usage:
> echo <desc_buf> > /config/ufshcd/ufs_provision
> To check provisioning status:
> cat /config/ufshcd/ufs_provision
> 1 -> Success (Reboot device to check updated provisioning)

This commit message could contain a bit more detail, including
motivation and what goes into desc_buf. Also, the description of
reading ufs_provision isn't really correct, is it? 1 doesn't indicate
success, 1 just indicates bConfigDescrLock's value.

>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  Documentation/ABI/testing/configfs-driver-ufs |  15 ++
>  drivers/scsi/ufs/Makefile                     |   1 +
>  drivers/scsi/ufs/ufs-configfs.c               | 198 ++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufs.h                        |   2 +
>  drivers/scsi/ufs/ufshcd.c                     |   2 +
>  drivers/scsi/ufs/ufshcd.h                     |  18 +++
>  6 files changed, 236 insertions(+)
>  create mode 100644 Documentation/ABI/testing/configfs-driver-ufs
>  create mode 100644 drivers/scsi/ufs/ufs-configfs.c
>
> diff --git a/Documentation/ABI/testing/configfs-driver-ufs b/Documentation/ABI/testing/configfs-driver-ufs
> new file mode 100644
> index 0000000..f6ef38e
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-driver-ufs
> @@ -0,0 +1,15 @@
> +What:          /config/ufshcd/ufs_provision
> +Date:          Jun 2018
> +KernelVersion: 4.14
> +Description:
> +               This file shows the status of runtime ufs provisioning.
> +               This can be used to provision ufs device if bConfigDescrLock is 0.
> +               Configuration buffer needs to be written in space separated format
> +               specificied as below:
> +               echo <bNumberLU> <bBootEnable> <bDescrAccessEn> <bInitPowerMode>
> +               <bHighPriorityLUN> <bSecureRemovalType> <bInitActiveICCLevel>
> +               <wPeriodicRTCUpdate> <bConfigDescrLock> <LUNum> <bLUEnable>
> +               <bBootLunID> <size_in_kb> <bDataReliability> <bLUWriteProtect>
> +               <bMemoryType> <bLogicalBlockSize> <bProvisioningType>
> +               <wContextCapabilities> <LUNtoGrow> <commit> <NumofLUNs>
> +               > /config/ufshcd/ufs_provision
> diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
> index 918f579..d438e74 100644
> --- a/drivers/scsi/ufs/Makefile
> +++ b/drivers/scsi/ufs/Makefile
> @@ -5,5 +5,6 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-d
>  obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
>  obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
>  ufshcd-core-objs := ufshcd.o ufs-sysfs.o
> +obj-$(CONFIG_CONFIGFS_FS) += ufs-configfs.o
>  obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
>  obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
> diff --git a/drivers/scsi/ufs/ufs-configfs.c b/drivers/scsi/ufs/ufs-configfs.c
> new file mode 100644
> index 0000000..614b017
> --- /dev/null
> +++ b/drivers/scsi/ufs/ufs-configfs.c
> @@ -0,0 +1,198 @@
> +/*
> + * Copyright (c) 2018, Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */

I believe there's a new SPDX license identification that they prefer
you to use, at least according to:
https://lwn.net/Articles/739183/

> +
> +#include <linux/err.h>
> +#include <linux/string.h>
> +#include <asm/unaligned.h>
> +#include <linux/configfs.h>

These should be alphabetized.

> +
> +#include "ufs.h"
> +#include "ufshcd.h"
> +
> +struct ufs_hba *hba;

How does this work if there is more than one UFS controller in the
system? Given that there are both UFS cards and embedded UFS chips, I
think multiple controllers needs to be supported.

> +
> +static ssize_t ufs_provision_show(struct config_item *item, char *buf)
> +{
> +       return snprintf(buf, PAGE_SIZE, "provision_enabled = %x\n",
> +               hba->provision_enabled);

Why can't this show the current configuration, barring some of the
weirder parameters like lun_to_grow, which I have comments about
below. I'm not sure provision_enabled is very useful, given that we
can already get at this attribute via sysfs.

> +}
> +
> +ssize_t ufshcd_desc_configfs_store(const char *buf, size_t count)
> +{
> +       struct ufs_config_descr *cfg = &hba->cfgs;
> +       char *strbuf;
> +       char *strbuf_copy;
> +       int desc_buf[count];

I believe with configfs, "count" can be up to PAGE_SIZE, which would
mean usermode could size this array at 16K. That's too big, especially
since you already know the maximum number of ints you're willing to
take in, and it's not anywhere near that.

> +       int *pt;
> +       char *token;
> +       int i, ret;
> +       int value, commit = 0;
> +       int num_luns = 0;
> +       int KB_per_block = 4;

What is this? Is this really fixed across all UFS devices?

> +
> +       /* reserve one byte for null termination */
> +       strbuf = kmalloc(count + 1, GFP_KERNEL);
> +       if (!strbuf)
> +               return -ENOMEM;
> +
> +       strbuf_copy = strbuf;
> +       strlcpy(strbuf, buf, count + 1);
> +       memset(desc_buf, 0, count);

This is zeroing count, which represents the number of characters
coming in, but desc_buf is an array of ints.

> +
> +       /* Just return if bConfigDescrLock is already set */
> +       ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +               QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
> +       if (ret) {
> +               dev_err(hba->dev, "%s: Failed reading bConfigDescrLock %d, cannot re-provision device!\n",
> +                       __func__, ret);
> +               hba->provision_enabled = 0;
> +               goto out;
> +       }
> +       if (cfg->bConfigDescrLock == 1) {

This might just be my paranoia, but I generally prefer checks against
zero, rather than checks specifically for one.

> +               dev_err(hba->dev, "%s: bConfigDescrLock already set to %u, cannot re-provision device!\n",
> +               __func__, cfg->bConfigDescrLock);
> +               hba->provision_enabled = 0;
> +               goto out;
> +       }
> +
> +       for (i = 0; i < count; i++) {
> +               token = strsep(&strbuf, " ");
> +               if (!token && i) {
> +                       num_luns = desc_buf[i-1];
> +                       dev_dbg(hba->dev, "%s: token %s, num_luns %d\n",
> +                               __func__, token, num_luns);
> +                       if (num_luns > 8) {

This is a magic number. Please make a define for this number. Then use
that define to come up with a maximum size for desc_buf, rather than
"count". Then you can iterate "i" up to the appropriate number based
on the number of luns, rather than up to the number of characters in
the string.

> +                               dev_err(hba->dev, "%s: Invalid num_luns %d\n",
> +                               __func__, num_luns);
> +                               hba->provision_enabled = 0;
> +                               goto out;
> +                       }

I don't love that there's a num_luns parameter being fed in here at
all. It would be better in my opinion if you could just faithfully
pass along the members of the configuration descriptor directly,
rather than having additional "features" like num_luns, commit, and
lun_to_grow.

> +                       break;
> +               }
> +
> +               ret = kstrtoint(token, 0, &value);
> +               if (ret) {
> +                       dev_err(hba->dev, "%s: kstrtoint failed %d %s\n",
> +                               __func__, ret, token);
> +                       break;
> +               }
> +               desc_buf[i] = value;
> +               dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);
> +       }
> +
> +       /* Fill in the descriptors with parsed configuration data */
> +       pt = desc_buf;
> +       cfg->bNumberLU = *pt++;
> +       cfg->bBootEnable = *pt++;
> +       cfg->bDescrAccessEn = *pt++;
> +       cfg->bInitPowerMode = *pt++;
> +       cfg->bHighPriorityLUN = *pt++;
> +       cfg->bSecureRemovalType = *pt++;
> +       cfg->bInitActiveICCLevel = *pt++;
> +       cfg->wPeriodicRTCUpdate = *pt++;
> +       cfg->bConfigDescrLock = *pt++;
> +       dev_dbg(hba->dev, "%s: %u %u %u %u %u %u %u %u %u\n", __func__,
> +       cfg->bNumberLU, cfg->bBootEnable, cfg->bDescrAccessEn,
> +       cfg->bInitPowerMode, cfg->bHighPriorityLUN, cfg->bSecureRemovalType,
> +       cfg->bInitActiveICCLevel, cfg->wPeriodicRTCUpdate,
> +       cfg->bConfigDescrLock);
> +
> +       for (i = 0; i < num_luns; i++) {
> +               cfg->unit[i].LUNum = *pt++;
> +               cfg->unit[i].bLUEnable = *pt++;
> +               cfg->unit[i].bBootLunID = *pt++;
> +               /* dNumAllocUnits = size_in_kb/KB_per_block */
> +               cfg->unit[i].dNumAllocUnits = (u32)(*pt++ / KB_per_block);

It's awkward that pt is an int, since you then divide by 4. That means
you lose two bits at the top. What if you want a size that includes
those two bits? Also as per my comment above, if KB_per_block might be
values other than 4, then you might lose even more bits. Perhaps this
should just be set to *pt++ directly, rather than the mystery divide.
It will be up to user mode to read the geometry descriptor to figure
out how bytes correspond to allocation units.

> +               cfg->unit[i].bDataReliability = *pt++;
> +               cfg->unit[i].bLUWriteProtect = *pt++;
> +               cfg->unit[i].bMemoryType = *pt++;
> +               cfg->unit[i].bLogicalBlockSize = *pt++;
> +               cfg->unit[i].bProvisioningType = *pt++;
> +               cfg->unit[i].wContextCapabilities = *pt++;
> +       }

Do you validate that the number of ints you received corresponds to
the number of LUNs, or does this just put uninitialized kernel stack
in here? Oh, right, above you attempted to zero out desc_buf. What
about cfg->unit[i] for i > num_luns?

> +
> +       cfg->lun_to_grow = *pt++;
> +       commit = *pt++;
> +       cfg->num_luns = *pt;
> +       dev_dbg(hba->dev, "%s: lun_to_grow %u, commit %u num_luns %u\n",
> +               __func__, cfg->lun_to_grow, commit, cfg->num_luns);
> +       if (commit == 1) {

Is this a common thing in configfs? Why do we need this dry run
feature, seeing as there's no real validation going on in this
function anyway.

> +               ret = ufshcd_do_config_device(hba);
> +               if (!ret) {
> +                       hba->provision_enabled = 1;
> +                       dev_err(hba->dev,
> +                       "%s: UFS Provisioning completed,num_luns %u, reboot now !\n",
> +                       __func__, cfg->num_luns);
> +               }
> +       } else
> +               dev_err(hba->dev, "%s: Invalid commit %u\n", __func__, commit);
> +out:
> +       kfree(strbuf_copy);
> +       return count;
> +}
> +
> +static ssize_t ufs_provision_store(struct config_item *item,
> +               const char *buf, size_t count)
> +{
> +       return ufshcd_desc_configfs_store(buf, count);
> +}
> +
> +static struct configfs_attribute ufshcd_attr_provision = {
> +       .ca_name        = "ufs_provision",
> +       .ca_mode        = S_IRUGO | S_IWUGO,
> +       .ca_owner       = THIS_MODULE,
> +       .show           = ufs_provision_show,
> +       .store          = ufs_provision_store,
> +};
> +
> +static struct configfs_attribute *ufshcd_attrs[] = {
> +       &ufshcd_attr_provision,
> +       NULL,
> +};
> +
> +static struct config_item_type ufscfg_type = {
> +       .ct_attrs       = ufshcd_attrs,
> +       .ct_owner       = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem ufscfg_subsys = {
> +       .su_group = {
> +               .cg_item = {
> +                       .ci_namebuf = "ufshcd",
> +                       .ci_type = &ufscfg_type,
> +               },
> +       },
> +};
> +
> +int ufshcd_configfs_init(struct ufs_hba *hba_ufs)
> +{
> +       int ret;
> +       struct configfs_subsystem *subsys = &ufscfg_subsys;
> +
> +       hba = hba_ufs;
> +       config_group_init(&subsys->su_group);
> +       mutex_init(&subsys->su_mutex);
> +       ret = configfs_register_subsystem(subsys);
> +       if (ret) {
> +               pr_err("Error %d while registering subsystem %s\n",
> +                      ret,
> +                      subsys->su_group.cg_item.ci_namebuf);
> +       }
> +       return ret;
> +}
> +
> +void ufshcd_configfs_exit(void)
> +{
> +       configfs_unregister_subsystem(&ufscfg_subsys);
> +}
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 1f99904..0b497fc 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -427,6 +427,7 @@ enum {
>  };
>
>  struct ufs_unit_desc {
> +       u8         LUNum;
>         u8     bLUEnable;              /* 1 for enabled LU */
>         u8     bBootLunID;             /* 0 for using this LU for boot */
>         u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
> @@ -451,6 +452,7 @@ struct ufs_config_descr {
>         u32    qVendorConfigCode;      /* Vendor specific configuration code */
>         struct ufs_unit_desc unit[8];
>         u8      lun_to_grow;
> +       u8 num_luns;
>  };

I don't like that these structs seem to be a blend of hardware and
software definitions. For the most part they match the hardware spec,
but then there are these random software accounting members sprinkled
in that break that. It would be better if the hardware structures
could be their own thing, and then additional structures can be
created if software needs its own accounting. But I'm also not a fan
of any of the software members here (num_luns, lun_to_grow, and
LUNum), and I'm hoping we can get rid of them altogether by instead
providing a more direct configfs interface to the config desciptor.

>
>  /* Task management service response */
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 370a7c6..e980d5a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7892,6 +7892,7 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>  void ufshcd_remove(struct ufs_hba *hba)
>  {
>         ufs_sysfs_remove_nodes(hba->dev);
> +       ufshcd_configfs_exit();
>         scsi_remove_host(hba->host);
>         /* disable interrupts */
>         ufshcd_disable_intr(hba, hba->intr_mask);
> @@ -8145,6 +8146,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>
>         async_schedule(ufshcd_async_scan, hba);
>         ufs_sysfs_add_nodes(hba->dev);
> +       ufshcd_configfs_init(hba);
>
>         return 0;
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 0e6bf30..7d2fa89 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -41,6 +41,7 @@
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> +#include <linux/configfs.h>
>  #include <linux/io.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> @@ -550,6 +551,7 @@ struct ufs_hba {
>         bool is_irq_enabled;
>         u32 dev_ref_clk_freq;
>         struct ufs_config_descr cfgs;
> +       bool provision_enabled;
>
>         /* Interrupt aggregation support is broken */
>         #define UFSHCD_QUIRK_BROKEN_INTR_AGGR                   0x1
> @@ -869,6 +871,22 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index,
>  int ufshcd_hold(struct ufs_hba *hba, bool async);
>  void ufshcd_release(struct ufs_hba *hba);
>  int ufshcd_do_config_device(struct ufs_hba *hba);
> +/* Expose UFS configfs API's */
> +ssize_t ufshcd_desc_configfs_store(const char *buf, size_t count);
> +
> +#ifdef CONFIG_CONFIGFS_FS
> +int ufshcd_configfs_init(struct ufs_hba *hba_ufs);
> +void ufshcd_configfs_exit(void);
> +#else /* CONFIG_CONFIGFS_FS */
> +int ufshcd_configfs_init(struct ufs_hba *hba_ufs)
> +{
> +       return 0;
> +}
> +
> +void ufshcd_configfs_exit(void)
> +{
> +}
> +#endif /* CONFIG_CONFIGFS_FS */
>
>  int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
>         int *desc_length);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH V4 3/3] scsi: ufs: Add configfs support for ufs provisioning

[Sayali Lokhande]

Hi Evan,

On 6/28/2018 12:09 AM, Evan Green wrote:
> Hi Sayali,
>
> On Mon, Jun 25, 2018 at 9:04 PM Sayali Lokhande <sayalil@codeaurora.org> wrote:
>> Add configfs support to provision ufs device at runtime.
>> Usage:
>> echo <desc_buf> > /config/ufshcd/ufs_provision
>> To check provisioning status:
>> cat /config/ufshcd/ufs_provision
>> 1 -> Success (Reboot device to check updated provisioning)
> This commit message could contain a bit more detail, including
> motivation and what goes into desc_buf. Also, the description of
> reading ufs_provision isn't really correct, is it? 1 doesn't indicate
> success, 1 just indicates bConfigDescrLock's value.
Agreed. Will add more details and also update code to show current 
configuration descriptor on reading ufs_provision.
>> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
>> ---
>>   Documentation/ABI/testing/configfs-driver-ufs |  15 ++
>>   drivers/scsi/ufs/Makefile                     |   1 +
>>   drivers/scsi/ufs/ufs-configfs.c               | 198 ++++++++++++++++++++++++++
>>   drivers/scsi/ufs/ufs.h                        |   2 +
>>   drivers/scsi/ufs/ufshcd.c                     |   2 +
>>   drivers/scsi/ufs/ufshcd.h                     |  18 +++
>>   6 files changed, 236 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/configfs-driver-ufs
>>   create mode 100644 drivers/scsi/ufs/ufs-configfs.c
>>
>> diff --git a/Documentation/ABI/testing/configfs-driver-ufs b/Documentation/ABI/testing/configfs-driver-ufs
>> new file mode 100644
>> index 0000000..f6ef38e
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/configfs-driver-ufs
>> @@ -0,0 +1,15 @@
>> +What:          /config/ufshcd/ufs_provision
>> +Date:          Jun 2018
>> +KernelVersion: 4.14
>> +Description:
>> +               This file shows the status of runtime ufs provisioning.
>> +               This can be used to provision ufs device if bConfigDescrLock is 0.
>> +               Configuration buffer needs to be written in space separated format
>> +               specificied as below:
>> +               echo <bNumberLU> <bBootEnable> <bDescrAccessEn> <bInitPowerMode>
>> +               <bHighPriorityLUN> <bSecureRemovalType> <bInitActiveICCLevel>
>> +               <wPeriodicRTCUpdate> <bConfigDescrLock> <LUNum> <bLUEnable>
>> +               <bBootLunID> <size_in_kb> <bDataReliability> <bLUWriteProtect>
>> +               <bMemoryType> <bLogicalBlockSize> <bProvisioningType>
>> +               <wContextCapabilities> <LUNtoGrow> <commit> <NumofLUNs>
>> +               > /config/ufshcd/ufs_provision
>> diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
>> index 918f579..d438e74 100644
>> --- a/drivers/scsi/ufs/Makefile
>> +++ b/drivers/scsi/ufs/Makefile
>> @@ -5,5 +5,6 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-d
>>   obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
>>   obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
>>   ufshcd-core-objs := ufshcd.o ufs-sysfs.o
>> +obj-$(CONFIG_CONFIGFS_FS) += ufs-configfs.o
>>   obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
>>   obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
>> diff --git a/drivers/scsi/ufs/ufs-configfs.c b/drivers/scsi/ufs/ufs-configfs.c
>> new file mode 100644
>> index 0000000..614b017
>> --- /dev/null
>> +++ b/drivers/scsi/ufs/ufs-configfs.c
>> @@ -0,0 +1,198 @@
>> +/*
>> + * Copyright (c) 2018, Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
> I believe there's a new SPDX license identification that they prefer
> you to use, at least according to:
> https://lwn.net/Articles/739183/
Agreed. Will use correct SPDX License format.
>> +
>> +#include <linux/err.h>
>> +#include <linux/string.h>
>> +#include <asm/unaligned.h>
>> +#include <linux/configfs.h>
> These should be alphabetized.
Agreed. Will update.
>> +
>> +#include "ufs.h"
>> +#include "ufshcd.h"
>> +
>> +struct ufs_hba *hba;
> How does this work if there is more than one UFS controller in the
> system? Given that there are both UFS cards and embedded UFS chips, I
> think multiple controllers needs to be supported.
Agreed. I will remove this global variable and instead pass it along in 
store/show() api's.
Currently I am anyway passing hba as part of ufshcd_configfs_init(hba); 
Will try to use same.
>> +
>> +static ssize_t ufs_provision_show(struct config_item *item, char *buf)
>> +{
>> +       return snprintf(buf, PAGE_SIZE, "provision_enabled = %x\n",
>> +               hba->provision_enabled);
> Why can't this show the current configuration, barring some of the
> weirder parameters like lun_to_grow, which I have comments about
> below. I'm not sure provision_enabled is very useful, given that we
> can already get at this attribute via sysfs.
Agreed. Will get rid of extra added parameters and will update this to 
show current configuration desc.
>> +}
>> +
>> +ssize_t ufshcd_desc_configfs_store(const char *buf, size_t count)
>> +{
>> +       struct ufs_config_descr *cfg = &hba->cfgs;
>> +       char *strbuf;
>> +       char *strbuf_copy;
>> +       int desc_buf[count];
> I believe with configfs, "count" can be up to PAGE_SIZE, which would
> mean usermode could size this array at 16K. That's too big, especially
> since you already know the maximum number of ints you're willing to
> take in, and it's not anywhere near that.
Agreed. Will replace count with actual size of configuration descriptor.
>> +       int *pt;
>> +       char *token;
>> +       int i, ret;
>> +       int value, commit = 0;
>> +       int num_luns = 0;
>> +       int KB_per_block = 4;
> What is this? Is this really fixed across all UFS devices?
Will get rid of these extra parameters added. and instead try to pass 
exact configuration descriptor parameters(as in spec).
>
>> +
>> +       /* reserve one byte for null termination */
>> +       strbuf = kmalloc(count + 1, GFP_KERNEL);
>> +       if (!strbuf)
>> +               return -ENOMEM;
>> +
>> +       strbuf_copy = strbuf;
>> +       strlcpy(strbuf, buf, count + 1);
>> +       memset(desc_buf, 0, count);
> This is zeroing count, which represents the number of characters
> coming in, but desc_buf is an array of ints.
Will update this.
>
>> +
>> +       /* Just return if bConfigDescrLock is already set */
>> +       ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> +               QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
>> +       if (ret) {
>> +               dev_err(hba->dev, "%s: Failed reading bConfigDescrLock %d, cannot re-provision device!\n",
>> +                       __func__, ret);
>> +               hba->provision_enabled = 0;
>> +               goto out;
>> +       }
>> +       if (cfg->bConfigDescrLock == 1) {
> This might just be my paranoia, but I generally prefer checks against
> zero, rather than checks specifically for one.
Ok . Will update it.
>
>> +               dev_err(hba->dev, "%s: bConfigDescrLock already set to %u, cannot re-provision device!\n",
>> +               __func__, cfg->bConfigDescrLock);
>> +               hba->provision_enabled = 0;
>> +               goto out;
>> +       }
>> +
>> +       for (i = 0; i < count; i++) {
>> +               token = strsep(&strbuf, " ");
>> +               if (!token && i) {
>> +                       num_luns = desc_buf[i-1];
>> +                       dev_dbg(hba->dev, "%s: token %s, num_luns %d\n",
>> +                               __func__, token, num_luns);
>> +                       if (num_luns > 8) {
> This is a magic number. Please make a define for this number. Then use
> that define to come up with a maximum size for desc_buf, rather than
> "count". Then you can iterate "i" up to the appropriate number based
> on the number of luns, rather than up to the number of characters in
> the string.
Agreed. I already know size of configuration desc. Will use same instead 
of "count".
>
>> +                               dev_err(hba->dev, "%s: Invalid num_luns %d\n",
>> +                               __func__, num_luns);
>> +                               hba->provision_enabled = 0;
>> +                               goto out;
>> +                       }
> I don't love that there's a num_luns parameter being fed in here at
> all. It would be better in my opinion if you could just faithfully
> pass along the members of the configuration descriptor directly,
> rather than having additional "features" like num_luns, commit, and
> lun_to_grow.
Agreed. Will update code to faithfully accept the entire configuration 
descriptor passed and will remove all extra sw params added (like 
num_luns, commit etc).
>> +                       break;
>> +               }
>> +
>> +               ret = kstrtoint(token, 0, &value);
>> +               if (ret) {
>> +                       dev_err(hba->dev, "%s: kstrtoint failed %d %s\n",
>> +                               __func__, ret, token);
>> +                       break;
>> +               }
>> +               desc_buf[i] = value;
>> +               dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);
>> +       }
>> +
>> +       /* Fill in the descriptors with parsed configuration data */
>> +       pt = desc_buf;
>> +       cfg->bNumberLU = *pt++;
>> +       cfg->bBootEnable = *pt++;
>> +       cfg->bDescrAccessEn = *pt++;
>> +       cfg->bInitPowerMode = *pt++;
>> +       cfg->bHighPriorityLUN = *pt++;
>> +       cfg->bSecureRemovalType = *pt++;
>> +       cfg->bInitActiveICCLevel = *pt++;
>> +       cfg->wPeriodicRTCUpdate = *pt++;
>> +       cfg->bConfigDescrLock = *pt++;
>> +       dev_dbg(hba->dev, "%s: %u %u %u %u %u %u %u %u %u\n", __func__,
>> +       cfg->bNumberLU, cfg->bBootEnable, cfg->bDescrAccessEn,
>> +       cfg->bInitPowerMode, cfg->bHighPriorityLUN, cfg->bSecureRemovalType,
>> +       cfg->bInitActiveICCLevel, cfg->wPeriodicRTCUpdate,
>> +       cfg->bConfigDescrLock);
>> +
>> +       for (i = 0; i < num_luns; i++) {
>> +               cfg->unit[i].LUNum = *pt++;
>> +               cfg->unit[i].bLUEnable = *pt++;
>> +               cfg->unit[i].bBootLunID = *pt++;
>> +               /* dNumAllocUnits = size_in_kb/KB_per_block */
>> +               cfg->unit[i].dNumAllocUnits = (u32)(*pt++ / KB_per_block);
> It's awkward that pt is an int, since you then divide by 4. That means
> you lose two bits at the top. What if you want a size that includes
> those two bits? Also as per my comment above, if KB_per_block might be
> values other than 4, then you might lose even more bits. Perhaps this
> should just be set to *pt++ directly, rather than the mystery divide.
> It will be up to user mode to read the geometry descriptor to figure
> out how bytes correspond to allocation units.
Agreed. Will get rid of all these calculations from kernel code. Instead 
we can faithfully pass the configuration desc from configfs as is.
All these calculations (if required) can be done beforehand (in 
userspace or via some script).
>> +               cfg->unit[i].bDataReliability = *pt++;
>> +               cfg->unit[i].bLUWriteProtect = *pt++;
>> +               cfg->unit[i].bMemoryType = *pt++;
>> +               cfg->unit[i].bLogicalBlockSize = *pt++;
>> +               cfg->unit[i].bProvisioningType = *pt++;
>> +               cfg->unit[i].wContextCapabilities = *pt++;
>> +       }
> Do you validate that the number of ints you received corresponds to
> the number of LUNs, or does this just put uninitialized kernel stack
> in here? Oh, right, above you attempted to zero out desc_buf. What
> about cfg->unit[i] for i > num_luns?
Will get rid of these calculations here.
>
>> +
>> +       cfg->lun_to_grow = *pt++;
>> +       commit = *pt++;
>> +       cfg->num_luns = *pt;
>> +       dev_dbg(hba->dev, "%s: lun_to_grow %u, commit %u num_luns %u\n",
>> +               __func__, cfg->lun_to_grow, commit, cfg->num_luns);
>> +       if (commit == 1) {
> Is this a common thing in configfs? Why do we need this dry run
> feature, seeing as there's no real validation going on in this
> function anyway.
Will get rid of commit as check.
>> +               ret = ufshcd_do_config_device(hba);
>> +               if (!ret) {
>> +                       hba->provision_enabled = 1;
>> +                       dev_err(hba->dev,
>> +                       "%s: UFS Provisioning completed,num_luns %u, reboot now !\n",
>> +                       __func__, cfg->num_luns);
>> +               }
>> +       } else
>> +               dev_err(hba->dev, "%s: Invalid commit %u\n", __func__, commit);
>> +out:
>> +       kfree(strbuf_copy);
>> +       return count;
>> +}
>> +
>> +static ssize_t ufs_provision_store(struct config_item *item,
>> +               const char *buf, size_t count)
>> +{
>> +       return ufshcd_desc_configfs_store(buf, count);
>> +}
>> +
>> +static struct configfs_attribute ufshcd_attr_provision = {
>> +       .ca_name        = "ufs_provision",
>> +       .ca_mode        = S_IRUGO | S_IWUGO,
>> +       .ca_owner       = THIS_MODULE,
>> +       .show           = ufs_provision_show,
>> +       .store          = ufs_provision_store,
>> +};
>> +
>> +static struct configfs_attribute *ufshcd_attrs[] = {
>> +       &ufshcd_attr_provision,
>> +       NULL,
>> +};
>> +
>> +static struct config_item_type ufscfg_type = {
>> +       .ct_attrs       = ufshcd_attrs,
>> +       .ct_owner       = THIS_MODULE,
>> +};
>> +
>> +static struct configfs_subsystem ufscfg_subsys = {
>> +       .su_group = {
>> +               .cg_item = {
>> +                       .ci_namebuf = "ufshcd",
>> +                       .ci_type = &ufscfg_type,
>> +               },
>> +       },
>> +};
>> +
>> +int ufshcd_configfs_init(struct ufs_hba *hba_ufs)
>> +{
>> +       int ret;
>> +       struct configfs_subsystem *subsys = &ufscfg_subsys;
>> +
>> +       hba = hba_ufs;
>> +       config_group_init(&subsys->su_group);
>> +       mutex_init(&subsys->su_mutex);
>> +       ret = configfs_register_subsystem(subsys);
>> +       if (ret) {
>> +               pr_err("Error %d while registering subsystem %s\n",
>> +                      ret,
>> +                      subsys->su_group.cg_item.ci_namebuf);
>> +       }
>> +       return ret;
>> +}
>> +
>> +void ufshcd_configfs_exit(void)
>> +{
>> +       configfs_unregister_subsystem(&ufscfg_subsys);
>> +}
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index 1f99904..0b497fc 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -427,6 +427,7 @@ enum {
>>   };
>>
>>   struct ufs_unit_desc {
>> +       u8         LUNum;
>>          u8     bLUEnable;              /* 1 for enabled LU */
>>          u8     bBootLunID;             /* 0 for using this LU for boot */
>>          u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
>> @@ -451,6 +452,7 @@ struct ufs_config_descr {
>>          u32    qVendorConfigCode;      /* Vendor specific configuration code */
>>          struct ufs_unit_desc unit[8];
>>          u8      lun_to_grow;
>> +       u8 num_luns;
>>   };
> I don't like that these structs seem to be a blend of hardware and
> software definitions. For the most part they match the hardware spec,
> but then there are these random software accounting members sprinkled
> in that break that. It would be better if the hardware structures
> could be their own thing, and then additional structures can be
> created if software needs its own accounting. But I'm also not a fan
> of any of the software members here (num_luns, lun_to_grow, and
> LUNum), and I'm hoping we can get rid of them altogether by instead
> providing a more direct configfs interface to the config desciptor.
Will get rid of entire struct and provide more direct configfs interface 
to read/write
exact/entire configuration descriptor.
>>   /* Task management service response */
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 370a7c6..e980d5a 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -7892,6 +7892,7 @@ int ufshcd_shutdown(struct ufs_hba *hba)
>>   void ufshcd_remove(struct ufs_hba *hba)
>>   {
>>          ufs_sysfs_remove_nodes(hba->dev);
>> +       ufshcd_configfs_exit();
>>          scsi_remove_host(hba->host);
>>          /* disable interrupts */
>>          ufshcd_disable_intr(hba, hba->intr_mask);
>> @@ -8145,6 +8146,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>>
>>          async_schedule(ufshcd_async_scan, hba);
>>          ufs_sysfs_add_nodes(hba->dev);
>> +       ufshcd_configfs_init(hba);
>>
>>          return 0;
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 0e6bf30..7d2fa89 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -41,6 +41,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/init.h>
>>   #include <linux/interrupt.h>
>> +#include <linux/configfs.h>
>>   #include <linux/io.h>
>>   #include <linux/delay.h>
>>   #include <linux/slab.h>
>> @@ -550,6 +551,7 @@ struct ufs_hba {
>>          bool is_irq_enabled;
>>          u32 dev_ref_clk_freq;
>>          struct ufs_config_descr cfgs;
>> +       bool provision_enabled;
>>
>>          /* Interrupt aggregation support is broken */
>>          #define UFSHCD_QUIRK_BROKEN_INTR_AGGR                   0x1
>> @@ -869,6 +871,22 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index,
>>   int ufshcd_hold(struct ufs_hba *hba, bool async);
>>   void ufshcd_release(struct ufs_hba *hba);
>>   int ufshcd_do_config_device(struct ufs_hba *hba);
>> +/* Expose UFS configfs API's */
>> +ssize_t ufshcd_desc_configfs_store(const char *buf, size_t count);
>> +
>> +#ifdef CONFIG_CONFIGFS_FS
>> +int ufshcd_configfs_init(struct ufs_hba *hba_ufs);
>> +void ufshcd_configfs_exit(void);
>> +#else /* CONFIG_CONFIGFS_FS */
>> +int ufshcd_configfs_init(struct ufs_hba *hba_ufs)
>> +{
>> +       return 0;
>> +}
>> +
>> +void ufshcd_configfs_exit(void)
>> +{
>> +}
>> +#endif /* CONFIG_CONFIGFS_FS */
>>
>>   int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
>>          int *desc_length);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
Thanks,
Sayali