[PATCH 0/7] Enable UFS provisioning via Linux

[PATCH 0/7] Enable UFS provisioning via Linux

[Evan Green]

This series enables provisioning UFS devices using the existing sysfs
interface. This functionality is primarily useful along the assembly
line, but might also be useful for end users that receive devices that
aren't locked down.

Evan Green (7):
  scsi: ufs: Add Configuration Descriptor to sysfs
  scsi: ufs: Add config descriptor documentation
  scsi: ufs: Make sysfs attributes writable
  scsi: ufs: sysfs: Document attribute writability
  scsi: ufs: Refactor descriptor read for write
  scsi: ufs: Enable writing config descriptor
  scsi: ufs: Update config descriptor documentation

 Documentation/ABI/testing/sysfs-driver-ufs | 174 ++++++++++++++++++++---
 drivers/scsi/ufs/ufs-sysfs.c               | 217 ++++++++++++++++++++++++++---
 drivers/scsi/ufs/ufs.h                     |  29 ++++
 drivers/scsi/ufs/ufshcd.c                  |  89 ++++++++----
 drivers/scsi/ufs/ufshcd.h                  |  16 ++-
 5 files changed, 458 insertions(+), 67 deletions(-)

-- 
2.13.5

[PATCH 1/7] scsi: ufs: Add Configuration Descriptor to sysfs

[Evan Green]

This change adds the configuration descriptor to the UFS
sysfs interface. This is done in preparation for making the
interface writable, which will enable provisioning UFS devices
via Linux.

The configuration descriptor is laid out as a header, then a set of
(usually 8) copies of the same descriptor for each unit. Rather than
creating 8 sets of the same attribute, this interface adds a cfg_unit
file, which can be set as an index that defines which of the 8
configuration unit descriptors are being shown through sysfs.

Signed-off-by: Evan Green <evgreen@chromium.org>
---
In this change, I used cfg_unit as a selector because it matched the
precedent set by rpm_lvl and spm_lvl, and was frankly the easiest option.

It was pointed out to me that this creates a synchronization problem for
user mode, as it means only one process can be safely using this interface
at a time. If it's preferred, I can change the interface to enumerate a
sysfs group for each unit descriptor in the config descriptor.

The argument for keeping it the way it currently is goes like:
1) On the read side, all the information available here is
available elsewhere in other descriptors. I suppose that doesn't
stop apps from doing it anyway and interfering with a write.
2) On the write side, there's probably already something wrong
if more than one process is trying to provision the disk.

Let me know what you think. I think I'm leaning towards the dynamic groups,
but I could be convinced either way.
---
 drivers/scsi/ufs/ufs-sysfs.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs.h       | 29 +++++++++++++
 drivers/scsi/ufs/ufshcd.h    |  3 ++
 3 files changed, 129 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 8d9332bb7d0c..a7d65492832a 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -327,6 +327,102 @@ static const struct attribute_group ufs_sysfs_device_descriptor_group = {
 	.attrs = ufs_sysfs_device_descriptor,
 };
 
