Skip to contentSkip to conversation

scsi: ufs: set the device reference clock setting

PATCH Superseded Sayali Lokhande 4 participants 9 messages Jun 26, 2018 – Jul 4, 2018 1 message not in the archive

Subsystems (3)
Device tree bindings SCSI UFS host controller

v4 of 15 Superseded Latest is v15 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Sayali Lokhande

author
#1 lore raw

PATCH v4 1/3 scsi: ufs: set the device reference clock setting

From: Subhash Jadavani <[email protected]>

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 <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Sayali Lokhande <[email protected]>
---
 .../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(+)
Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +7 -0
index c39dfef..4522434 100644
@@ -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 */
 	};
drivers/scsi/ufs/ufs.h +9 -0
index 14e5bf7..e15deb0 100644
@@ -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,
drivers/scsi/ufs/ufshcd-pltfrm.c +2 -0
index e82bde0..0953563 100644
@@ -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);
drivers/scsi/ufs/ufshcd.c +72 -0

84 lines not on this page · raw message

drivers/scsi/ufs/ufshcd.h +2 -0
index 8110dcd..101a75c 100644
@@ -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)
 {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#2 lore raw

PATCH v4 2/3 scsi: ufs: Add ufs provisioning support

In reply to a message not in the archive

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 <[email protected]>
---
 drivers/scsi/ufs/ufs.h    |  28 +++++++
 drivers/scsi/ufs/ufshcd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h |   2 +
 3 files changed, 228 insertions(+)
drivers/scsi/ufs/ufs.h +28 -0
index e15deb0..1f99904 100644
@@ -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,
drivers/scsi/ufs/ufshcd.c +198 -0

216 lines not on this page · raw message

drivers/scsi/ufs/ufshcd.h +2 -0
index 101a75c..0e6bf30 100644
@@ -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);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#3 lore raw

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

Quoted 106 lines · 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 <[email protected]>
---
  drivers/scsi/ufs/ufs.h    |  28 +++++++
  drivers/scsi/ufs/ufshcd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++
  drivers/scsi/ufs/ufshcd.h |   2 +
  3 files changed, 228 insertions(+)
drivers/scsi/ufs/ufs.h +28 -0
index e15deb0..1f99904 100644
@@ -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,
drivers/scsi/ufs/ufshcd.c +24 -0
index 351f874..370a7c6 100644
@@ -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;

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6399

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +10 -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.

Quoted 188 lines
drivers/scsi/ufs/ufshcd.c +164 -0
+
+	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
   *
drivers/scsi/ufs/ufshcd.h +2 -0
index 101a75c..0e6bf30 100644
@@ -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);
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
#4 lore raw

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

In reply to Sayali Lokhande

Hi Sayali,

Quoted 20 lines · 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 <[email protected]>
---
 drivers/scsi/ufs/ufs.h    |  28 +++++++
 drivers/scsi/ufs/ufshcd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h |   2 +
 3 files changed, 228 insertions(+)
drivers/scsi/ufs/ufs.h +0 -0
index e15deb0..1f99904 100644

drivers/scsi/ufs/ufs.h enum { · line 336

Quoted 4 lines
drivers/scsi/ufs/ufs.h +1 -0
@@ -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.

Quoted 23 lines
drivers/scsi/ufs/ufs.h +16 -0
 #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 */

drivers/scsi/ufs/ufs.h enum { · line 454

Quoted 10 lines
drivers/scsi/ufs/ufs.h +10 -0
+       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.

drivers/scsi/ufs/ufshcd.c line 45

Quoted 13 lines
drivers/scsi/ufs/ufs.h +1 -0
+
 /* Task management service response */
 enum {
        UPIU_TASK_MANAGEMENT_FUNC_COMPL         = 0x00,
drivers/scsi/ufs/ufshcd.c +1 -0
index 351f874..370a7c6 100644
@@ -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.

Quoted 5 lines
drivers/scsi/ufs/ufshcd.c +0 -0
 #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);

drivers/scsi/ufs/ufshcd.c static inline int ufshcd_read_power_desc(struct ufs_hba *hba, · line 3074

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +8 -0
 }
 
+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.

Quoted 17 lines
drivers/scsi/ufs/ufshcd.c +10 -0
 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;

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6394

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +10 -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.

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6397

Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +3 -0
+       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.

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6402

Quoted 5 lines
drivers/scsi/ufs/ufshcd.c +5 -0
+
+       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?

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6403

Quoted 1 line
drivers/scsi/ufs/ufshcd.c +1 -0
+       if (ufshcd_wait_for_doorbell_clr(hba, U64_MAX)) {

Why is this needed?

Quoted 15 lines
drivers/scsi/ufs/ufshcd.c +15 -0
+               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

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6428

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +10 -0
+        * 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).

Quoted 69 lines
drivers/scsi/ufs/ufshcd.c +69 -0
+
+       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;

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6507

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +10 -0
+               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.

Quoted 19 lines
drivers/scsi/ufs/ufshcd.c +19 -0
+
+       /* 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;

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6536

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +10 -0
+               *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.

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6539

Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +3 -0
+
+       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.

Quoted 5 lines
drivers/scsi/ufs/ufshcd.c +5 -0
+
+       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);

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6554

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +10 -0
+               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].

Quoted 17 lines
drivers/scsi/ufs/ufshcd.c +9 -0
+
+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
  *
drivers/scsi/ufs/ufshcd.h +0 -0
index 101a75c..0e6bf30 100644

drivers/scsi/ufs/ufshcd.h struct ufs_hba { · line 552

Quoted 4 lines
drivers/scsi/ufs/ufshcd.h +1 -0
@@ -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()?

Quoted 12 lines
drivers/scsi/ufs/ufshcd.h +1 -1
        /* 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);
--
Quoted 3 lines

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/

author
#5 lore raw

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

Hi Evan,

Quoted 25 lines · Evan Green

Hi Sayali,

Quoted 22 lines · 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 <[email protected]>
---
  drivers/scsi/ufs/ufs.h    |  28 +++++++
  drivers/scsi/ufs/ufshcd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++
  drivers/scsi/ufs/ufshcd.h |   2 +
  3 files changed, 228 insertions(+)
drivers/scsi/ufs/ufs.h +0 -0
index e15deb0..1f99904 100644
@@ -333,6 +333,7 @@ enum {
         UFSHCD_AMP              = 3,
  };
Quoted 4 lines

drivers/scsi/ufs/ufs.h enum { · line 336

Quoted 1 line
drivers/scsi/ufs/ufs.h +1 -0
+#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.

Quoted 31 lines
Quoted 31 lines
drivers/scsi/ufs/ufs.h +23 -0
  #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 */
Quoted 8 lines

drivers/scsi/ufs/ufs.h enum { · line 454

Quoted 3 lines
drivers/scsi/ufs/ufs.h +3 -0
+       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.

Quoted 7 lines
Quoted 7 lines
drivers/scsi/ufs/ufs.h +1 -0
+
  /* Task management service response */
  enum {
         UPIU_TASK_MANAGEMENT_FUNC_COMPL         = 0x00,
drivers/scsi/ufs/ufshcd.c +0 -1
index 351f874..370a7c6 100644
--- a/drivers/scsi/ufs/ufshcd.c
Quoted 8 lines

drivers/scsi/ufs/ufshcd.c line 45

Quoted 6 lines

+++ b/drivers/scsi/ufs/ufshcd.c

drivers/scsi/ufs/ufshcd.c +1 -0
@@ -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.

Quoted 38 lines
Quoted 5 lines
drivers/scsi/ufs/ufshcd.c +0 -0
  #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);

drivers/scsi/ufs/ufshcd.c static inline int ufshcd_read_power_desc(struct ufs_hba *hba, · line 3074

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +8 -0
  }
 
+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.

Quoted 21 lines
drivers/scsi/ufs/ufshcd.c +14 -0
  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;
Quoted 8 lines

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6394

Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +6 -0
+       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.

Quoted 4 lines

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6397

Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +3 -0
+       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.

Quoted 11 lines

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6402

Quoted 5 lines
drivers/scsi/ufs/ufshcd.c +5 -0
+
+       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?

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6403

Quoted 1 line
drivers/scsi/ufs/ufshcd.c +1 -0
+       if (ufshcd_wait_for_doorbell_clr(hba, U64_MAX)) {

Why is this needed?

This is just to ensure stable operation while doing runtime provisioning.

Quoted 19 lines
Quoted 19 lines
drivers/scsi/ufs/ufshcd.c +19 -0
+               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.
Quoted 8 lines

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6428

Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +6 -0
+        */
+       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.

Quoted 75 lines
Quoted 75 lines
drivers/scsi/ufs/ufshcd.c +75 -0
+
+       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 +=
Quoted 8 lines

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6507

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +4 -0
+               ((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).

Quoted 25 lines
Quoted 25 lines
drivers/scsi/ufs/ufshcd.c +25 -0
+
+       /* 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;
Quoted 8 lines

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6536

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +4 -0
+               *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.

Quoted 8 lines

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6539

Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +3 -0
+
+       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.

Quoted 11 lines
Quoted 11 lines
drivers/scsi/ufs/ufshcd.c +10 -0
+
+       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,
Quoted 8 lines

drivers/scsi/ufs/ufshcd.c static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba) · line 6554

Quoted 5 lines
drivers/scsi/ufs/ufshcd.c +5 -0
+               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).

Quoted 16 lines
Quoted 16 lines
drivers/scsi/ufs/ufshcd.c +9 -0
+
+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
   *
drivers/scsi/ufs/ufshcd.h +0 -1
index 101a75c..0e6bf30 100644
--- a/drivers/scsi/ufs/ufshcd.h
Quoted 8 lines

drivers/scsi/ufs/ufshcd.h struct ufs_hba { · line 552

Quoted 6 lines

+++ b/drivers/scsi/ufs/ufshcd.h

drivers/scsi/ufs/ufshcd.h +1 -0
@@ -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.

Quoted 8 lines
Quoted 8 lines
drivers/scsi/ufs/ufshcd.h +1 -0
         /* 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);
Quoted 8 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.h +0 -1
  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

author
#6 lore raw

PATCH v4 3/3 scsi: ufs: Add configfs support for ufs provisioning

In reply to a message not in the archive

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 <[email protected]>
---
 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
Documentation/ABI/testing/configfs-driver-ufs new file +15 -0
new file mode 100644
index 0000000..f6ef38e
@@ -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
drivers/scsi/ufs/Makefile +1 -0
index 918f579..d438e74 100644
@@ -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
drivers/scsi/ufs/ufs-configfs.c new file +198 -0

198 lines not on this page · raw message

drivers/scsi/ufs/ufs.h +2 -0
index 1f99904..0b497fc 100644
@@ -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 */
drivers/scsi/ufs/ufshcd.c +2 -0
index 370a7c6..e980d5a 100644
@@ -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;
drivers/scsi/ufs/ufshcd.h +18 -0
index 0e6bf30..7d2fa89 100644
@@ -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);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
build bot
#7 lore raw

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

Thank you for the patch! Yet something to improve:

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

#8 lore raw

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

In reply to Sayali Lokhande

Hi Sayali,

Quoted 7 lines · 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)

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.

Quoted 54 lines
Signed-off-by Sayali Lokhande <[email protected]>
---
 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
Documentation/ABI/testing/configfs-driver-ufs new file +15 -0
new file mode 100644
index 0000000..f6ef38e
@@ -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
drivers/scsi/ufs/Makefile +1 -0
index 918f579..d438e74 100644
@@ -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
drivers/scsi/ufs/ufs-configfs.c new file +3 -0
new file mode 100644
index 0000000..614b017
@@ -0,0 +1,198 @@
+/*
+ * Copyright (c) 2018, Linux Foundation. All rights reserved.
+ *

drivers/scsi/ufs/ufs-configfs.c line 13

Quoted 10 lines
drivers/scsi/ufs/ufs-configfs.c new file +10 -0
+ * 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/

drivers/scsi/ufs/ufs-configfs.c line 18

Quoted 5 lines
drivers/scsi/ufs/ufs-configfs.c new file +5 -0
+
+#include <linux/err.h>
+#include <linux/string.h>
+#include <asm/unaligned.h>
+#include <linux/configfs.h>

These should be alphabetized.

drivers/scsi/ufs/ufs-configfs.c line 23

Quoted 5 lines
drivers/scsi/ufs/ufs-configfs.c new file +5 -0
+
+#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.

drivers/scsi/ufs/ufs-configfs.c line 28

Quoted 5 lines
drivers/scsi/ufs/ufs-configfs.c new file +5 -0
+
+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.

drivers/scsi/ufs/ufs-configfs.c line 36

Quoted 8 lines
drivers/scsi/ufs/ufs-configfs.c new file +8 -0
+}
+
+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.

drivers/scsi/ufs/ufs-configfs.c line 42

Quoted 6 lines
drivers/scsi/ufs/ufs-configfs.c new file +6 -0
+       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?

drivers/scsi/ufs/ufs-configfs.c line 51

Quoted 9 lines
drivers/scsi/ufs/ufs-configfs.c new file +9 -0
+
+       /* 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.

drivers/scsi/ufs/ufs-configfs.c line 62

Quoted 11 lines
drivers/scsi/ufs/ufs-configfs.c new file +11 -0
+
+       /* 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.

drivers/scsi/ufs/ufs-configfs.c line 75

Quoted 13 lines
drivers/scsi/ufs/ufs-configfs.c new file +13 -0
+               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.

drivers/scsi/ufs/ufs-configfs.c line 80

Quoted 5 lines
drivers/scsi/ufs/ufs-configfs.c new file +5 -0
+                               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.

Quoted 26 lines
drivers/scsi/ufs/ufs-configfs.c new file +26 -0
+                       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,

drivers/scsi/ufs/ufs-configfs.c line 116

Quoted 10 lines
drivers/scsi/ufs/ufs-configfs.c new file +10 -0
+       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.

drivers/scsi/ufs/ufs-configfs.c line 123

Quoted 7 lines
drivers/scsi/ufs/ufs-configfs.c new file +7 -0
+               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?

drivers/scsi/ufs/ufs-configfs.c line 130

Quoted 7 lines
drivers/scsi/ufs/ufs-configfs.c new file +7 -0
+
+       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.

Quoted 81 lines
drivers/scsi/ufs/ufs-configfs.c new file +68 -0
+               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);
+}
drivers/scsi/ufs/ufs.h +1 -0
index 1f99904..0b497fc 100644
@@ -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 */

drivers/scsi/ufs/ufs.h struct ufs_config_descr { · line 456

Quoted 5 lines
drivers/scsi/ufs/ufs.h +1 -0
@@ -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.

Quoted 69 lines
drivers/scsi/ufs/ufs.h +0 -0
 /* Task management service response */
drivers/scsi/ufs/ufshcd.c +2 -0
index 370a7c6..e980d5a 100644
@@ -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;
drivers/scsi/ufs/ufshcd.h +18 -1
index 0e6bf30..7d2fa89 100644
@@ -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

author
#9 lore raw

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

Hi Evan,

Quoted 9 lines · Evan Green

Hi Sayali,

Quoted 6 lines · 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)

Quoted 4 lines

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.

Quoted 58 lines
Quoted 58 lines
Signed-off-by Sayali Lokhande <[email protected]>
---
  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

Documentation/ABI/testing/configfs-driver-ufs new file +15 -0
new file mode 100644
index 0000000..f6ef38e
@@ -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
drivers/scsi/ufs/Makefile +1 -0
index 918f579..d438e74 100644
@@ -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
drivers/scsi/ufs/ufs-configfs.c new file +8 -0
new file mode 100644
index 0000000..614b017
@@ -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,
Quoted 8 lines

drivers/scsi/ufs/ufs-configfs.c line 13

Quoted 5 lines
drivers/scsi/ufs/ufs-configfs.c new file +5 -0
+ * 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.

Quoted 6 lines

drivers/scsi/ufs/ufs-configfs.c line 18

Quoted 5 lines
drivers/scsi/ufs/ufs-configfs.c new file +5 -0
+
+#include <linux/err.h>
+#include <linux/string.h>
+#include <asm/unaligned.h>
+#include <linux/configfs.h>

These should be alphabetized.

Agreed. Will update.

Quoted 8 lines

drivers/scsi/ufs/ufs-configfs.c line 23

Quoted 5 lines
drivers/scsi/ufs/ufs-configfs.c new file +5 -0
+
+#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.

Quoted 9 lines

drivers/scsi/ufs/ufs-configfs.c line 28

Quoted 5 lines
drivers/scsi/ufs/ufs-configfs.c new file +5 -0
+
+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.

Quoted 4 lines
Quoted 4 lines
drivers/scsi/ufs/ufs-configfs.c new file +4 -0
+}
+
+ssize_t ufshcd_desc_configfs_store(const char *buf, size_t count)
+{
Quoted 8 lines

drivers/scsi/ufs/ufs-configfs.c line 36

Quoted 4 lines
drivers/scsi/ufs/ufs-configfs.c new file +4 -0
+       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.

Quoted 7 lines

drivers/scsi/ufs/ufs-configfs.c line 42

Quoted 6 lines
drivers/scsi/ufs/ufs-configfs.c new file +6 -0
+       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).

Quoted 12 lines

drivers/scsi/ufs/ufs-configfs.c line 51

Quoted 10 lines
drivers/scsi/ufs/ufs-configfs.c new file +9 -0
+
+       /* 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.

Quoted 12 lines

drivers/scsi/ufs/ufs-configfs.c line 62

Quoted 12 lines
drivers/scsi/ufs/ufs-configfs.c new file +11 -0
+
+       /* 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) {
Quoted 2 lines

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

Ok . Will update it.

Quoted 14 lines
Quoted 4 lines
drivers/scsi/ufs/ufs-configfs.c new file +3 -0
+               dev_err(hba->dev, "%s: bConfigDescrLock already set to %u, cannot re-provision device!\n",
+               __func__, cfg->bConfigDescrLock);
+               hba->provision_enabled = 0;

drivers/scsi/ufs/ufs-configfs.c line 75

Quoted 10 lines
drivers/scsi/ufs/ufs-configfs.c new file +10 -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) {
Quoted 5 lines

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".

Quoted 11 lines

drivers/scsi/ufs/ufs-configfs.c line 80

Quoted 6 lines
drivers/scsi/ufs/ufs-configfs.c new file +5 -0
+                               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).

Quoted 35 lines
Quoted 35 lines
drivers/scsi/ufs/ufs-configfs.c new file +35 -0
+                       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 */
Quoted 8 lines

drivers/scsi/ufs/ufs-configfs.c line 116

Quoted 1 line
drivers/scsi/ufs/ufs-configfs.c new file +1 -0
+               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).

Quoted 11 lines

drivers/scsi/ufs/ufs-configfs.c line 123

Quoted 7 lines
drivers/scsi/ufs/ufs-configfs.c new file +7 -0
+               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.

Quoted 11 lines

drivers/scsi/ufs/ufs-configfs.c line 130

Quoted 8 lines
drivers/scsi/ufs/ufs-configfs.c new file +7 -0
+
+       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.

Quoted 87 lines
Quoted 81 lines
drivers/scsi/ufs/ufs-configfs.c new file +68 -0
+               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);
+}
drivers/scsi/ufs/ufs.h +1 -0
index 1f99904..0b497fc 100644
@@ -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 */

drivers/scsi/ufs/ufs.h struct ufs_config_descr { · line 456

Quoted 5 lines
drivers/scsi/ufs/ufs.h +1 -0
@@ -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

Quoted 8 lines

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.

Quoted 62 lines
Quoted 62 lines
drivers/scsi/ufs/ufs.h +0 -0
  /* Task management service response */
drivers/scsi/ufs/ufshcd.c +2 -0
index 370a7c6..e980d5a 100644
@@ -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;
drivers/scsi/ufs/ufshcd.h +18 -0
index 0e6bf30..7d2fa89 100644
@@ -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 */
Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.h +0 -1
  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