+static ssize_t cfg_unit_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", hba->sysfs_config_unit);
+}
+
+static ssize_t cfg_unit_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	unsigned long max_value, value;
+
+	if (kstrtoul(buf, 0, &value))
+		return -EINVAL;
+
+	if (hba->desc_size.conf_desc < CONFIGURATION_DESC_PARAM_UNIT0)
+		return -EINVAL;
+
+	max_value = (hba->desc_size.conf_desc -
+		CONFIGURATION_DESC_PARAM_UNIT0) / CONFIGURATION_UNIT_DESC_SIZE;
+
+	if (value >= max_value)
+		return -EINVAL;
+
+	hba->sysfs_config_unit = value;
+	return count;
+}
+
+#define UFS_CONFIG_DESC_PARAM(_name, _uname, _size)			\
+	UFS_DESC_PARAM(cfg_##_name, _uname, CONFIGURATION, _size)
+
+#define UFS_CONFIG_UNIT_DESC_PARAM(_name, _uname, _size)		\
+static ssize_t unit_##_name##_show(struct device *dev,			\
+	struct device_attribute *attr, char *buf)			\
+{									\
+	struct ufs_hba *hba = dev_get_drvdata(dev);			\
+	size_t offset = CONFIGURATION_DESC_PARAM_UNIT0 +		\
+		(hba->sysfs_config_unit *				\
+		 CONFIGURATION_UNIT_DESC_SIZE) +			\
+		 CONFIGURATION_UNIT_DESC_PARAM_##_uname;		\
+	if (offset + _size > hba->desc_size.conf_desc) {		\
+		return -EINVAL;						\
+	}								\
+	return ufs_sysfs_read_desc_param(hba,				\
+		QUERY_DESC_IDN_CONFIGURATION, 0, offset, buf, _size);	\
+}									\
+static DEVICE_ATTR_RO(unit_##_name)
+
+UFS_CONFIG_DESC_PARAM(number_of_luns, _NUM_LU, 1);
+UFS_CONFIG_DESC_PARAM(boot_enable, _BOOT_ENBL, 1);
+UFS_CONFIG_DESC_PARAM(descriptor_access_enable, _DESC_ACCSS_ENBL, 1);
+UFS_CONFIG_DESC_PARAM(initial_power_mode, _INIT_PWR_MODE, 1);
+UFS_CONFIG_DESC_PARAM(high_priority_lun, _HIGH_PR_LUN, 1);
+UFS_CONFIG_DESC_PARAM(secure_removal_type, _SEC_RMV_TYPE, 1);
+UFS_CONFIG_DESC_PARAM(init_active_icc_level, _ACTVE_ICC_LVL, 1);
+UFS_CONFIG_DESC_PARAM(periodic_rtc_update, _FRQ_RTC, 2);
+DEVICE_ATTR_RW(cfg_unit);
+UFS_CONFIG_UNIT_DESC_PARAM(lu_enable, LU_ENABLE, 1);
+UFS_CONFIG_UNIT_DESC_PARAM(boot_lun_id, BOOT_LUN_ID, 1);
+UFS_CONFIG_UNIT_DESC_PARAM(lu_write_protect, LU_WR_PROTECT, 1);
+UFS_CONFIG_UNIT_DESC_PARAM(memory_type, MEM_TYPE, 1);
+UFS_CONFIG_UNIT_DESC_PARAM(allocation_units, NUM_ALLOC_UNITS, 4);
+UFS_CONFIG_UNIT_DESC_PARAM(data_reliability, DATA_RELIABILITY, 1);
+UFS_CONFIG_UNIT_DESC_PARAM(logical_block_size, LOGICAL_BLK_SIZE, 1);
+UFS_CONFIG_UNIT_DESC_PARAM(provisioning_type, PROVISIONING_TYPE, 1);
+UFS_CONFIG_UNIT_DESC_PARAM(context_capabilities, CTX_CAPABILITIES, 2);
+
+static struct attribute *ufs_sysfs_config_descriptor[] = {
+	&dev_attr_cfg_number_of_luns.attr,
+	&dev_attr_cfg_boot_enable.attr,
+	&dev_attr_cfg_descriptor_access_enable.attr,
+	&dev_attr_cfg_initial_power_mode.attr,
+	&dev_attr_cfg_high_priority_lun.attr,
+	&dev_attr_cfg_secure_removal_type.attr,
+	&dev_attr_cfg_init_active_icc_level.attr,
+	&dev_attr_cfg_periodic_rtc_update.attr,
+	&dev_attr_cfg_unit.attr,
+	&dev_attr_unit_lu_enable.attr,
+	&dev_attr_unit_boot_lun_id.attr,
+	&dev_attr_unit_lu_write_protect.attr,
+	&dev_attr_unit_memory_type.attr,
+	&dev_attr_unit_allocation_units.attr,
+	&dev_attr_unit_data_reliability.attr,
+	&dev_attr_unit_logical_block_size.attr,
+	&dev_attr_unit_provisioning_type.attr,
+	&dev_attr_unit_context_capabilities.attr,
+	NULL,
+};
+
+static const struct attribute_group ufs_sysfs_config_descriptor_group = {
+	.name = "config_descriptor",
+	.attrs = ufs_sysfs_config_descriptor,
+};
+
 #define UFS_INTERCONNECT_DESC_PARAM(_name, _uname, _size)		\
 	UFS_DESC_PARAM(_name, _uname, INTERCONNECT, _size)
 
@@ -713,6 +809,7 @@ static const struct attribute_group ufs_sysfs_attributes_group = {
 static const struct attribute_group *ufs_sysfs_groups[] = {
 	&ufs_sysfs_default_group,
 	&ufs_sysfs_device_descriptor_group,
+	&ufs_sysfs_config_descriptor_group,
 	&ufs_sysfs_interconnect_descriptor_group,
 	&ufs_sysfs_geometry_descriptor_group,
 	&ufs_sysfs_health_descriptor_group,
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 14e5bf7af0bb..a5712b1a07ad 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -260,6 +260,35 @@ enum device_desc_param {
 	DEVICE_DESC_PARAM_PRDCT_REV		= 0x2A,
 };
 
+/* Configuration descriptor parameter offsets in bytes */
+enum configuration_desc_param {
+	CONFIGURATION_DESC_PARAM_LEN			= 0x0,
+	CONFIGURATION_DESC_PARAM_TYPE			= 0x1,
+	CONFIGURATION_DESC_PARAM_NUM_LU			= 0x2,
+	CONFIGURATION_DESC_PARAM_BOOT_ENBL		= 0x3,
+	CONFIGURATION_DESC_PARAM_DESC_ACCSS_ENBL	= 0x4,
+	CONFIGURATION_DESC_PARAM_INIT_PWR_MODE		= 0x5,
+	CONFIGURATION_DESC_PARAM_HIGH_PR_LUN		= 0x6,
+	CONFIGURATION_DESC_PARAM_SEC_RMV_TYPE		= 0x7,
+	CONFIGURATION_DESC_PARAM_ACTVE_ICC_LVL		= 0x8,
+	CONFIGURATION_DESC_PARAM_FRQ_RTC		= 0x9,
+	CONFIGURATION_DESC_PARAM_UNIT0			= 0x10,
+};
+
+/* Configuration unit descriptor parameter offsets in bytes */
+enum configuration_unit_desc_param {
+	CONFIGURATION_UNIT_DESC_PARAM_LU_ENABLE		= 0x0,
+	CONFIGURATION_UNIT_DESC_PARAM_BOOT_LUN_ID	= 0x1,
+	CONFIGURATION_UNIT_DESC_PARAM_LU_WR_PROTECT	= 0x2,
+	CONFIGURATION_UNIT_DESC_PARAM_MEM_TYPE		= 0x3,
+	CONFIGURATION_UNIT_DESC_PARAM_NUM_ALLOC_UNITS	= 0x4,
+	CONFIGURATION_UNIT_DESC_PARAM_DATA_RELIABILITY	= 0x8,
+	CONFIGURATION_UNIT_DESC_PARAM_LOGICAL_BLK_SIZE	= 0x9,
+	CONFIGURATION_UNIT_DESC_PARAM_PROVISIONING_TYPE	= 0xA,
+	CONFIGURATION_UNIT_DESC_PARAM_CTX_CAPABILITIES	= 0xB,
+	CONFIGURATION_UNIT_DESC_SIZE			= 0x10,
+};
+
 /* Interconnect descriptor parameters offsets in bytes*/
 enum interconnect_desc_param {
 	INTERCONNECT_DESC_PARAM_LEN		= 0x0,
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 8110dcd04d22..fb6dcc490f21 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -683,6 +683,9 @@ struct ufs_hba {
 
 	struct rw_semaphore clk_scaling_lock;
 	struct ufs_desc_size desc_size;
+
+	/* Which unit descriptor is showing in the sysfs config descriptor. */
+	int sysfs_config_unit;
 };
 
 /* Returns true if clocks can be gated. Otherwise false */
-- 
2.13.5

Re: [PATCH 1/7] scsi: ufs: Add Configuration Descriptor to sysfs

[Bart Van Assche]

On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> This change adds the configuration descriptor to the UFS
> sysfs interface. This is done in preparation for making the
> interface writable, which will enable provisioning UFS devices
> via Linux.
> 
> The configuration descriptor is laid out as a header, then a set of
> (usually 8) copies of the same descriptor for each unit. Rather than
> creating 8 sets of the same attribute, this interface adds a cfg_unit
> file, which can be set as an index that defines which of the 8
> configuration unit descriptors are being shown through sysfs.

I don't know any other example of a sysfs attribute that controls the
contents of another sysfs attribute so I don't know whether this kind
of behavior is acceptable for sysfs attributes. Additionally, how can
these two sysfs attributes be used together from concurrently running
processes without triggering a race condition?

Bart.

Re: [PATCH 1/7] scsi: ufs: Add Configuration Descriptor to sysfs

[Evan Green]

On Mon, Jun 4, 2018 at 1:31 AM Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>
> On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> > This change adds the configuration descriptor to the UFS
> > sysfs interface. This is done in preparation for making the
> > interface writable, which will enable provisioning UFS devices
> > via Linux.
> >
> > The configuration descriptor is laid out as a header, then a set of
> > (usually 8) copies of the same descriptor for each unit. Rather than
> > creating 8 sets of the same attribute, this interface adds a cfg_unit
> > file, which can be set as an index that defines which of the 8
> > configuration unit descriptors are being shown through sysfs.
>
> I don't know any other example of a sysfs attribute that controls the
> contents of another sysfs attribute so I don't know whether this kind
> of behavior is acceptable for sysfs attributes. Additionally, how can
> these two sysfs attributes be used together from concurrently running
> processes without triggering a race condition?
>
> Bart.

Hi Bart,
I was following the example of rpm_lvl and spm_lvl in this driver,
whose values control what is shown in rpm_target_dev_state and
rpm_target_link_state (and spm equivalents). My original thinking was
that provisioning the high level storage configuration of a UFS disk
is by definition a single threaded one-time-only activity. However
Stanislav and others have also expressed concerns about this
mechanism, so it sounds like I should revise it. I'm hoping Stanislav
will comment on one of two possible approaches I suggested in the
cover letter.

-Evan

[PATCH 2/7] scsi: ufs: Add config descriptor documentation

[Evan Green]

This change adds the documentation for the new sysfs files plumbed out
for the UFS configuration descriptor.

Signed-off-by: Evan Green <evgreen@chromium.org>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 174 +++++++++++++++++++++++++++++
 1 file changed, 174 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index 016724ec26d5..a1336194628f 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -237,6 +237,180 @@ Description:	This file shows the command maximum timeout for a change
 		The file is read only.
 
 
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_boot_enable
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows whether or not the UFS boot feature is enabled.
+		This is one of the UFS configuration descriptor parameters.
+		More information about the descriptor can be found in the UFS
+		2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_descriptor_access_enable
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows whether or not access will be permitted to the
+		Device Descriptor after the partial initialization phase of the
+		boot sequence. This is one of the UFS configuration descriptor
+		parameters. More information about the descriptor can be found
+		in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_high_priority_lun
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the identifier of the high priority logical
+		unit. This is one of the UFS configuration descriptor
+		parameters. More information about the descriptor can be found
+		in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_init_active_icc_level
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the ICC level in active mode after device
+		initialization or hardware reset. This is one of the UFS
+		configuration descriptor parameters. More information about the
+		descriptor can be found in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_initial_power_mode
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the power mode after device initialization or
+		hardware reset. This is one of the UFS configuration descriptor
+		parameters. More information about the descriptor can be found
+		in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_number_of_luns
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the number of logical units that the device will
+		support. This is one of the UFS configuration descriptor
+		parameters. More information about the descriptor can be found
+		in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_periodic_rtc_update
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the frequency and method of real time clock
+		updates. This is one of the UFS configuration descriptor
+		parameters. More information about the descriptor can be found
+		in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_secure_removal_type
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the secure removal type of the UFS device. This
+		is one of the UFS configuration descriptor parameters. More
+		information about the descriptor can be found in the UFS 2.1
+		specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_unit
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file identifies the logical unit number whose parameters
+		are being displayed by the unit_* files in this directory. This
+		file can be set to different numerical values in order to
+		interact with configuration unit descriptors from additional
+		LUNs.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_allocation_units
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the number of allocation units assigned to the
+		logical unit. The cfg_unit file controls which logical unit is
+		being displayed. This is one of the UFS configuration unit
+		descriptor parameters. More information about the descriptor
+		can be found in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_boot_lun_id
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the boot LUN ID for this logical unit,
+		indicating whether it is Boot A, Boot B, or not special. The
+		cfg_unit file controls which logical unit is being displayed.
+		This is one of the UFS configuration unit descriptor parameters.
+		More information about the descriptor can be found in the UFS
+		2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_context_capabilities
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the context capabilities for the logical unit.
+		The cfg_unit file controls which logical unit is
+		being displayed. This is one of the UFS configuration unit
+		descriptor parameters. More information about the descriptor
+		can be found in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_data_reliability
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the data reliability for the logical unit.
+		The cfg_unit file controls which logical unit is
+		being displayed. This is one of the UFS configuration unit
+		descriptor parameters. More information about the descriptor
+		can be found in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_logical_block_size
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the logical block size for the logical unit as
+		a power of two. The cfg_unit file controls which logical unit is
+		being displayed. This is one of the UFS configuration unit
+		descriptor parameters. More information about the descriptor
+		can be found in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_lu_enable
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows whether or not the logical unit is enabled.
+		The cfg_unit file controls which logical unit is
+		being displayed. This is one of the UFS configuration unit
+		descriptor parameters. More information about the descriptor
+		can be found in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_lu_write_protect
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the write protect status for the logical unit.
+		The cfg_unit file controls which logical unit is
+		being displayed. This is one of the UFS configuration unit
+		descriptor parameters. More information about the descriptor
+		can be found in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_memory_type
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the memory type for the logical unit.
+		The cfg_unit file controls which logical unit is
+		being displayed. This is one of the UFS configuration unit
+		descriptor parameters. More information about the descriptor
+		can be found in the UFS 2.1 specification.
+		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_provisioning_type
+Date:		May 2018
+Contact:	Evan Green <evgreen@chromium.org>
+Description:	This file shows the provisioning type information for the
+		logical unit. The cfg_unit file controls which logical unit is
+		being displayed. This is one of the UFS configuration unit
+		descriptor parameters. More information about the descriptor
+		can be found in the UFS 2.1 specification.
+		The file is read only.
+
+
 What:		/sys/bus/platform/drivers/ufshcd/*/interconnect_descriptor/unipro_version
 Date:		February 2018
 Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
-- 
2.13.5

Re: [PATCH 2/7] scsi: ufs: Add config descriptor documentation

[Bart Van Assche]

On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> This change adds the documentation for the new sysfs files plumbed out
> for the UFS configuration descriptor.

Shouldn't these changes be merged into the patch that adds these attributes?

Bart.

Re: [PATCH 2/7] scsi: ufs: Add config descriptor documentation

[Evan Green]

On Mon, Jun 4, 2018 at 1:35 AM Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>
> On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> > This change adds the documentation for the new sysfs files plumbed out
> > for the UFS configuration descriptor.
>
> Shouldn't these changes be merged into the patch that adds these attributes?
>
> Bart.

Will do.

[PATCH 3/7] scsi: ufs: Make sysfs attributes writable

[Evan Green]

This change makes the UFS controller's sysfs attributes writable, which
will enable users to provision unprovisioned UFS devices, or
re-provision unlocked UFS devices.

Signed-off-by: Evan Green <evgreen@chromium.org>
---
 drivers/scsi/ufs/ufs-sysfs.c | 58 +++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index a7d65492832a..51bf54e6b47a 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -751,7 +751,7 @@ static const struct attribute_group ufs_sysfs_flags_group = {
 	.attrs = ufs_sysfs_device_flags,
 };
 
-#define UFS_ATTRIBUTE(_name, _uname)					\
+#define UFS_ATTRIBUTE_SHOW(_name, _uname)				\
 static ssize_t _name##_show(struct device *dev,				\
 	struct device_attribute *attr, char *buf)			\
 {									\
@@ -761,25 +761,45 @@ static ssize_t _name##_show(struct device *dev,				\
 		QUERY_ATTR_IDN##_uname, 0, 0, &value))			\
 		return -EINVAL;						\
 	return sprintf(buf, "0x%08X\n", value);				\
-}									\
-static DEVICE_ATTR_RO(_name)
+}
 
-UFS_ATTRIBUTE(boot_lun_enabled, _BOOT_LU_EN);
-UFS_ATTRIBUTE(current_power_mode, _POWER_MODE);
-UFS_ATTRIBUTE(active_icc_level, _ACTIVE_ICC_LVL);
-UFS_ATTRIBUTE(ooo_data_enabled, _OOO_DATA_EN);
-UFS_ATTRIBUTE(bkops_status, _BKOPS_STATUS);
-UFS_ATTRIBUTE(purge_status, _PURGE_STATUS);
-UFS_ATTRIBUTE(max_data_in_size, _MAX_DATA_IN);
-UFS_ATTRIBUTE(max_data_out_size, _MAX_DATA_OUT);
-UFS_ATTRIBUTE(reference_clock_frequency, _REF_CLK_FREQ);
-UFS_ATTRIBUTE(configuration_descriptor_lock, _CONF_DESC_LOCK);
-UFS_ATTRIBUTE(max_number_of_rtt, _MAX_NUM_OF_RTT);
-UFS_ATTRIBUTE(exception_event_control, _EE_CONTROL);
-UFS_ATTRIBUTE(exception_event_status, _EE_STATUS);
-UFS_ATTRIBUTE(ffu_status, _FFU_STATUS);
-UFS_ATTRIBUTE(psa_state, _PSA_STATE);
-UFS_ATTRIBUTE(psa_data_size, _PSA_DATA_SIZE);
+#define UFS_ATTRIBUTE_RO(_name, _uname)					\
+UFS_ATTRIBUTE_SHOW(_name, _uname)					\
+DEVICE_ATTR_RO(_name)
+
+#define UFS_ATTRIBUTE_RW(_name, _uname)					\
+UFS_ATTRIBUTE_SHOW(_name, _uname)					\
+static ssize_t _name##_store(struct device *dev,			\
+		struct device_attribute *attr, const char *buf,		\
+		size_t count)						\
+{									\
+	struct ufs_hba *hba = dev_get_drvdata(dev);			\
+	unsigned long value;						\
+	if (kstrtoul(buf, 0, &value))					\
+		return -EINVAL;						\
+	if (ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,	\
+		QUERY_ATTR_IDN##_uname, 0, 0, (u32 *)&value))		\
+		return -EINVAL;						\
+	return count;							\
+}									\
+static DEVICE_ATTR_RW(_name)
+
+UFS_ATTRIBUTE_RW(boot_lun_enabled, _BOOT_LU_EN);
+UFS_ATTRIBUTE_RO(current_power_mode, _POWER_MODE);
+UFS_ATTRIBUTE_RW(active_icc_level, _ACTIVE_ICC_LVL);
+UFS_ATTRIBUTE_RW(ooo_data_enabled, _OOO_DATA_EN);
+UFS_ATTRIBUTE_RO(bkops_status, _BKOPS_STATUS);
+UFS_ATTRIBUTE_RO(purge_status, _PURGE_STATUS);
+UFS_ATTRIBUTE_RW(max_data_in_size, _MAX_DATA_IN);
+UFS_ATTRIBUTE_RW(max_data_out_size, _MAX_DATA_OUT);
+UFS_ATTRIBUTE_RW(reference_clock_frequency, _REF_CLK_FREQ);
+UFS_ATTRIBUTE_RW(configuration_descriptor_lock, _CONF_DESC_LOCK);
+UFS_ATTRIBUTE_RW(max_number_of_rtt, _MAX_NUM_OF_RTT);
+UFS_ATTRIBUTE_RW(exception_event_control, _EE_CONTROL);
+UFS_ATTRIBUTE_RW(exception_event_status, _EE_STATUS);
+UFS_ATTRIBUTE_RO(ffu_status, _FFU_STATUS);
+UFS_ATTRIBUTE_RO(psa_state, _PSA_STATE);
+UFS_ATTRIBUTE_RO(psa_data_size, _PSA_DATA_SIZE);
 
 static struct attribute *ufs_sysfs_attributes[] = {
 	&dev_attr_boot_lun_enabled.attr,
-- 
2.13.5

Re: [PATCH 3/7] scsi: ufs: Make sysfs attributes writable

[Bart Van Assche]

On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> +static ssize_t _name##_store(struct device *dev,			\
> +		struct device_attribute *attr, const char *buf,		\
> +		size_t count)						\
> +{									\
> +	struct ufs_hba *hba = dev_get_drvdata(dev);			\
> +	unsigned long value;						\
> +	if (kstrtoul(buf, 0, &value))					\
> +		return -EINVAL;						\
> +	if (ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,	\
> +		QUERY_ATTR_IDN##_uname, 0, 0, (u32 *)&value))		\
> +		return -EINVAL;						\
> +	return count;							\
> +}									\

Casting an "unsigned long" pointer into an u32 pointer looks evil to me.
Please don't do this.

Thanks,

Bart.

Re: [PATCH 3/7] scsi: ufs: Make sysfs attributes writable

[Evan Green]

On Mon, Jun 4, 2018 at 1:33 AM Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>
> On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> > +static ssize_t _name##_store(struct device *dev,                     \
> > +             struct device_attribute *attr, const char *buf,         \
> > +             size_t count)                                           \
> > +{                                                                    \
> > +     struct ufs_hba *hba = dev_get_drvdata(dev);                     \
> > +     unsigned long value;                                            \
> > +     if (kstrtoul(buf, 0, &value))                                   \
> > +             return -EINVAL;                                         \
> > +     if (ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,        \
> > +             QUERY_ATTR_IDN##_uname, 0, 0, (u32 *)&value))           \
> > +             return -EINVAL;                                         \
> > +     return count;                                                   \
> > +}                                                                    \
>
> Casting an "unsigned long" pointer into an u32 pointer looks evil to me.
> Please don't do this.
>

Will fix.

[PATCH 4/7] scsi: ufs: sysfs: Document attribute writability

[Evan Green]

This change removes the read-only clauses from the documentation
for UFS attributes, which are now writable.

Signed-off-by: Evan Green <evgreen@chromium.org>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index a1336194628f..da154a0d4b4f 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -859,7 +859,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the boot lun enabled UFS device attribute.
 		The full information about the attribute could be found at
 		UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/current_power_mode
 Date:		February 2018
@@ -867,7 +866,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the current power mode UFS device attribute.
 		The full information about the attribute could be found at
 		UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/active_icc_level
 Date:		February 2018
@@ -875,7 +873,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the active icc level UFS device attribute.
 		The full information about the attribute could be found at
 		UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/ooo_data_enabled
 Date:		February 2018
@@ -883,7 +880,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the out of order data transfer enabled UFS
 		device attribute. The full information about the attribute
 		could be found at UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/bkops_status
 Date:		February 2018
@@ -891,7 +887,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the background operations status UFS device
 		attribute. The full information about the attribute could
 		be found at UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/purge_status
 Date:		February 2018
@@ -899,7 +894,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the purge operation status UFS device
 		attribute. The full information about the attribute could
 		be found at UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/max_data_in_size
 Date:		February 2018
@@ -907,7 +901,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file shows the maximum data size in a DATA IN
 		UPIU. The full information about the attribute could
 		be found at UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/max_data_out_size
 Date:		February 2018
@@ -915,7 +908,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file shows the maximum number of bytes that can be
 		requested with a READY TO TRANSFER UPIU. The full information
 		about the attribute could be found at UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/reference_clock_frequency
 Date:		February 2018
@@ -923,14 +915,13 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the reference clock frequency UFS device
 		attribute. The full information about the attribute could
 		be found at UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/configuration_descriptor_lock
 Date:		February 2018
 Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file shows whether the configuration descriptor is locked.
 		The full information about the attribute could be found at
-		UFS specifications 2.1. The file is read only.
+		UFS specifications 2.1.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/max_number_of_rtt
 Date:		February 2018
@@ -939,7 +930,6 @@ Description:	This file provides the maximum current number of
 		outstanding RTTs in device that is allowed. The full
 		information about the attribute could be found at
 		UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/exception_event_control
 Date:		February 2018
@@ -947,7 +937,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the exception event control UFS device
 		attribute. The full information about the attribute could
 		be found at UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/exception_event_status
 Date:		February 2018
@@ -955,7 +944,6 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the exception event status UFS device
 		attribute. The full information about the attribute could
 		be found at UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/ffu_status
 Date:		February 2018
@@ -963,14 +951,12 @@ Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file provides the ffu status UFS device attribute.
 		The full information about the attribute could be found at
 		UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/psa_state
 Date:		February 2018
 Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
 Description:	This file show the PSA feature status. The full information
 		about the attribute could be found at UFS specifications 2.1.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/psa_data_size
 Date:		February 2018
@@ -979,7 +965,6 @@ Description:	This file shows the amount of data that the host plans to
 		load to all logical units in pre-soldering state.
 		The full information about the attribute could be found at
 		UFS specifications 2.1.
-		The file is read only.
 
 
 What:		/sys/class/scsi_device/*/device/dyn_cap_needed
-- 
2.13.5

Re: [PATCH 4/7] scsi: ufs: sysfs: Document attribute writability

[Bart Van Assche]

On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> This change removes the read-only clauses from the documentation
> for UFS attributes, which are now writable.

Also for this patch, why is this a separate patch instead of having merged
these changes into the patch that makes the attributes writable?

Bart.

Re: [PATCH 4/7] scsi: ufs: sysfs: Document attribute writability

[Evan Green]

On Mon, Jun 4, 2018 at 1:35 AM Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>
> On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> > This change removes the read-only clauses from the documentation
> > for UFS attributes, which are now writable.
>
> Also for this patch, why is this a separate patch instead of having merged
> these changes into the patch that makes the attributes writable?
>
> Bart.

I'll squash it in.

[PATCH 5/7] scsi: ufs: Refactor descriptor read for write

[Evan Green]

This change refactors the ufshcd_read_desc_param function into
one that can both read and write descriptors. Most of the low-level
plumbing for writing to descriptors was already there, this change
simply enables that code path, and manages the incoming data buffer
appropriately.

Signed-off-by: Evan Green <evgreen@chromium.org>
---
 drivers/scsi/ufs/ufs-sysfs.c |  4 +-
 drivers/scsi/ufs/ufshcd.c    | 89 ++++++++++++++++++++++++++++++++------------
 drivers/scsi/ufs/ufshcd.h    | 13 ++++---
 3 files changed, 74 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 51bf54e6b47a..623c56074572 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -227,8 +227,8 @@ static ssize_t ufs_sysfs_read_desc_param(struct ufs_hba *hba,
 	if (param_size > 8)
 		return -EINVAL;
 
-	ret = ufshcd_read_desc_param(hba, desc_id, desc_index,
-				param_offset, desc_buf, param_size);
+	ret = ufshcd_rw_desc_param(hba, UPIU_QUERY_OPCODE_READ_DESC, desc_id,
+				desc_index, param_offset, desc_buf, param_size);
 	if (ret)
 		return -EINVAL;
 	switch (param_size) {
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 00e79057f870..6a5939fb5da3 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3004,22 +3004,24 @@ int ufshcd_map_desc_id_to_length(struct ufs_hba *hba,
 EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
 
 /**
- * ufshcd_read_desc_param - read the specified descriptor parameter
+ * ufshcd_rw_desc_param - read or write the specified descriptor parameter
  * @hba: Pointer to adapter instance
+ * @opcode: indicates whether to read or write
  * @desc_id: descriptor idn value
  * @desc_index: descriptor index
- * @param_offset: offset of the parameter to read
- * @param_read_buf: pointer to buffer where parameter would be read
- * @param_size: sizeof(param_read_buf)
+ * @param_offset: offset of the parameter to read or write
+ * @param_buf: pointer to buffer to be read or written
+ * @param_size: sizeof(param_buf)
  *
  * Return 0 in case of success, non-zero otherwise
  */
-int ufshcd_read_desc_param(struct ufs_hba *hba,
-			   enum desc_idn desc_id,
-			   int desc_index,
-			   u8 param_offset,
-			   u8 *param_read_buf,
-			   u8 param_size)
+int ufshcd_rw_desc_param(struct ufs_hba *hba,
+			 enum query_opcode opcode,
+			 enum desc_idn desc_id,
+			 int desc_index,
+			 u8 param_offset,
+			 u8 *param_buf,
+			 u8 param_size)
 {
 	int ret;
 	u8 *desc_buf;
@@ -3042,24 +3044,57 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 		return ret;
 	}
 
+	if (param_offset > buff_len)
+		return 0;
+
 	/* Check whether we need temp memory */
 	if (param_offset != 0 || param_size < buff_len) {
-		desc_buf = kmalloc(buff_len, GFP_KERNEL);
+		desc_buf = kzalloc(buff_len, GFP_KERNEL);
 		if (!desc_buf)
 			return -ENOMEM;
+
+		/* If it's a write, first read the complete descriptor, then
+		 * copy in the parts being changed.
+		 */
+		if (opcode == UPIU_QUERY_OPCODE_WRITE_DESC) {
+			if ((int)param_offset + (int)param_size > buff_len) {
+				ret = -EINVAL;
+				goto out;
+			}
+
+			ret = ufshcd_query_descriptor_retry(hba,
+						UPIU_QUERY_OPCODE_READ_DESC,
+						desc_id, desc_index, 0,
+						desc_buf, &buff_len);
+
+			if (ret) {
+				dev_err(hba->dev,
+					"%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d",
+					__func__, desc_id, desc_index,
+					param_offset, ret);
+
+				goto out;
+			}
+
+			memcpy(desc_buf + param_offset, param_buf, param_size);
+		}
+
 	} else {
-		desc_buf = param_read_buf;
+		desc_buf = param_buf;
 		is_kmalloc = false;
 	}
 
-	/* Request for full descriptor */
-	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
+	/* Read or write the entire descriptor. */
+	ret = ufshcd_query_descriptor_retry(hba, opcode,
 					desc_id, desc_index, 0,
 					desc_buf, &buff_len);
 
 	if (ret) {
-		dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d",
-			__func__, desc_id, desc_index, param_offset, ret);
+		dev_err(hba->dev, "%s: Failed %s descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d",
+			__func__,
+			opcode == UPIU_QUERY_OPCODE_READ_DESC ?
+			"reading" : "writing",
+			desc_id, desc_index, param_offset, ret);
 		goto out;
 	}
 
@@ -3071,12 +3106,16 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 		goto out;
 	}
 
-	/* Check wherher we will not copy more data, than available */
-	if (is_kmalloc && param_size > buff_len)
-		param_size = buff_len;
+	/* Copy data to the output. The offset is already validated to be
+	 * within the buffer.
+	 */
+	if (is_kmalloc && (opcode == UPIU_QUERY_OPCODE_READ_DESC)) {
+		if ((int)param_offset + (int)param_size > buff_len)
+			param_size = buff_len - param_offset;
+
+		memcpy(param_buf, &desc_buf[param_offset], param_size);
+	}
 
-	if (is_kmalloc)
-		memcpy(param_read_buf, &desc_buf[param_offset], param_size);
 out:
 	if (is_kmalloc)
 		kfree(desc_buf);
@@ -3089,7 +3128,8 @@ static inline int ufshcd_read_desc(struct ufs_hba *hba,
 				   u8 *buf,
 				   u32 size)
 {
-	return ufshcd_read_desc_param(hba, desc_id, desc_index, 0, buf, size);
+	return ufshcd_rw_desc_param(hba, UPIU_QUERY_OPCODE_READ_DESC, desc_id,
+					desc_index, 0, buf, size);
 }
 
 static inline int ufshcd_read_power_desc(struct ufs_hba *hba,
@@ -3195,8 +3235,9 @@ static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba,
 	if (!ufs_is_valid_unit_desc_lun(lun))
 		return -EOPNOTSUPP;
 
-	return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun,
-				      param_offset, param_read_buf, param_size);
+	return ufshcd_rw_desc_param(hba, UPIU_QUERY_OPCODE_READ_DESC,
+				    QUERY_DESC_IDN_UNIT, lun, param_offset,
+				    param_read_buf, param_size);
 }
 
 /**
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index fb6dcc490f21..ebbd9d580c4b 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -853,12 +853,13 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
 				  enum desc_idn idn, u8 index,
 				  u8 selector,
 				  u8 *desc_buf, int *buf_len);
-int ufshcd_read_desc_param(struct ufs_hba *hba,
-			   enum desc_idn desc_id,
-			   int desc_index,
-			   u8 param_offset,
-			   u8 *param_read_buf,
-			   u8 param_size);
+int ufshcd_rw_desc_param(struct ufs_hba *hba,
+			 enum query_opcode opcode,
+			 enum desc_idn desc_id,
+			 int desc_index,
+			 u8 param_offset,
+			 u8 *param_read_buf,
+			 u8 param_size);
 int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
 		      enum attr_idn idn, u8 index, u8 selector, u32 *attr_val);
 int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
-- 
2.13.5

Re: [PATCH 5/7] scsi: ufs: Refactor descriptor read for write

[Evan Green]

Reviewing my own patch...

On Tue, May 29, 2018 at 11:18 AM Evan Green <evgreen@chromium.org> wrote:

> This change refactors the ufshcd_read_desc_param function into
> one that can both read and write descriptors. Most of the low-level
> plumbing for writing to descriptors was already there, this change
> simply enables that code path, and manages the incoming data buffer
> appropriately.

> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>   drivers/scsi/ufs/ufs-sysfs.c |  4 +-
>   drivers/scsi/ufs/ufshcd.c    | 89
++++++++++++++++++++++++++++++++------------
>   drivers/scsi/ufs/ufshcd.h    | 13 ++++---
>   3 files changed, 74 insertions(+), 32 deletions(-)

...
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 00e79057f870..6a5939fb5da3 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3004,22 +3004,24 @@ int ufshcd_map_desc_id_to_length(struct ufs_hba
*hba,
>   EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);

>   /**
> - * ufshcd_read_desc_param - read the specified descriptor parameter
> + * ufshcd_rw_desc_param - read or write the specified descriptor
parameter
>    * @hba: Pointer to adapter instance
> + * @opcode: indicates whether to read or write
>    * @desc_id: descriptor idn value
>    * @desc_index: descriptor index
> - * @param_offset: offset of the parameter to read
> - * @param_read_buf: pointer to buffer where parameter would be read
> - * @param_size: sizeof(param_read_buf)
> + * @param_offset: offset of the parameter to read or write
> + * @param_buf: pointer to buffer to be read or written
> + * @param_size: sizeof(param_buf)
>    *
>    * Return 0 in case of success, non-zero otherwise
>    */
> -int ufshcd_read_desc_param(struct ufs_hba *hba,
> -                          enum desc_idn desc_id,
> -                          int desc_index,
> -                          u8 param_offset,
> -                          u8 *param_read_buf,
> -                          u8 param_size)
> +int ufshcd_rw_desc_param(struct ufs_hba *hba,
> +                        enum query_opcode opcode,
> +                        enum desc_idn desc_id,
> +                        int desc_index,
> +                        u8 param_offset,
> +                        u8 *param_buf,
> +                        u8 param_size)
>   {
>          int ret;
>          u8 *desc_buf;
> @@ -3042,24 +3044,57 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
>                  return ret;
>          }

> +       if (param_offset > buff_len)
> +               return 0;
> +
>          /* Check whether we need temp memory */
>          if (param_offset != 0 || param_size < buff_len) {
> -               desc_buf = kmalloc(buff_len, GFP_KERNEL);
> +               desc_buf = kzalloc(buff_len, GFP_KERNEL);
>                  if (!desc_buf)
>                          return -ENOMEM;
> +
> +               /* If it's a write, first read the complete descriptor,
then
> +                * copy in the parts being changed.
> +                */
> +               if (opcode == UPIU_QUERY_OPCODE_WRITE_DESC) {
> +                       if ((int)param_offset + (int)param_size >
buff_len) {
> +                               ret = -EINVAL;
> +                               goto out;
> +                       }
> +
> +                       ret = ufshcd_query_descriptor_retry(hba,
> +
UPIU_QUERY_OPCODE_READ_DESC,
> +                                               desc_id, desc_index, 0,
> +                                               desc_buf, &buff_len);
> +
> +                       if (ret) {
> +                               dev_err(hba->dev,
> +                                       "%s: Failed reading descriptor.
desc_id %d, desc_index %d, param_offset %d, ret %d",
> +                                       __func__, desc_id, desc_index,
> +                                       param_offset, ret);
> +
> +                               goto out;
> +                       }
> +
> +                       memcpy(desc_buf + param_offset, param_buf,
param_size);
> +               }
> +
>          } else {
> -               desc_buf = param_read_buf;
> +               desc_buf = param_buf;
>                  is_kmalloc = false;
>          }

> -       /* Request for full descriptor */
> -       ret = ufshcd_query_descriptor_retry(hba,
UPIU_QUERY_OPCODE_READ_DESC,
> +       /* Read or write the entire descriptor. */
> +       ret = ufshcd_query_descriptor_retry(hba, opcode,
>                                          desc_id, desc_index, 0,
>                                          desc_buf, &buff_len);

>          if (ret) {
> -               dev_err(hba->dev, "%s: Failed reading descriptor. desc_id
%d, desc_index %d, param_offset %d, ret %d",
> -                       __func__, desc_id, desc_index, param_offset, ret);
> +               dev_err(hba->dev, "%s: Failed %s descriptor. desc_id %d,
desc_index %d, param_offset %d, ret %d",
> +                       __func__,
> +                       opcode == UPIU_QUERY_OPCODE_READ_DESC ?
> +                       "reading" : "writing",
> +                       desc_id, desc_index, param_offset, ret);
>                  goto out;
>          }

> @@ -3071,12 +3106,16 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
>                  goto out;
>          }

> -       /* Check wherher we will not copy more data, than available */
> -       if (is_kmalloc && param_size > buff_len)
> -               param_size = buff_len;
> +       /* Copy data to the output. The offset is already validated to be
> +        * within the buffer.
> +        */
> +       if (is_kmalloc && (opcode == UPIU_QUERY_OPCODE_READ_DESC)) {
> +               if ((int)param_offset + (int)param_size > buff_len)
> +                       param_size = buff_len - param_offset;
> +
> +               memcpy(param_buf, &desc_buf[param_offset], param_size);
> +       }

Previously this function didn't handle param_offset being non-zero well at
all, as it would allow you to read off the end of the descriptor. My hunk
here, as well as the "if (param_offset > buff_len) return 0;" above prevent
reading off the end of the kmalloced buffer. But it will return success
while leaving the caller's buffer uninitialized, which I think is asking
for trouble. I think I should fail if the starting offset is greater than
the size. If the requested read starts at a valid index but ends beyond the
end, I think I should clear the remaining buffer. ufshcd_read_string_desc,
for example, always asks for the max, and would need this bit of leniency.
-Evan

Re: [PATCH 5/7] scsi: ufs: Refactor descriptor read for write

[Bart Van Assche]

On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
>  	/* Check whether we need temp memory */
>  	if (param_offset != 0 || param_size < buff_len) {
> -		desc_buf = kmalloc(buff_len, GFP_KERNEL);
> +		desc_buf = kzalloc(buff_len, GFP_KERNEL);
>  		if (!desc_buf)
>  			return -ENOMEM;
> +
> +		/* If it's a write, first read the complete descriptor, then
> +		 * copy in the parts being changed.
> +		 */

Have you verified this patch with checkpatch? The above comment does not follow
the Linux kernel coding style.

> +		if (opcode == UPIU_QUERY_OPCODE_WRITE_DESC) {
> +			if ((int)param_offset + (int)param_size > buff_len) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +
> +			ret = ufshcd_query_descriptor_retry(hba,
> +						UPIU_QUERY_OPCODE_READ_DESC,
> +						desc_id, desc_index, 0,
> +						desc_buf, &buff_len);
> +
> +			if (ret) {
> +				dev_err(hba->dev,
> +					"%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d",
> +					__func__, desc_id, desc_index,
> +					param_offset, ret);
> +
> +				goto out;
> +			}
> +
> +			memcpy(desc_buf + param_offset, param_buf, param_size);
> +		}

The above code is indented deeply. I think that means that this code would become
easier to read if a helper function would be introduced.

Additionally, I think locking is missing from the above code. How else can race
conditions between concurrent writers be prevented?

Bart.

Re: [PATCH 5/7] scsi: ufs: Refactor descriptor read for write

[Evan Green]

On Mon, Jun 4, 2018 at 1:41 AM Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>
> On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> >       /* Check whether we need temp memory */
> >       if (param_offset != 0 || param_size < buff_len) {
> > -             desc_buf = kmalloc(buff_len, GFP_KERNEL);
> > +             desc_buf = kzalloc(buff_len, GFP_KERNEL);
> >               if (!desc_buf)
> >                       return -ENOMEM;
> > +
> > +             /* If it's a write, first read the complete descriptor, then
> > +              * copy in the parts being changed.
> > +              */
>
> Have you verified this patch with checkpatch? The above comment does not follow
> the Linux kernel coding style.

Yes, but I probably forgot to add that switch that turns on even more
checks. Will fix.

>
> > +             if (opcode == UPIU_QUERY_OPCODE_WRITE_DESC) {
> > +                     if ((int)param_offset + (int)param_size > buff_len) {
> > +                             ret = -EINVAL;
> > +                             goto out;
> > +                     }
> > +
> > +                     ret = ufshcd_query_descriptor_retry(hba,
> > +                                             UPIU_QUERY_OPCODE_READ_DESC,
> > +                                             desc_id, desc_index, 0,
> > +                                             desc_buf, &buff_len);
> > +
> > +                     if (ret) {
> > +                             dev_err(hba->dev,
> > +                                     "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d",
> > +                                     __func__, desc_id, desc_index,
> > +                                     param_offset, ret);
> > +
> > +                             goto out;
> > +                     }
> > +
> > +                     memcpy(desc_buf + param_offset, param_buf, param_size);
> > +             }
>
> The above code is indented deeply. I think that means that this code would become
> easier to read if a helper function would be introduced.

Ok.
>
> Additionally, I think locking is missing from the above code. How else can race
> conditions between concurrent writers be prevented?

Hm, yeah I think this followed along with my thinking that there
wouldn't be multiple processes provisioning at once. This function
will always write a consistent version of one caller's view, but
multiple callers might clobber each other's writes. I can explore
adding locking.
-Evan

[PATCH 6/7] scsi: ufs: Enable writing config descriptor

[Evan Green]

This change enables writing to fields of the config descriptor
via sysfs. It can be used to provision an unprovisioned UFS
device, or reprovision an unlocked device.

Signed-off-by: Evan Green <evgreen@chromium.org>
---
 drivers/scsi/ufs/ufs-sysfs.c | 64 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 623c56074572..54e9f3bca9db 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -252,6 +252,31 @@ static ssize_t ufs_sysfs_read_desc_param(struct ufs_hba *hba,
 	return ret;
 }
 
+static ssize_t ufs_sysfs_write_desc_param(struct ufs_hba *hba,
+				  enum desc_idn desc_id,
+				  u8 desc_index,
+				  u8 param_offset,
+				  const char *buf,
+				  ssize_t buf_size,
+				  u8 width)
+{
+	int ret;
+	unsigned long long value;
+
+	if (kstrtoull(buf, 0, &value))
+		return -EINVAL;
+
+	/* Convert to big endian, and send only the appropriate width. */
+	value = cpu_to_be64(value);
+	ret = ufshcd_rw_desc_param(hba, UPIU_QUERY_OPCODE_WRITE_DESC, desc_id,
+				desc_index, param_offset,
+				(u8 *)&value + 8 - width, width);
+	if (ret)
+		return -EINVAL;
+
+	return buf_size;
+}
+
 #define UFS_DESC_PARAM(_name, _puname, _duname, _size)			\
 static ssize_t _name##_show(struct device *dev,				\
 	struct device_attribute *attr, char *buf)			\
@@ -357,8 +382,25 @@ static ssize_t cfg_unit_store(struct device *dev,
 	return count;
 }
 
-#define UFS_CONFIG_DESC_PARAM(_name, _uname, _size)			\
-	UFS_DESC_PARAM(cfg_##_name, _uname, CONFIGURATION, _size)
+#define UFS_CONFIG_DESC_PARAM(_name, _puname, _size)			\
+static ssize_t cfg_##_name##_show(struct device *dev,			\
+	struct device_attribute *attr, char *buf)			\
+{									\
+	struct ufs_hba *hba = dev_get_drvdata(dev);			\
+	return ufs_sysfs_read_desc_param(hba,				\
+		QUERY_DESC_IDN_CONFIGURATION, 0,			\
+		CONFIGURATION_DESC_PARAM##_puname, buf, _size);		\
+}									\
+static ssize_t cfg_##_name##_store(struct device *dev,			\
+		struct device_attribute *attr, const char *buf,		\
+		size_t count)						\
+{									\
+	struct ufs_hba *hba = dev_get_drvdata(dev);			\
+	return ufs_sysfs_write_desc_param(hba,				\
+		QUERY_DESC_IDN_CONFIGURATION, 0,			\
+		CONFIGURATION_DESC_PARAM##_puname, buf, count, _size);	\
+}									\
+static DEVICE_ATTR_RW(cfg_##_name)
 
 #define UFS_CONFIG_UNIT_DESC_PARAM(_name, _uname, _size)		\
 static ssize_t unit_##_name##_show(struct device *dev,			\
@@ -375,7 +417,23 @@ static ssize_t unit_##_name##_show(struct device *dev,			\
 	return ufs_sysfs_read_desc_param(hba,				\
 		QUERY_DESC_IDN_CONFIGURATION, 0, offset, buf, _size);	\
 }									\
-static DEVICE_ATTR_RO(unit_##_name)
+static ssize_t unit_##_name##_store(struct device *dev,			\
+		struct device_attribute *attr, const char *buf,		\
+		size_t count)						\
+{									\
+	struct ufs_hba *hba = dev_get_drvdata(dev);			\
+	size_t offset = CONFIGURATION_DESC_PARAM_UNIT0 +		\
+		(hba->sysfs_config_unit *				\
+		 CONFIGURATION_UNIT_DESC_SIZE) +			\
+		 CONFIGURATION_UNIT_DESC_PARAM_##_uname;		\
+	if (offset + _size > hba->desc_size.conf_desc) {		\
+		return -EINVAL;						\
+	}								\
+	return ufs_sysfs_write_desc_param(hba,				\
+		QUERY_DESC_IDN_CONFIGURATION, 0, offset, buf, count,	\
+		_size);							\
+}									\
+static DEVICE_ATTR_RW(unit_##_name)
 
 UFS_CONFIG_DESC_PARAM(number_of_luns, _NUM_LU, 1);
 UFS_CONFIG_DESC_PARAM(boot_enable, _BOOT_ENBL, 1);
-- 
2.13.5

Re: [PATCH 6/7] scsi: ufs: Enable writing config descriptor

[Bart Van Assche]

On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> This change enables writing to fields of the config descriptor
> via sysfs. It can be used to provision an unprovisioned UFS
> device, or reprovision an unlocked device.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>  drivers/scsi/ufs/ufs-sysfs.c | 64 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 61 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> index 623c56074572..54e9f3bca9db 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -252,6 +252,31 @@ static ssize_t ufs_sysfs_read_desc_param(struct ufs_hba *hba,
>  	return ret;
>  }
>  
> +static ssize_t ufs_sysfs_write_desc_param(struct ufs_hba *hba,
> +				  enum desc_idn desc_id,
> +				  u8 desc_index,
> +				  u8 param_offset,
> +				  const char *buf,
> +				  ssize_t buf_size,
> +				  u8 width)
> +{
> +	int ret;
> +	unsigned long long value;
> +
> +	if (kstrtoull(buf, 0, &value))
> +		return -EINVAL;
> +
> +	/* Convert to big endian, and send only the appropriate width. */
> +	value = cpu_to_be64(value);
> +	ret = ufshcd_rw_desc_param(hba, UPIU_QUERY_OPCODE_WRITE_DESC, desc_id,
> +				desc_index, param_offset,
> +				(u8 *)&value + 8 - width, width);

Instead of using the above construct, please provide separate versions of
ufs_sysfs_write_desc_param() and ufshcd_rw_desc_param() for each supported
value of "width" (8, 16, 32 and/or 64) and also use the __be8 / __be16 /
__be32 / __be64 annotations for big endian numbers such that it becomes
possible to verify endianness correctness with sparse.

Thanks,

Bart.

[PATCH 7/7] scsi: ufs: Update config descriptor documentation

[Evan Green]

This change updates the sysfs documentation for the UFS config
descriptor, now that config descriptor fields are writable.

Signed-off-by: Evan Green <evgreen@chromium.org>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index da154a0d4b4f..d11b0a91f4b2 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -244,7 +244,6 @@ Description:	This file shows whether or not the UFS boot feature is enabled.
 		This is one of the UFS configuration descriptor parameters.
 		More information about the descriptor can be found in the UFS
 		2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_descriptor_access_enable
 Date:		May 2018
@@ -254,7 +253,6 @@ Description:	This file shows whether or not access will be permitted to the
 		boot sequence. This is one of the UFS configuration descriptor
 		parameters. More information about the descriptor can be found
 		in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_high_priority_lun
 Date:		May 2018
@@ -263,7 +261,6 @@ Description:	This file shows the identifier of the high priority logical
 		unit. This is one of the UFS configuration descriptor
 		parameters. More information about the descriptor can be found
 		in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_init_active_icc_level
 Date:		May 2018
@@ -272,7 +269,6 @@ Description:	This file shows the ICC level in active mode after device
 		initialization or hardware reset. This is one of the UFS
 		configuration descriptor parameters. More information about the
 		descriptor can be found in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_initial_power_mode
 Date:		May 2018
@@ -281,7 +277,6 @@ Description:	This file shows the power mode after device initialization or
 		hardware reset. This is one of the UFS configuration descriptor
 		parameters. More information about the descriptor can be found
 		in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_number_of_luns
 Date:		May 2018
@@ -290,7 +285,6 @@ Description:	This file shows the number of logical units that the device will
 		support. This is one of the UFS configuration descriptor
 		parameters. More information about the descriptor can be found
 		in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_periodic_rtc_update
 Date:		May 2018
@@ -299,7 +293,6 @@ Description:	This file shows the frequency and method of real time clock
 		updates. This is one of the UFS configuration descriptor
 		parameters. More information about the descriptor can be found
 		in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_secure_removal_type
 Date:		May 2018
@@ -308,7 +301,6 @@ Description:	This file shows the secure removal type of the UFS device. This
 		is one of the UFS configuration descriptor parameters. More
 		information about the descriptor can be found in the UFS 2.1
 		specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/cfg_unit
 Date:		May 2018
@@ -327,7 +319,6 @@ Description:	This file shows the number of allocation units assigned to the
 		being displayed. This is one of the UFS configuration unit
 		descriptor parameters. More information about the descriptor
 		can be found in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_boot_lun_id
 Date:		May 2018
@@ -338,7 +329,6 @@ Description:	This file shows the boot LUN ID for this logical unit,
 		This is one of the UFS configuration unit descriptor parameters.
 		More information about the descriptor can be found in the UFS
 		2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_context_capabilities
 Date:		May 2018
@@ -348,7 +338,6 @@ Description:	This file shows the context capabilities for the logical unit.
 		being displayed. This is one of the UFS configuration unit
 		descriptor parameters. More information about the descriptor
 		can be found in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_data_reliability
 Date:		May 2018
@@ -358,7 +347,6 @@ Description:	This file shows the data reliability for the logical unit.
 		being displayed. This is one of the UFS configuration unit
 		descriptor parameters. More information about the descriptor
 		can be found in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_logical_block_size
 Date:		May 2018
@@ -368,7 +356,6 @@ Description:	This file shows the logical block size for the logical unit as
 		being displayed. This is one of the UFS configuration unit
 		descriptor parameters. More information about the descriptor
 		can be found in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_lu_enable
 Date:		May 2018
@@ -378,7 +365,6 @@ Description:	This file shows whether or not the logical unit is enabled.
 		being displayed. This is one of the UFS configuration unit
 		descriptor parameters. More information about the descriptor
 		can be found in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_lu_write_protect
 Date:		May 2018
@@ -388,7 +374,6 @@ Description:	This file shows the write protect status for the logical unit.
 		being displayed. This is one of the UFS configuration unit
 		descriptor parameters. More information about the descriptor
 		can be found in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_memory_type
 Date:		May 2018
@@ -398,7 +383,6 @@ Description:	This file shows the memory type for the logical unit.
 		being displayed. This is one of the UFS configuration unit
 		descriptor parameters. More information about the descriptor
 		can be found in the UFS 2.1 specification.
-		The file is read only.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/unit_provisioning_type
 Date:		May 2018
@@ -408,7 +392,6 @@ Description:	This file shows the provisioning type information for the
 		being displayed. This is one of the UFS configuration unit
 		descriptor parameters. More information about the descriptor
 		can be found in the UFS 2.1 specification.
-		The file is read only.
 
 
 What:		/sys/bus/platform/drivers/ufshcd/*/interconnect_descriptor/unipro_version
-- 
2.13.5

RE: [PATCH 0/7] Enable UFS provisioning via Linux

[Stanislav Nijnikov]

Hi Evan,
I have some generic notes:
- Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and unit descriptors? And the sysfs representation of the device and unit descriptors is existing already.
- It would be nice to have some "packet" mode allowing to gather configuration changes and apply them at once, not one by one.
- Why to put documentation update in the separate patches?

Regards
Stanislav

> -----Original Message-----
> From: Evan Green <evgreen@chromium.org>
> Sent: Tuesday, May 29, 2018 9:18 PM
> To: Vinayak Holikatti <vinholikatti@gmail.com>; James E.J. Bottomley <jejb@linux.vnet.ibm.com>; Martin K. Petersen
> <martin.petersen@oracle.com>; Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>; linux-kernel@vger.kernel.org; linux-
> scsi@vger.kernel.org
> Cc: Gwendal Grignou <gwendal@chromium.org>; Evan Green <evgreen@chromium.org>
> Subject: [PATCH 0/7] Enable UFS provisioning via Linux
> 
> This series enables provisioning UFS devices using the existing sysfs
> interface. This functionality is primarily useful along the assembly
> line, but might also be useful for end users that receive devices that
> aren't locked down.
> 
> Evan Green (7):
>   scsi: ufs: Add Configuration Descriptor to sysfs
>   scsi: ufs: Add config descriptor documentation
>   scsi: ufs: Make sysfs attributes writable
>   scsi: ufs: sysfs: Document attribute writability
>   scsi: ufs: Refactor descriptor read for write
>   scsi: ufs: Enable writing config descriptor
>   scsi: ufs: Update config descriptor documentation
> 
>  Documentation/ABI/testing/sysfs-driver-ufs | 174 ++++++++++++++++++++---
>  drivers/scsi/ufs/ufs-sysfs.c               | 217 ++++++++++++++++++++++++++---
>  drivers/scsi/ufs/ufs.h                     |  29 ++++
>  drivers/scsi/ufs/ufshcd.c                  |  89 ++++++++----
>  drivers/scsi/ufs/ufshcd.h                  |  16 ++-
>  5 files changed, 458 insertions(+), 67 deletions(-)
> 
> --
> 2.13.5

Re: [PATCH 0/7] Enable UFS provisioning via Linux

[Evan Green]

Hi Stanislav. Thanks for taking a look. Responses below.

On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
<Stanislav.Nijnikov@wdc.com> wrote:
>
> Hi Evan,
> I have some generic notes:
> - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and unit descriptors? And the sysfs representation of the device and unit descriptors is existing already.

Well, UFS describes them as different descriptors. I worry that if I
add a bunch of clever logic to hide the config descriptor behind other
descriptors, there might be trouble later if 1) there is a quirky
device that doesn't reflect the values between descriptors quite the
same way or at the same time, or 2) if a later UFS spec adds more
configuration descriptor fields that don't exactly reflect into other
non-config descriptors, the cleverness will look awkward.

> - It would be nice to have some "packet" mode allowing to gather configuration changes and apply them at once, not one by one.

That's definitely doable. Do you think it's needed? I suppose if there
were a device that truly allowed you to do only a single write to the
config descriptor, then the commit style would be needed. The two
devices I've tested (Toshiba and Samsung) allow multiple writes to the
config descriptor, which makes me lean towards not needing the
batch-and-commit style, since if you get interrupted you can simply
try again. I'm happy to do either, though.

> - Why to put documentation update in the separate patches?
Well, in case some piece of this turned out to be controversial, I
wanted to allow for the option of taking these changes independently,
without the concern of missing the documentation. I'm happy to squash
all the documentation changes into one if that's preferred.

-Evan

RE: [PATCH 0/7] Enable UFS provisioning via Linux

[Stanislav Nijnikov]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> On Behalf Of Evan Green
> Sent: Friday, June 1, 2018 5:44 PM
> To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> 
> Hi Stanislav. Thanks for taking a look. Responses below.
> 
> On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
> <Stanislav.Nijnikov@wdc.com> wrote:
> >
> > Hi Evan,
> > I have some generic notes:
> > - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and unit
> descriptors? And the sysfs representation of the device and unit descriptors is existing already.
> 
> Well, UFS describes them as different descriptors. I worry that if I
> add a bunch of clever logic to hide the config descriptor behind other
> descriptors, there might be trouble later if 1) there is a quirky
> device that doesn't reflect the values between descriptors quite the
> same way or at the same time, or 2) if a later UFS spec adds more
> configuration descriptor fields that don't exactly reflect into other
> non-config descriptors, the cleverness will look awkward.
 
No additional logic will be required to attach write functionality to the
existing entries instead of new defined ones. It will reduce the patch 
size significantly. And there will be no need for the unit selector 
mechanism which I'm not sure will be accepted by the SCSI community.

> 
> > - It would be nice to have some "packet" mode allowing to gather configuration changes and apply them at once, not one by one.
> 
> That's definitely doable. Do you think it's needed? I suppose if there
> were a device that truly allowed you to do only a single write to the
> config descriptor, then the commit style would be needed. The two
> devices I've tested (Toshiba and Samsung) allow multiple writes to the
> config descriptor, which makes me lean towards not needing the
> batch-and-commit style, since if you get interrupted you can simply
> try again. I'm happy to do either, though.

Agree. It doesn't have to be part of this patch set. It's just thoughts  
about "quirky devices and future changes" :) that might require to 
change several parameters simultaneously.

> 
> > - Why to put documentation update in the separate patches?
> Well, in case some piece of this turned out to be controversial, I
> wanted to allow for the option of taking these changes independently,
> without the concern of missing the documentation. I'm happy to squash
> all the documentation changes into one if that's preferred.
> 
> -Evan

Regards
Stanislav

Re: [PATCH 0/7] Enable UFS provisioning via Linux

[Evan Green]

On Sun, Jun 3, 2018 at 3:21 AM Stanislav Nijnikov
<Stanislav.Nijnikov@wdc.com> wrote:
>
>
>
> > -----Original Message-----
> > From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> On Behalf Of Evan Green
> > Sent: Friday, June 1, 2018 5:44 PM
> > To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> > Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> > kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> > <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> > Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> >
> > Hi Stanislav. Thanks for taking a look. Responses below.
> >
> > On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
> > <Stanislav.Nijnikov@wdc.com> wrote:
> > >
> > > Hi Evan,
> > > I have some generic notes:
> > > - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and unit
> > descriptors? And the sysfs representation of the device and unit descriptors is existing already.
> >
> > Well, UFS describes them as different descriptors. I worry that if I
> > add a bunch of clever logic to hide the config descriptor behind other
> > descriptors, there might be trouble later if 1) there is a quirky
> > device that doesn't reflect the values between descriptors quite the
> > same way or at the same time, or 2) if a later UFS spec adds more
> > configuration descriptor fields that don't exactly reflect into other
> > non-config descriptors, the cleverness will look awkward.
>
> No additional logic will be required to attach write functionality to the
> existing entries instead of new defined ones. It will reduce the patch
> size significantly. And there will be no need for the unit selector
> mechanism which I'm not sure will be accepted by the SCSI community.
>

So this would be modifying the existing sysfs entries so that reads
still come from the device and unit descriptors, but writes go to
equivalent fields in the config descriptor? I can explore that
approach. Alternatively, if the unit selector mechanism is not
desired, I could dynamically create sysfs directories for each unit in
the config descriptor, but still bring out the config descriptor
values as separate entries. (I still worry a bit about smashing the
descriptors together as the UFS spec called them out as different).

-Evan

Re: [PATCH 0/7] Enable UFS provisioning via Linux

[Adrian Hunter]

On 04/06/18 17:59, Evan Green wrote:
> On Sun, Jun 3, 2018 at 3:21 AM Stanislav Nijnikov
> <Stanislav.Nijnikov@wdc.com> wrote:
>>
>>
>>
>>> -----Original Message-----
>>> From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> On Behalf Of Evan Green
>>> Sent: Friday, June 1, 2018 5:44 PM
>>> To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
>>> Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
>>> kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
>>> <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
>>> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
>>>
>>> Hi Stanislav. Thanks for taking a look. Responses below.
>>>
>>> On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
>>> <Stanislav.Nijnikov@wdc.com> wrote:
>>>>
>>>> Hi Evan,
>>>> I have some generic notes:
>>>> - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and unit
>>> descriptors? And the sysfs representation of the device and unit descriptors is existing already.
>>>
>>> Well, UFS describes them as different descriptors. I worry that if I
>>> add a bunch of clever logic to hide the config descriptor behind other
>>> descriptors, there might be trouble later if 1) there is a quirky
>>> device that doesn't reflect the values between descriptors quite the
>>> same way or at the same time, or 2) if a later UFS spec adds more
>>> configuration descriptor fields that don't exactly reflect into other
>>> non-config descriptors, the cleverness will look awkward.
>>
>> No additional logic will be required to attach write functionality to the
>> existing entries instead of new defined ones. It will reduce the patch
>> size significantly. And there will be no need for the unit selector
>> mechanism which I'm not sure will be accepted by the SCSI community.
>>
> 
> So this would be modifying the existing sysfs entries so that reads
> still come from the device and unit descriptors, but writes go to
> equivalent fields in the config descriptor? I can explore that
> approach. Alternatively, if the unit selector mechanism is not
> desired, I could dynamically create sysfs directories for each unit in
> the config descriptor, but still bring out the config descriptor
> values as separate entries. (I still worry a bit about smashing the
> descriptors together as the UFS spec called them out as different).

If you use the unit attributes, how do you configure units that do not yet
exist?

Perhaps it is better to represent the configuration descriptors exactly as
they are defined in the specification.  Probably not worth exposing them at
all if the configuration is locked (attribute bConfigDescrLock == 1).

Note also that the 2.1 spec. defines  bConfDescContinue which allows updates
to be grouped and committed together.

RE: [PATCH 0/7] Enable UFS provisioning via Linux

[Stanislav Nijnikov]

Hi Adrian,

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Friday, June 8, 2018 3:31 PM
> To: Evan Green <evgreen@chromium.org>; Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> 
> On 04/06/18 17:59, Evan Green wrote:
> > On Sun, Jun 3, 2018 at 3:21 AM Stanislav Nijnikov
> > <Stanislav.Nijnikov@wdc.com> wrote:
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> On Behalf Of Evan Green
> >>> Sent: Friday, June 1, 2018 5:44 PM
> >>> To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> >>> Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> >>> kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> >>> <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> >>> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> >>>
> >>> Hi Stanislav. Thanks for taking a look. Responses below.
> >>>
> >>> On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
> >>> <Stanislav.Nijnikov@wdc.com> wrote:
> >>>>
> >>>> Hi Evan,
> >>>> I have some generic notes:
> >>>> - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and unit
> >>> descriptors? And the sysfs representation of the device and unit descriptors is existing already.
> >>>
> >>> Well, UFS describes them as different descriptors. I worry that if I
> >>> add a bunch of clever logic to hide the config descriptor behind other
> >>> descriptors, there might be trouble later if 1) there is a quirky
> >>> device that doesn't reflect the values between descriptors quite the
> >>> same way or at the same time, or 2) if a later UFS spec adds more
> >>> configuration descriptor fields that don't exactly reflect into other
> >>> non-config descriptors, the cleverness will look awkward.
> >>
> >> No additional logic will be required to attach write functionality to the
> >> existing entries instead of new defined ones. It will reduce the patch
> >> size significantly. And there will be no need for the unit selector
> >> mechanism which I'm not sure will be accepted by the SCSI community.
> >>
> >
> > So this would be modifying the existing sysfs entries so that reads
> > still come from the device and unit descriptors, but writes go to
> > equivalent fields in the config descriptor? I can explore that
> > approach. Alternatively, if the unit selector mechanism is not
> > desired, I could dynamically create sysfs directories for each unit in
> > the config descriptor, but still bring out the config descriptor
> > values as separate entries. (I still worry a bit about smashing the
> > descriptors together as the UFS spec called them out as different).
> 
> If you use the unit attributes, how do you configure units that do not yet
> exist?

For example by adding the enable_lun writeable sysfs entry. I think both ways are
viable and there are several pitfalls in each of them. Now it's up to Evan to decide
how to implement this.

> 
> Perhaps it is better to represent the configuration descriptors exactly as
> they are defined in the specification.  Probably not worth exposing them at
> all if the configuration is locked (attribute bConfigDescrLock == 1).
> 
> Note also that the 2.1 spec. defines  bConfDescContinue which allows updates
> to be grouped and committed together.

The only question is how many devices are ready to get dozens of configuration
descriptors related to first eight LUNs instead just one when this lock is enabled.

Regards
Stanislav

Re: [PATCH 0/7] Enable UFS provisioning via Linux

[Evan Green]

On Sun, Jun 10, 2018 at 2:31 AM Stanislav Nijnikov
<Stanislav.Nijnikov@wdc.com> wrote:
>
> Hi Adrian,
>
> > -----Original Message-----
> > From: Adrian Hunter <adrian.hunter@intel.com>
> > Sent: Friday, June 8, 2018 3:31 PM
> > To: Evan Green <evgreen@chromium.org>; Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> > Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> > kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> > <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> > Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> >
> > On 04/06/18 17:59, Evan Green wrote:
> > > On Sun, Jun 3, 2018 at 3:21 AM Stanislav Nijnikov
> > > <Stanislav.Nijnikov@wdc.com> wrote:
> > >>
> > >>
> > >>
> > >>> -----Original Message-----
> > >>> From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> On Behalf Of Evan Green
> > >>> Sent: Friday, June 1, 2018 5:44 PM
> > >>> To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> > >>> Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> > >>> kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> > >>> <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> > >>> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> > >>>
> > >>> Hi Stanislav. Thanks for taking a look. Responses below.
> > >>>
> > >>> On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
> > >>> <Stanislav.Nijnikov@wdc.com> wrote:
> > >>>>
> > >>>> Hi Evan,
> > >>>> I have some generic notes:
> > >>>> - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and unit
> > >>> descriptors? And the sysfs representation of the device and unit descriptors is existing already.
> > >>>
> > >>> Well, UFS describes them as different descriptors. I worry that if I
> > >>> add a bunch of clever logic to hide the config descriptor behind other
> > >>> descriptors, there might be trouble later if 1) there is a quirky
> > >>> device that doesn't reflect the values between descriptors quite the
> > >>> same way or at the same time, or 2) if a later UFS spec adds more
> > >>> configuration descriptor fields that don't exactly reflect into other
> > >>> non-config descriptors, the cleverness will look awkward.
> > >>
> > >> No additional logic will be required to attach write functionality to the
> > >> existing entries instead of new defined ones. It will reduce the patch
> > >> size significantly. And there will be no need for the unit selector
> > >> mechanism which I'm not sure will be accepted by the SCSI community.
> > >>
> > >
> > > So this would be modifying the existing sysfs entries so that reads
> > > still come from the device and unit descriptors, but writes go to
> > > equivalent fields in the config descriptor? I can explore that
> > > approach. Alternatively, if the unit selector mechanism is not
> > > desired, I could dynamically create sysfs directories for each unit in
> > > the config descriptor, but still bring out the config descriptor
> > > values as separate entries. (I still worry a bit about smashing the
> > > descriptors together as the UFS spec called them out as different).
> >
> > If you use the unit attributes, how do you configure units that do not yet
> > exist?
>
> For example by adding the enable_lun writeable sysfs entry. I think both ways are
> viable and there are several pitfalls in each of them. Now it's up to Evan to decide
> how to implement this.
>
> >
> > Perhaps it is better to represent the configuration descriptors exactly as
> > they are defined in the specification.  Probably not worth exposing them at
> > all if the configuration is locked (attribute bConfigDescrLock == 1).
> >
> > Note also that the 2.1 spec. defines  bConfDescContinue which allows updates
> > to be grouped and committed together.
>
> The only question is how many devices are ready to get dozens of configuration
> descriptors related to first eight LUNs instead just one when this lock is enabled.
>
> Regards
> Stanislav

Actually I could use some advice on this. It seems like folks are
opposed to the idea of having a cfg_unit file, whose value determines
which index to talk to in the unit_* files. (I personally liked that
approach, as it was simple, has precedence, and fit the requirements,
but oh well). My instinct favors Adrian's approach of keeping the
configuration descriptor separate, rather than hiding it behind the
device and unit descriptors, as I think it's more true to the UFS spec
and less likely to cause problems in the future. However I'm trying to
figure out the best way to do that.

What I _want_ to do is basically create N sysfs groups, where each
group points to the same array of attributes. Then in the show/store
methods, look up which group I'm in and use that as an index. But the
show/store functions only pass the attributes themselves, and there
seems to be no way for me to get the parent node. So my next plan is
to create a wrapper around struct device_attribute where I can store
my index, create a template of attributes, and then create N copies of
this template. The show/store method is then a single method, which
uses container_of on the attribute to get the index, offset, and size
of the descriptor to change. This seems less than ideal to me, as it's
never fun to feel like you're wasting memory, even though it's
probably on the order of a kilobyte or two.

Stanislav, you've got the unit descriptors off in the scsi_device,
which would make a lot of sense for me too, except that I need to
configure luns that may not exist yet. Can you expand on your
enable_lun idea?
-Evan

Re: [PATCH 0/7] Enable UFS provisioning via Linux

[Bart Van Assche]

On Tue, 2018-06-12 at 12:42 -0700, Evan Green wrote:
> What I _want_ to do is basically create N sysfs groups, where each
> group points to the same array of attributes. Then in the show/store
> methods, look up which group I'm in and use that as an index. But the
> show/store functions only pass the attributes themselves, and there
> seems to be no way for me to get the parent node.

The first argument that is passed to sysfs show and store methods is
a kobject pointer. Have you considered to access kobject.parent from
inside the show/store methods? From fs/sysfs/file.c:

	if (ops->show) {
		count = ops->show(kobj, of->kn->priv, buf);
		if (count < 0)
			return count;
	}

Bart.

RE: [PATCH 0/7] Enable UFS provisioning via Linux

[Stanislav Nijnikov]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> On Behalf Of Evan Green
> Sent: Tuesday, June 12, 2018 10:43 PM
> To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> Cc: adrian.hunter@intel.com; Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com;
> linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> 
> On Sun, Jun 10, 2018 at 2:31 AM Stanislav Nijnikov
> <Stanislav.Nijnikov@wdc.com> wrote:
> >
> > Hi Adrian,
> >
> > > -----Original Message-----
> > > From: Adrian Hunter <adrian.hunter@intel.com>
> > > Sent: Friday, June 8, 2018 3:31 PM
> > > To: Evan Green <evgreen@chromium.org>; Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> > > Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> > > kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> > > <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> > > Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> > >
> > > On 04/06/18 17:59, Evan Green wrote:
> > > > On Sun, Jun 3, 2018 at 3:21 AM Stanislav Nijnikov
> > > > <Stanislav.Nijnikov@wdc.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >>> -----Original Message-----
> > > >>> From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> On Behalf Of Evan Green
> > > >>> Sent: Friday, June 1, 2018 5:44 PM
> > > >>> To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> > > >>> Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> > > >>> kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> > > >>> <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> > > >>> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> > > >>>
> > > >>> Hi Stanislav. Thanks for taking a look. Responses below.
> > > >>>
> > > >>> On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
> > > >>> <Stanislav.Nijnikov@wdc.com> wrote:
> > > >>>>
> > > >>>> Hi Evan,
> > > >>>> I have some generic notes:
> > > >>>> - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and
> unit
> > > >>> descriptors? And the sysfs representation of the device and unit descriptors is existing already.
> > > >>>
> > > >>> Well, UFS describes them as different descriptors. I worry that if I
> > > >>> add a bunch of clever logic to hide the config descriptor behind other
> > > >>> descriptors, there might be trouble later if 1) there is a quirky
> > > >>> device that doesn't reflect the values between descriptors quite the
> > > >>> same way or at the same time, or 2) if a later UFS spec adds more
> > > >>> configuration descriptor fields that don't exactly reflect into other
> > > >>> non-config descriptors, the cleverness will look awkward.
> > > >>
> > > >> No additional logic will be required to attach write functionality to the
> > > >> existing entries instead of new defined ones. It will reduce the patch
> > > >> size significantly. And there will be no need for the unit selector
> > > >> mechanism which I'm not sure will be accepted by the SCSI community.
> > > >>
> > > >
> > > > So this would be modifying the existing sysfs entries so that reads
> > > > still come from the device and unit descriptors, but writes go to
> > > > equivalent fields in the config descriptor? I can explore that
> > > > approach. Alternatively, if the unit selector mechanism is not
> > > > desired, I could dynamically create sysfs directories for each unit in
> > > > the config descriptor, but still bring out the config descriptor
> > > > values as separate entries. (I still worry a bit about smashing the
> > > > descriptors together as the UFS spec called them out as different).
> > >
> > > If you use the unit attributes, how do you configure units that do not yet
> > > exist?
> >
> > For example by adding the enable_lun writeable sysfs entry. I think both ways are
> > viable and there are several pitfalls in each of them. Now it's up to Evan to decide
> > how to implement this.
> >
> > >
> > > Perhaps it is better to represent the configuration descriptors exactly as
> > > they are defined in the specification.  Probably not worth exposing them at
> > > all if the configuration is locked (attribute bConfigDescrLock == 1).
> > >
> > > Note also that the 2.1 spec. defines  bConfDescContinue which allows updates
> > > to be grouped and committed together.
> >
> > The only question is how many devices are ready to get dozens of configuration
> > descriptors related to first eight LUNs instead just one when this lock is enabled.
> >
> > Regards
> > Stanislav
> 
> Actually I could use some advice on this. It seems like folks are
> opposed to the idea of having a cfg_unit file, whose value determines
> which index to talk to in the unit_* files. (I personally liked that
> approach, as it was simple, has precedence, and fit the requirements,
> but oh well). My instinct favors Adrian's approach of keeping the
> configuration descriptor separate, rather than hiding it behind the
> device and unit descriptors, as I think it's more true to the UFS spec
> and less likely to cause problems in the future. However I'm trying to
> figure out the best way to do that.
> 
> What I _want_ to do is basically create N sysfs groups, where each
> group points to the same array of attributes. Then in the show/store
> methods, look up which group I'm in and use that as an index. But the
> show/store functions only pass the attributes themselves, and there
> seems to be no way for me to get the parent node. So my next plan is
> to create a wrapper around struct device_attribute where I can store
> my index, create a template of attributes, and then create N copies of
> this template. The show/store method is then a single method, which
> uses container_of on the attribute to get the index, offset, and size
> of the descriptor to change. This seems less than ideal to me, as it's
> never fun to feel like you're wasting memory, even though it's
> probably on the order of a kilobyte or two.
> 
> Stanislav, you've got the unit descriptors off in the scsi_device,
> which would make a lot of sense for me too, except that I need to
> configure luns that may not exist yet. Can you expand on your
> enable_lun idea?
> -Evan

It's a writeable sysfs entry that receive an integer value (as an index 
of a lun that should be enable). The store function does some sanity
checks, reads the configuration descriptor, update the specified
lun enable parameter and sends it. After restart the lun will be available
and ready for further configuration.

Stanislav
 

Re: [PATCH 0/7] Enable UFS provisioning via Linux

[Evan Green]

On Wed, Jun 13, 2018 at 3:12 AM Stanislav Nijnikov
<Stanislav.Nijnikov@wdc.com> wrote:
>
>
>
> > -----Original Message-----
> > From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> On Behalf Of Evan Green
> > Sent: Tuesday, June 12, 2018 10:43 PM
> > To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> > Cc: adrian.hunter@intel.com; Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com;
> > linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> > <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> > Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> >
> > On Sun, Jun 10, 2018 at 2:31 AM Stanislav Nijnikov
> > <Stanislav.Nijnikov@wdc.com> wrote:
> > >
> > > Hi Adrian,
> > >
> > > > -----Original Message-----
> > > > From: Adrian Hunter <adrian.hunter@intel.com>
> > > > Sent: Friday, June 8, 2018 3:31 PM
> > > > To: Evan Green <evgreen@chromium.org>; Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> > > > Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> > > > kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> > > > <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> > > > Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> > > >
> > > > On 04/06/18 17:59, Evan Green wrote:
> > > > > On Sun, Jun 3, 2018 at 3:21 AM Stanislav Nijnikov
> > > > > <Stanislav.Nijnikov@wdc.com> wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >>> -----Original Message-----
> > > > >>> From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org> On Behalf Of Evan Green
> > > > >>> Sent: Friday, June 1, 2018 5:44 PM
> > > > >>> To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
> > > > >>> Cc: Vinayak Holikatti <vinholikatti@gmail.com>; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; linux-
> > > > >>> kernel@vger.kernel.org; linux-scsi@vger.kernel.org; Gwendal Grignou <gwendal@chromium.org>; Alex Lemberg
> > > > >>> <Alex.Lemberg@wdc.com>; Avri Altman <Avri.Altman@wdc.com>
> > > > >>> Subject: Re: [PATCH 0/7] Enable UFS provisioning via Linux
> > > > >>>
> > > > >>> Hi Stanislav. Thanks for taking a look. Responses below.
> > > > >>>
> > > > >>> On Thu, May 31, 2018 at 3:04 AM Stanislav Nijnikov
> > > > >>> <Stanislav.Nijnikov@wdc.com> wrote:
> > > > >>>>
> > > > >>>> Hi Evan,
> > > > >>>> I have some generic notes:
> > > > >>>> - Why to create new sysfs entries for the configuration descriptor fields if they are just duplication of fields in the device and
> > unit
> > > > >>> descriptors? And the sysfs representation of the device and unit descriptors is existing already.
> > > > >>>
> > > > >>> Well, UFS describes them as different descriptors. I worry that if I
> > > > >>> add a bunch of clever logic to hide the config descriptor behind other
> > > > >>> descriptors, there might be trouble later if 1) there is a quirky
> > > > >>> device that doesn't reflect the values between descriptors quite the
> > > > >>> same way or at the same time, or 2) if a later UFS spec adds more
> > > > >>> configuration descriptor fields that don't exactly reflect into other
> > > > >>> non-config descriptors, the cleverness will look awkward.
> > > > >>
> > > > >> No additional logic will be required to attach write functionality to the
> > > > >> existing entries instead of new defined ones. It will reduce the patch
> > > > >> size significantly. And there will be no need for the unit selector
> > > > >> mechanism which I'm not sure will be accepted by the SCSI community.
> > > > >>
> > > > >
> > > > > So this would be modifying the existing sysfs entries so that reads
> > > > > still come from the device and unit descriptors, but writes go to
> > > > > equivalent fields in the config descriptor? I can explore that
> > > > > approach. Alternatively, if the unit selector mechanism is not
> > > > > desired, I could dynamically create sysfs directories for each unit in
> > > > > the config descriptor, but still bring out the config descriptor
> > > > > values as separate entries. (I still worry a bit about smashing the
> > > > > descriptors together as the UFS spec called them out as different).
> > > >
> > > > If you use the unit attributes, how do you configure units that do not yet
> > > > exist?
> > >
> > > For example by adding the enable_lun writeable sysfs entry. I think both ways are
> > > viable and there are several pitfalls in each of them. Now it's up to Evan to decide
> > > how to implement this.
> > >
> > > >
> > > > Perhaps it is better to represent the configuration descriptors exactly as
> > > > they are defined in the specification.  Probably not worth exposing them at
> > > > all if the configuration is locked (attribute bConfigDescrLock == 1).
> > > >
> > > > Note also that the 2.1 spec. defines  bConfDescContinue which allows updates
> > > > to be grouped and committed together.
> > >
> > > The only question is how many devices are ready to get dozens of configuration
> > > descriptors related to first eight LUNs instead just one when this lock is enabled.
> > >
> > > Regards
> > > Stanislav
> >
> > Actually I could use some advice on this. It seems like folks are
> > opposed to the idea of having a cfg_unit file, whose value determines
> > which index to talk to in the unit_* files. (I personally liked that
> > approach, as it was simple, has precedence, and fit the requirements,
> > but oh well). My instinct favors Adrian's approach of keeping the
> > configuration descriptor separate, rather than hiding it behind the
> > device and unit descriptors, as I think it's more true to the UFS spec
> > and less likely to cause problems in the future. However I'm trying to
> > figure out the best way to do that.
> >
> > What I _want_ to do is basically create N sysfs groups, where each
> > group points to the same array of attributes. Then in the show/store
> > methods, look up which group I'm in and use that as an index. But the
> > show/store functions only pass the attributes themselves, and there
> > seems to be no way for me to get the parent node. So my next plan is
> > to create a wrapper around struct device_attribute where I can store
> > my index, create a template of attributes, and then create N copies of
> > this template. The show/store method is then a single method, which
> > uses container_of on the attribute to get the index, offset, and size
> > of the descriptor to change. This seems less than ideal to me, as it's
> > never fun to feel like you're wasting memory, even though it's
> > probably on the order of a kilobyte or two.
> >
> > Stanislav, you've got the unit descriptors off in the scsi_device,
> > which would make a lot of sense for me too, except that I need to
> > configure luns that may not exist yet. Can you expand on your
> > enable_lun idea?
> > -Evan
>
> It's a writeable sysfs entry that receive an integer value (as an index
> of a lun that should be enable). The store function does some sanity
> checks, reads the configuration descriptor, update the specified
> lun enable parameter and sends it. After restart the lun will be available
> and ready for further configuration.
>
> Stanislav
>
>

Thanks Bart for the advice. I took that and ran with it a little in
the next series, which I've just sent out.
Stanislav, thanks for elaborating. For the next spin of this series I
went with keeping the config descriptor separate, but if everyone
prefers putting the config descriptor behind the device/unit
descriptors, I will keep this solution in mind.
-Evan

Re: [PATCH 0/7] Enable UFS provisioning via Linux

[Kyuho Choi]

Hi Evan,

I've some question about end user's provisioning.

On 5/30/18, Evan Green <evgreen@chromium.org> wrote:
> This series enables provisioning UFS devices using the existing sysfs
> interface. This functionality is primarily useful along the assembly
> line, but might also be useful for end users that receive devices that
> aren't locked down.

As we knew, ufs are mainly adopted to boot device like mobile phone.
Usally after  provisioning operation, every data in ufs (eg. Android :
system, userdata, boot info and etc) would be removed. How end user
download and flash all data to ufs?. dd or another download tools?.

>
> Evan Green (7):
>   scsi: ufs: Add Configuration Descriptor to sysfs
>   scsi: ufs: Add config descriptor documentation
>   scsi: ufs: Make sysfs attributes writable
>   scsi: ufs: sysfs: Document attribute writability
>   scsi: ufs: Refactor descriptor read for write
>   scsi: ufs: Enable writing config descriptor
>   scsi: ufs: Update config descriptor documentation
>
>  Documentation/ABI/testing/sysfs-driver-ufs | 174 ++++++++++++++++++++---
>  drivers/scsi/ufs/ufs-sysfs.c               | 217
> ++++++++++++++++++++++++++---
>  drivers/scsi/ufs/ufs.h                     |  29 ++++
>  drivers/scsi/ufs/ufshcd.c                  |  89 ++++++++----
>  drivers/scsi/ufs/ufshcd.h                  |  16 ++-
>  5 files changed, 458 insertions(+), 67 deletions(-)
>
> --
> 2.13.5
>
>

Re: [PATCH 0/7] Enable UFS provisioning via Linux

[Evan Green]

Hi Kyuho,

On Mon, Jun 4, 2018 at 4:11 AM Kyuho Choi <chlrbgh0@gmail.com> wrote:
>
> Hi Evan,
>
> I've some question about end user's provisioning.
>
> On 5/30/18, Evan Green <evgreen@chromium.org> wrote:
> > This series enables provisioning UFS devices using the existing sysfs
> > interface. This functionality is primarily useful along the assembly
> > line, but might also be useful for end users that receive devices that
> > aren't locked down.
>
> As we knew, ufs are mainly adopted to boot device like mobile phone.
> Usally after  provisioning operation, every data in ufs (eg. Android :
> system, userdata, boot info and etc) would be removed. How end user
> download and flash all data to ufs?. dd or another download tools?.
>

You're right, the user would need to be booted into a RAM disk, or off
of removable media like micro SD, and use dd or custom tools to set up
data inside the new provisioning scheme. So yes, on a mobile phone, it
would be a fairly advanced end user.
-Evan