[PATCH V5 1/2] scsi: ufs: set the device reference clock setting

[PATCH V5 1/2] scsi: ufs: set the device reference clock setting

[Sayali Lokhande]

From: Subhash Jadavani <subhashj@codeaurora.org>

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

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

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

Re: [PATCH V5 1/2] scsi: ufs: set the device reference clock setting

[Rob Herring]

On Fri, Jul 06, 2018 at 11:50:39AM +0530, Sayali Lokhande wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> UFS host supplies the reference clock to UFS device and UFS device
> specification allows host to provide one of the 4 frequencies (19.2 MHz,
> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the
> device reference clock frequency setting in the device based on what
> frequency it is supplying to UFS device.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      |  7 +++
>  drivers/scsi/ufs/ufs.h                             |  9 ++++
>  drivers/scsi/ufs/ufshcd-pltfrm.c                   |  2 +
>  drivers/scsi/ufs/ufshcd.c                          | 62 ++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h                          |  2 +
>  5 files changed, 82 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index c39dfef..bc20f59 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -41,6 +41,12 @@ Optional properties:
>  -lanes-per-direction	: number of lanes available per direction - either 1 or 2.
>  			  Note that it is assume same number of lanes is used both
>  			  directions at once. If not specified, default is 2 lanes per direction.
> +- assigned-clock-rates 	: Specify the device reference clock frequency, must be one of the following:
> +			  0: 19.2 MHz
> +			  1: 26 MHz
> +			  2: 38.4 MHz
> +			  3: 52 MHz
> +			  Defaults to 26 MHz if not specified.

Sigh. You obviously did not read the definition of assigned-clock-rates 
and understand how it works. I don't think you want the frequency to be 
0-3 Hz.


>  Note: If above properties are not defined it can be assumed that the supply
>  regulators or clocks are always on.
> @@ -66,4 +72,5 @@ Example:
>  		freq-table-hz = <100000000 200000000>, <0 0>, <0 0>;
>  		phys = <&ufsphy1>;
>  		phy-names = "ufsphy";
> +		assigned-clock-rates = <0>; /* reference clock freq: 19.2 MHz */
>  	};
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 14e5bf7..e15deb0 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -378,6 +378,15 @@ enum query_opcode {
>  	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
>  };
>  
> +/* bRefClkFreq attribute values */
> +enum ref_clk_freq {
> +	REF_CLK_FREQ_19_2_MHZ	= 0x0,
> +	REF_CLK_FREQ_26_MHZ	= 0x1,
> +	REF_CLK_FREQ_38_4_MHZ	= 0x2,
> +	REF_CLK_FREQ_52_MHZ	= 0x3,
> +	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
> +};
> +
>  /* Query response result code */
>  enum {
>  	QUERY_RESULT_SUCCESS                    = 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index e82bde0..0953563 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -343,6 +343,8 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	ufshcd_parse_dev_ref_clk_freq(hba);
> +
>  	ufshcd_init_lanes_per_dir(hba);
>  
>  	err = ufshcd_init(hba, mmio_base, irq);
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c5b1bf1..455a6ad 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6296,6 +6296,62 @@ static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
>  	hba->desc_size.hlth_desc = QUERY_DESC_HEALTH_DEF_SIZE;
>  }
>  
> +void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba)
> +{
> +	struct device *dev = hba->dev;
> +	int ret;
> +
> +	if (!dev)
> +		return;
> +
> +	ret = device_property_read_u32(dev, "assigned-clock-rates",
> +				   &hba->dev_ref_clk_freq);
> +	if (ret ||
> +	    (hba->dev_ref_clk_freq > REF_CLK_FREQ_52_MHZ)) {
> +		dev_err(hba->dev,
> +		"%s: invalid ref_clk setting = %d\n",
> +		__func__, hba->dev_ref_clk_freq);
> +		hba->dev_ref_clk_freq = REF_CLK_FREQ_MAX + 1;
> +	}
> +}
> +
> +static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
> +{
> +	int err = 0;
> +	int ref_clk = -1;
> +	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
> +						     "38.4 MHz", "52 MHz"};
> +
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
> +
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
> +			 __func__, err);
> +		goto out;
> +	}
> +
> +	if (ref_clk == hba->dev_ref_clk_freq)
> +		goto out; /* nothing to update */
> +
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
> +			&hba->dev_ref_clk_freq);
> +
> +	if (err)
> +		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> +	/*
> +	 * It is good to print this out here to debug any later failures
> +	 * related to gear switch.
> +	 */
> +	dev_dbg(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> +

I still don't understand how you think this is supposed to work. All 
this does is copy whatever freq setting you put into DT to this 
device attr. That does nothing to actually set the frequency of the 
ref_clk. It's frequency could be anything before and after this 
function.

Also, according to the spec, isn't QUERY_ATTR_IDN_REF_CLK_FREQ 
write-once? So if you can read the value, wouldn't that mean you can't 
set it.

Rob

Re: [PATCH V5 1/2] scsi: ufs: set the device reference clock setting

[Sayali Lokhande]

Hi Rob,


On 7/7/2018 2:37 AM, Rob Herring wrote:
> On Fri, Jul 06, 2018 at 11:50:39AM +0530, Sayali Lokhande wrote:
>> From: Subhash Jadavani <subhashj@codeaurora.org>
>>
>> UFS host supplies the reference clock to UFS device and UFS device
>> specification allows host to provide one of the 4 frequencies (19.2 MHz,
>> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the
>> device reference clock frequency setting in the device based on what
>> frequency it is supplying to UFS device.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
>> ---
>>   .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      |  7 +++
>>   drivers/scsi/ufs/ufs.h                             |  9 ++++
>>   drivers/scsi/ufs/ufshcd-pltfrm.c                   |  2 +
>>   drivers/scsi/ufs/ufshcd.c                          | 62 ++++++++++++++++++++++
>>   drivers/scsi/ufs/ufshcd.h                          |  2 +
>>   5 files changed, 82 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> index c39dfef..bc20f59 100644
>> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> @@ -41,6 +41,12 @@ Optional properties:
>>   -lanes-per-direction	: number of lanes available per direction - either 1 or 2.
>>   			  Note that it is assume same number of lanes is used both
>>   			  directions at once. If not specified, default is 2 lanes per direction.
>> +- assigned-clock-rates 	: Specify the device reference clock frequency, must be one of the following:
>> +			  0: 19.2 MHz
>> +			  1: 26 MHz
>> +			  2: 38.4 MHz
>> +			  3: 52 MHz
>> +			  Defaults to 26 MHz if not specified.
> Sigh. You obviously did not read the definition of assigned-clock-rates
> and understand how it works. I don't think you want the frequency to be
> 0-3 Hz.
>
Let me revisit this patch set w.r.t to use of assigned-clock-rates.
I will be separating this patch from provisioning patches and will 
upload its next version separately.
>>   Note: If above properties are not defined it can be assumed that the supply
>>   regulators or clocks are always on.
>> @@ -66,4 +72,5 @@ Example:
>>   		freq-table-hz = <100000000 200000000>, <0 0>, <0 0>;
>>   		phys = <&ufsphy1>;
>>   		phy-names = "ufsphy";
>> +		assigned-clock-rates = <0>; /* reference clock freq: 19.2 MHz */
>>   	};
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index 14e5bf7..e15deb0 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -378,6 +378,15 @@ enum query_opcode {
>>   	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
>>   };
>>   
>> +/* bRefClkFreq attribute values */
>> +enum ref_clk_freq {
>> +	REF_CLK_FREQ_19_2_MHZ	= 0x0,
>> +	REF_CLK_FREQ_26_MHZ	= 0x1,
>> +	REF_CLK_FREQ_38_4_MHZ	= 0x2,
>> +	REF_CLK_FREQ_52_MHZ	= 0x3,
>> +	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
>> +};
>> +
>>   /* Query response result code */
>>   enum {
>>   	QUERY_RESULT_SUCCESS                    = 0x00,
>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> index e82bde0..0953563 100644
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> @@ -343,6 +343,8 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
>>   	pm_runtime_set_active(&pdev->dev);
>>   	pm_runtime_enable(&pdev->dev);
>>   
>> +	ufshcd_parse_dev_ref_clk_freq(hba);
>> +
>>   	ufshcd_init_lanes_per_dir(hba);
>>   
>>   	err = ufshcd_init(hba, mmio_base, irq);
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index c5b1bf1..455a6ad 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -6296,6 +6296,62 @@ static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
>>   	hba->desc_size.hlth_desc = QUERY_DESC_HEALTH_DEF_SIZE;
>>   }
>>   
>> +void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba)
>> +{
>> +	struct device *dev = hba->dev;
>> +	int ret;
>> +
>> +	if (!dev)
>> +		return;
>> +
>> +	ret = device_property_read_u32(dev, "assigned-clock-rates",
>> +				   &hba->dev_ref_clk_freq);
>> +	if (ret ||
>> +	    (hba->dev_ref_clk_freq > REF_CLK_FREQ_52_MHZ)) {
>> +		dev_err(hba->dev,
>> +		"%s: invalid ref_clk setting = %d\n",
>> +		__func__, hba->dev_ref_clk_freq);
>> +		hba->dev_ref_clk_freq = REF_CLK_FREQ_MAX + 1;
>> +	}
>> +}
>> +
>> +static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
>> +{
>> +	int err = 0;
>> +	int ref_clk = -1;
>> +	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
>> +						     "38.4 MHz", "52 MHz"};
>> +
>> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
>> +
>> +	if (err) {
>> +		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
>> +			 __func__, err);
>> +		goto out;
>> +	}
>> +
>> +	if (ref_clk == hba->dev_ref_clk_freq)
>> +		goto out; /* nothing to update */
>> +
>> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
>> +			&hba->dev_ref_clk_freq);
>> +
>> +	if (err)
>> +		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
>> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
>> +	/*
>> +	 * It is good to print this out here to debug any later failures
>> +	 * related to gear switch.
>> +	 */
>> +	dev_dbg(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
>> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
>> +
> I still don't understand how you think this is supposed to work. All
> this does is copy whatever freq setting you put into DT to this
> device attr. That does nothing to actually set the frequency of the
> ref_clk. It's frequency could be anything before and after this
> function.
Here, we are trying to check and then set parsed ref clock freq setting 
from DT.
Function ufshcd_query_attr_retry() is being used to first read the 
current freq and then write ref clk freq attr (if DT setting is 
different than what is already set in device).
> Also, according to the spec, isn't QUERY_ATTR_IDN_REF_CLK_FREQ
> write-once? So if you can read the value, wouldn't that mean you can't
> set it.
As per specs, bRefClkFreq field had “Write once” Access Property up to 
UFS 2.0.
 From 2.1 onwards,  bRefClkFreq access property changed from “Write 
once” to “Persistent”(can be written multiple times, the value is kept 
after power cycle or any type reset event).

> Rob

[PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Sayali Lokhande]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 9790 bytes --]

This patch adds configfs support to provision ufs device at
runtime. This feature can be primarily useful in factory or
assembly line as some devices may be required to be configured
multiple times during initial system development phase.
Configuration Descriptors can be written multiple times until
bConfigDescrLock attribute is zero.

Configuration descriptor buffer consists of Device and Unit
descriptor configurable parameters which are parsed from vendor
specific provisioning file and then passed via configfs node at
runtime to provision ufs device.
CONFIG_CONFIGFS_FS needs to be enabled for using this feature.

Usage:
1) To read current configuration descriptor :
   cat /config/ufshcd/ufs_provision
2) To provision ufs device:
   echo <config_desc_buf> > /config/ufshcd/ufs_provision

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

diff --git a/Documentation/ABI/testing/configfs-driver-ufs b/Documentation/ABI/testing/configfs-driver-ufs
new file mode 100644
index 0000000..dd16842
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-driver-ufs
@@ -0,0 +1,18 @@
+What:		/config/ufshcd/ufs_provision
+Date:		Jun 2018
+KernelVersion:	4.14
+Description:
+		This file shows the current ufs configuration descriptor set in device.
+		This can be used to provision ufs device if bConfigDescrLock is 0.
+		For more details, refer 14.1.6.3 Configuration Descriptor and
+		Table 14-12 — Unit Descriptor configurable parameters from specs for
+		description of each configuration descriptor parameter.
+		Configuration descriptor buffer needs to be passed in space separated
+		format specificied as below:
+		echo <bLength> <bDescriptorIDN> <bConfDescContinue> <bBootEnable>
+		<bDescrAccessEn> <bInitPowerMode> <bHighPriorityLUN>
+		<bSecureRemovalType> <bInitActiveICCLevel> <wPeriodicRTCUpdate>
+		<0Bh:0Fh_ReservedAs_0> <bLUEnable> <bBootLunID> <bLUWriteProtect>
+		<bMemoryType> <dNumAllocUnits> <bDataReliability> <bLogicalBlockSize>
+		<bProvisioningType> <wContextCapabilities> <0Dh:0Fh_ReservedAs_0>
+		> /config/ufshcd/ufs_provision
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 918f579..d438e74 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -5,5 +5,6 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-d
 obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
 obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
 ufshcd-core-objs := ufshcd.o ufs-sysfs.o
+obj-$(CONFIG_CONFIGFS_FS) += ufs-configfs.o
 obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
 obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
diff --git a/drivers/scsi/ufs/ufs-configfs.c b/drivers/scsi/ufs/ufs-configfs.c
new file mode 100644
index 0000000..7d207fc
--- /dev/null
+++ b/drivers/scsi/ufs/ufs-configfs.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (c) 2018, Linux Foundation.
+
+#include <linux/configfs.h>
+#include <linux/err.h>
+#include <linux/string.h>
+
+#include "ufs.h"
+#include "ufshcd.h"
+
+static inline struct ufs_hba *config_item_to_hba(struct config_item *item)
+{
+	struct config_group *group = to_config_group(item);
+	struct configfs_subsystem *subsys = to_configfs_subsystem(group);
+	struct ufs_hba *hba = container_of(subsys, struct ufs_hba, subsys);
+
+	return hba ? hba : NULL;
+}
+
+static ssize_t ufs_provision_show(struct config_item *item, char *buf)
+{
+	u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
+	int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
+	int i, ret, curr_len = 0;
+	struct ufs_hba *hba = config_item_to_hba(item);
+
+	if (!hba)
+		return snprintf(buf, PAGE_SIZE, "ufs hba is NULL!\n");
+
+	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
+		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
+
+	if (ret)
+		return snprintf(buf, PAGE_SIZE,
+		"Failed reading descriptor. desc_idn %d, opcode %x ret %d\n",
+		QUERY_DESC_IDN_CONFIGURATION,
+		UPIU_QUERY_OPCODE_READ_DESC, ret);
+
+	for (i = 0; i < buff_len; i++)
+		curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
+				"0x%x ", desc_buf[i]);
+
+	return curr_len;
+}
+
+ssize_t ufshcd_desc_configfs_store(struct ufs_hba *hba,
+		const char *buf, size_t count)
+{
+	char *strbuf;
+	char *strbuf_copy;
+	u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
+	int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
+	char *token;
+	int i, ret, value;
+	u32 bConfigDescrLock = 0;
+
+	/* reserve one byte for null termination */
+	strbuf = kmalloc(count + 1, GFP_KERNEL);
+	if (!strbuf)
+		return -ENOMEM;
+
+	strbuf_copy = strbuf;
+	strlcpy(strbuf, buf, count + 1);
+
+	if (!hba)
+		goto out;
+
+	/* Just return if bConfigDescrLock is already set */
+	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &bConfigDescrLock);
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed reading bConfigDescrLock %d, cannot re-provision device!\n",
+			__func__, ret);
+		goto out;
+	}
+	if (bConfigDescrLock) {
+		dev_err(hba->dev, "%s: bConfigDescrLock already set to %u, cannot re-provision device!\n",
+		__func__, bConfigDescrLock);
+		goto out;
+	}
+
+	for (i = 0; i < QUERY_DESC_CONFIGURATION_DEF_SIZE; i++) {
+		token = strsep(&strbuf, " ");
+		if (!token && i) {
+			dev_dbg(hba->dev, "%s: token %s, i %d\n",
+				__func__, token, i);
+			break;
+		}
+
+		ret = kstrtoint(token, 0, &value);
+		if (ret) {
+			dev_err(hba->dev, "%s: kstrtoint failed %d %s\n",
+				__func__, ret, token);
+			break;
+		}
+		desc_buf[i] = (u8)value;
+		dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);
+	}
+
+	/* Write configuration descriptor to provision ufs */
+	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
+		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
+
+	if (ret)
+		dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x ret %d\n",
+		__func__, QUERY_DESC_IDN_CONFIGURATION,
+		UPIU_QUERY_OPCODE_WRITE_DESC, ret);
+	else
+		dev_err(hba->dev, "%s: UFS Provisioning done, reboot now!\n",
+		__func__);
+
+out:
+	kfree(strbuf_copy);
+	return count;
+}
+
+static ssize_t ufs_provision_store(struct config_item *item,
+		const char *buf, size_t count)
+{
+	struct ufs_hba *hba = config_item_to_hba(item);
+
+	return ufshcd_desc_configfs_store(hba, buf, count);
+}
+
+static struct configfs_attribute ufshcd_attr_provision = {
+	.ca_name	= "ufs_provision",
+	.ca_mode	= 0666,
+	.ca_owner	= THIS_MODULE,
+	.show		= ufs_provision_show,
+	.store		= ufs_provision_store,
+};
+
+static struct configfs_attribute *ufshcd_attrs[] = {
+	&ufshcd_attr_provision,
+	NULL,
+};
+
+static struct config_item_type ufscfg_type = {
+	.ct_attrs	= ufshcd_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct configfs_subsystem ufscfg_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "ufshcd",
+			.ci_type = &ufscfg_type,
+		},
+	},
+};
+
+int ufshcd_configfs_init(struct ufs_hba *hba)
+{
+	int ret;
+	struct configfs_subsystem *subsys = &hba->subsys;
+
+	subsys->su_group = ufscfg_subsys.su_group;
+	config_group_init(&subsys->su_group);
+	mutex_init(&subsys->su_mutex);
+	ret = configfs_register_subsystem(subsys);
+	if (ret)
+		pr_err("Error %d while registering subsystem %s\n",
+		       ret,
+		       subsys->su_group.cg_item.ci_namebuf);
+
+	return ret;
+}
+
+void ufshcd_configfs_exit(void)
+{
+	configfs_unregister_subsystem(&ufscfg_subsys);
+}
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 455a6ad..fb97a42 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7684,6 +7684,7 @@ int ufshcd_shutdown(struct ufs_hba *hba)
 void ufshcd_remove(struct ufs_hba *hba)
 {
 	ufs_sysfs_remove_nodes(hba->dev);
+	ufshcd_configfs_exit();
 	scsi_remove_host(hba->host);
 	/* disable interrupts */
 	ufshcd_disable_intr(hba, hba->intr_mask);
@@ -7937,6 +7938,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	async_schedule(ufshcd_async_scan, hba);
 	ufs_sysfs_add_nodes(hba->dev);
+	ufshcd_configfs_init(hba);
 
 	return 0;
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 101a75c..e9081de 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -37,6 +37,7 @@
 #ifndef _UFSHCD_H
 #define _UFSHCD_H
 
+#include <linux/configfs.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -515,6 +516,9 @@ struct ufs_hba {
 
 	struct Scsi_Host *host;
 	struct device *dev;
+#ifdef CONFIG_CONFIGFS_FS
+	struct configfs_subsystem subsys;
+#endif
 	/*
 	 * This field is to keep a reference to "scsi_device" corresponding to
 	 * "UFS device" W-LU.
@@ -868,6 +872,21 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index,
 int ufshcd_hold(struct ufs_hba *hba, bool async);
 void ufshcd_release(struct ufs_hba *hba);
 
+/* Expose UFS configfs API's */
+#ifdef CONFIG_CONFIGFS_FS
+int ufshcd_configfs_init(struct ufs_hba *hba);
+void ufshcd_configfs_exit(void);
+#else /* CONFIG_CONFIGFS_FS */
+int ufshcd_configfs_init(struct ufs_hba *hba)
+{
+	return 0;
+}
+
+void ufshcd_configfs_exit(void)
+{
+}
+#endif /* CONFIG_CONFIGFS_FS */
+
 int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
 	int *desc_length);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Christoph Hellwig]

On Fri, Jul 06, 2018 at 11:50:40AM +0530, Sayali Lokhande wrote:
> This patch adds configfs support to provision ufs device at
> runtime. This feature can be primarily useful in factory or
> assembly line as some devices may be required to be configured
> multiple times during initial system development phase.
> Configuration Descriptors can be written multiple times until
> bConfigDescrLock attribute is zero.
> 
> Configuration descriptor buffer consists of Device and Unit
> descriptor configurable parameters which are parsed from vendor
> specific provisioning file and then passed via configfs node at
> runtime to provision ufs device.
> CONFIG_CONFIGFS_FS needs to be enabled for using this feature.

As mentioned before:  this absolutely needs to be a separate
config option so that the code can be compiled out entirely.

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Sayali Lokhande]

On 7/9/2018 1:51 AM, Christoph Hellwig wrote:
> On Fri, Jul 06, 2018 at 11:50:40AM +0530, Sayali Lokhande wrote:
>> This patch adds configfs support to provision ufs device at
>> runtime. This feature can be primarily useful in factory or
>> assembly line as some devices may be required to be configured
>> multiple times during initial system development phase.
>> Configuration Descriptors can be written multiple times until
>> bConfigDescrLock attribute is zero.
>>
>> Configuration descriptor buffer consists of Device and Unit
>> descriptor configurable parameters which are parsed from vendor
>> specific provisioning file and then passed via configfs node at
>> runtime to provision ufs device.
>> CONFIG_CONFIGFS_FS needs to be enabled for using this feature.
> As mentioned before:  this absolutely needs to be a separate
> config option so that the code can be compiled out entirely.
Agree. Will add separate config for UFS Provision and set it dependent 
on CONFIGFS_FS

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Christoph Hellwig]

On Wed, Jul 11, 2018 at 03:20:06PM +0530, Sayali Lokhande wrote:
> Agree. Will add separate config for UFS Provision and set it dependent on
> CONFIGFS_FS

Please add an actual CONFIG_UFS_PROVISIONING user selectable option.

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Evan Green]

Hi Sayali,
Thanks for the prompt spin.

On Thu, Jul 5, 2018 at 11:21 PM Sayali Lokhande <sayalil@codeaurora.org> wrote:
>
> This patch adds configfs support to provision ufs device at

s/ufs/UFS/

> runtime. This feature can be primarily useful in factory or
> assembly line as some devices may be required to be configured
> multiple times during initial system development phase.
> Configuration Descriptors can be written multiple times until
> bConfigDescrLock attribute is zero.
>
> Configuration descriptor buffer consists of Device and Unit
> descriptor configurable parameters which are parsed from vendor
> specific provisioning file and then passed via configfs node at
> runtime to provision ufs device.
> CONFIG_CONFIGFS_FS needs to be enabled for using this feature.
>
> Usage:
> 1) To read current configuration descriptor :
>    cat /config/ufshcd/ufs_provision
> 2) To provision ufs device:
>    echo <config_desc_buf> > /config/ufshcd/ufs_provision
>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  Documentation/ABI/testing/configfs-driver-ufs |  18 +++
>  drivers/scsi/ufs/Makefile                     |   1 +
>  drivers/scsi/ufs/ufs-configfs.c               | 172 ++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.c                     |   2 +
>  drivers/scsi/ufs/ufshcd.h                     |  19 +++
>  5 files changed, 212 insertions(+)
>  create mode 100644 Documentation/ABI/testing/configfs-driver-ufs
>  create mode 100644 drivers/scsi/ufs/ufs-configfs.c
>
> diff --git a/Documentation/ABI/testing/configfs-driver-ufs b/Documentation/ABI/testing/configfs-driver-ufs
> new file mode 100644
> index 0000000..dd16842
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-driver-ufs
> @@ -0,0 +1,18 @@
> +What:          /config/ufshcd/ufs_provision
> +Date:          Jun 2018
> +KernelVersion: 4.14
> +Description:
> +               This file shows the current ufs configuration descriptor set in device.
> +               This can be used to provision ufs device if bConfigDescrLock is 0.
> +               For more details, refer 14.1.6.3 Configuration Descriptor and
> +               Table 14-12 — Unit Descriptor configurable parameters from specs for

Can this be a regular dash, rather than some sort of exotic 0xE2 byte.

> +               description of each configuration descriptor parameter.
> +               Configuration descriptor buffer needs to be passed in space separated
> +               format specificied as below:
> +               echo <bLength> <bDescriptorIDN> <bConfDescContinue> <bBootEnable>
> +               <bDescrAccessEn> <bInitPowerMode> <bHighPriorityLUN>
> +               <bSecureRemovalType> <bInitActiveICCLevel> <wPeriodicRTCUpdate>
> +               <0Bh:0Fh_ReservedAs_0> <bLUEnable> <bBootLunID> <bLUWriteProtect>
> +               <bMemoryType> <dNumAllocUnits> <bDataReliability> <bLogicalBlockSize>
> +               <bProvisioningType> <wContextCapabilities> <0Dh:0Fh_ReservedAs_0>

I wonder if at this point we should be using a binary attribute rather
than a text one. Since now you're basically just converting text to
bytes, it's not very human anymore.

> +               > /config/ufshcd/ufs_provision
> diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
> index 918f579..d438e74 100644
> --- a/drivers/scsi/ufs/Makefile
> +++ b/drivers/scsi/ufs/Makefile
> @@ -5,5 +5,6 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-d
>  obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
>  obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
>  ufshcd-core-objs := ufshcd.o ufs-sysfs.o
> +obj-$(CONFIG_CONFIGFS_FS) += ufs-configfs.o
>  obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
>  obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
> diff --git a/drivers/scsi/ufs/ufs-configfs.c b/drivers/scsi/ufs/ufs-configfs.c
> new file mode 100644
> index 0000000..7d207fc
> --- /dev/null
> +++ b/drivers/scsi/ufs/ufs-configfs.c
> @@ -0,0 +1,172 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright (c) 2018, Linux Foundation.
> +
> +#include <linux/configfs.h>
> +#include <linux/err.h>
> +#include <linux/string.h>
> +
> +#include "ufs.h"
> +#include "ufshcd.h"
> +
> +static inline struct ufs_hba *config_item_to_hba(struct config_item *item)
> +{
> +       struct config_group *group = to_config_group(item);
> +       struct configfs_subsystem *subsys = to_configfs_subsystem(group);
> +       struct ufs_hba *hba = container_of(subsys, struct ufs_hba, subsys);
> +
> +       return hba ? hba : NULL;
> +}
> +
> +static ssize_t ufs_provision_show(struct config_item *item, char *buf)
> +{
> +       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
> +       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
> +       int i, ret, curr_len = 0;
> +       struct ufs_hba *hba = config_item_to_hba(item);
> +
> +       if (!hba)
> +               return snprintf(buf, PAGE_SIZE, "ufs hba is NULL!\n");
> +
> +       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
> +               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
> +
> +       if (ret)
> +               return snprintf(buf, PAGE_SIZE,
> +               "Failed reading descriptor. desc_idn %d, opcode %x ret %d\n",
> +               QUERY_DESC_IDN_CONFIGURATION,
> +               UPIU_QUERY_OPCODE_READ_DESC, ret);
> +
> +       for (i = 0; i < buff_len; i++)
> +               curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
> +                               "0x%x ", desc_buf[i]);
> +
> +       return curr_len;
> +}
> +
> +ssize_t ufshcd_desc_configfs_store(struct ufs_hba *hba,
> +               const char *buf, size_t count)
> +{
> +       char *strbuf;
> +       char *strbuf_copy;
> +       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
> +       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
> +       char *token;
> +       int i, ret, value;
> +       u32 bConfigDescrLock = 0;
> +
> +       /* reserve one byte for null termination */
> +       strbuf = kmalloc(count + 1, GFP_KERNEL);
> +       if (!strbuf)
> +               return -ENOMEM;
> +
> +       strbuf_copy = strbuf;
> +       strlcpy(strbuf, buf, count + 1);
> +
> +       if (!hba)
> +               goto out;
> +
> +       /* Just return if bConfigDescrLock is already set */
> +       ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +               QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &bConfigDescrLock);
> +       if (ret) {
> +               dev_err(hba->dev, "%s: Failed reading bConfigDescrLock %d, cannot re-provision device!\n",
> +                       __func__, ret);

I think ufshcd_query_attr has its own prints on failure, we probably
don't need this one.

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

You're printing token, which you know to be null. Is this print really useful?

> +                       break;
> +               }
> +
> +               ret = kstrtoint(token, 0, &value);
> +               if (ret) {
> +                       dev_err(hba->dev, "%s: kstrtoint failed %d %s\n",
> +                               __func__, ret, token);
> +                       break;
> +               }
> +               desc_buf[i] = (u8)value;
> +               dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);

This print is probably a bit noisy. If you really want to dump the
contents in the debug spew there's a print_hex_dump you can do after
you break out of the loop. Then you could also remove the print for
"token %s, i %d".

> +       }
> +
> +       /* Write configuration descriptor to provision ufs */
> +       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
> +               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
> +
> +       if (ret)
> +               dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x ret %d\n",
> +               __func__, QUERY_DESC_IDN_CONFIGURATION,
> +               UPIU_QUERY_OPCODE_WRITE_DESC, ret);

This is basically already covered by prints inside
ufshcd_query_descriptor_retry.

> +       else
> +               dev_err(hba->dev, "%s: UFS Provisioning done, reboot now!\n",
> +               __func__);
> +
> +out:
> +       kfree(strbuf_copy);
> +       return count;
> +}
> +
> +static ssize_t ufs_provision_store(struct config_item *item,
> +               const char *buf, size_t count)
> +{
> +       struct ufs_hba *hba = config_item_to_hba(item);
> +
> +       return ufshcd_desc_configfs_store(hba, buf, count);
> +}
> +
> +static struct configfs_attribute ufshcd_attr_provision = {
> +       .ca_name        = "ufs_provision",
> +       .ca_mode        = 0666,

This seems too permissive. You don't want just anybody to be able to
re-provision the disk. Maybe 0644?

> +       .ca_owner       = THIS_MODULE,
> +       .show           = ufs_provision_show,
> +       .store          = ufs_provision_store,
> +};
> +
> +static struct configfs_attribute *ufshcd_attrs[] = {
> +       &ufshcd_attr_provision,
> +       NULL,
> +};
> +
> +static struct config_item_type ufscfg_type = {
> +       .ct_attrs       = ufshcd_attrs,
> +       .ct_owner       = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem ufscfg_subsys = {
> +       .su_group = {
> +               .cg_item = {
> +                       .ci_namebuf = "ufshcd",
> +                       .ci_type = &ufscfg_type,
> +               },
> +       },
> +};
> +
> +int ufshcd_configfs_init(struct ufs_hba *hba)
> +{
> +       int ret;
> +       struct configfs_subsystem *subsys = &hba->subsys;
> +
> +       subsys->su_group = ufscfg_subsys.su_group;
> +       config_group_init(&subsys->su_group);
> +       mutex_init(&subsys->su_mutex);
> +       ret = configfs_register_subsystem(subsys);

Wait so for every host controller, you register a subsystem called
"ufshcd"? Won't that result in a naming conflict when the second
controller comes along? I was expecting to see subsystem registration
happen once, probably in a driver init function, and then some sort of
device specific registration happen here. You'd need a unique name for
each controller, maybe the device name itself?

-Evan

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Sayali Lokhande]

Hi Evan,


On 7/9/2018 11:18 PM, Evan Green wrote:
> Hi Sayali,
> Thanks for the prompt spin.
>
> On Thu, Jul 5, 2018 at 11:21 PM Sayali Lokhande <sayalil@codeaurora.org> wrote:
>> This patch adds configfs support to provision ufs device at
> s/ufs/UFS/
Will update.
>> runtime. This feature can be primarily useful in factory or
>> assembly line as some devices may be required to be configured
>> multiple times during initial system development phase.
>> Configuration Descriptors can be written multiple times until
>> bConfigDescrLock attribute is zero.
>>
>> Configuration descriptor buffer consists of Device and Unit
>> descriptor configurable parameters which are parsed from vendor
>> specific provisioning file and then passed via configfs node at
>> runtime to provision ufs device.
>> CONFIG_CONFIGFS_FS needs to be enabled for using this feature.
>>
>> Usage:
>> 1) To read current configuration descriptor :
>>     cat /config/ufshcd/ufs_provision
>> 2) To provision ufs device:
>>     echo <config_desc_buf> > /config/ufshcd/ufs_provision
>>
>> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
>> ---
>>   Documentation/ABI/testing/configfs-driver-ufs |  18 +++
>>   drivers/scsi/ufs/Makefile                     |   1 +
>>   drivers/scsi/ufs/ufs-configfs.c               | 172 ++++++++++++++++++++++++++
>>   drivers/scsi/ufs/ufshcd.c                     |   2 +
>>   drivers/scsi/ufs/ufshcd.h                     |  19 +++
>>   5 files changed, 212 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/configfs-driver-ufs
>>   create mode 100644 drivers/scsi/ufs/ufs-configfs.c
>>
>> diff --git a/Documentation/ABI/testing/configfs-driver-ufs b/Documentation/ABI/testing/configfs-driver-ufs
>> new file mode 100644
>> index 0000000..dd16842
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/configfs-driver-ufs
>> @@ -0,0 +1,18 @@
>> +What:          /config/ufshcd/ufs_provision
>> +Date:          Jun 2018
>> +KernelVersion: 4.14
>> +Description:
>> +               This file shows the current ufs configuration descriptor set in device.
>> +               This can be used to provision ufs device if bConfigDescrLock is 0.
>> +               For more details, refer 14.1.6.3 Configuration Descriptor and
>> +               Table 14-12 — Unit Descriptor configurable parameters from specs for
> Can this be a regular dash, rather than some sort of exotic 0xE2 byte.
>
>> +               description of each configuration descriptor parameter.
>> +               Configuration descriptor buffer needs to be passed in space separated
>> +               format specificied as below:
>> +               echo <bLength> <bDescriptorIDN> <bConfDescContinue> <bBootEnable>
>> +               <bDescrAccessEn> <bInitPowerMode> <bHighPriorityLUN>
>> +               <bSecureRemovalType> <bInitActiveICCLevel> <wPeriodicRTCUpdate>
>> +               <0Bh:0Fh_ReservedAs_0> <bLUEnable> <bBootLunID> <bLUWriteProtect>
>> +               <bMemoryType> <dNumAllocUnits> <bDataReliability> <bLogicalBlockSize>
>> +               <bProvisioningType> <wContextCapabilities> <0Dh:0Fh_ReservedAs_0>
> I wonder if at this point we should be using a binary attribute rather
> than a text one. Since now you're basically just converting text to
> bytes, it's not very human anymore.
Use of binary attr was ruled out before. Pasting previous comments from 
Greg :
"binary attributes are meant for hardware value pass-through" type stuff.
Unless this is "raw" data straight from the hardware, binary does not work.
>> +               > /config/ufshcd/ufs_provision
>> diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
>> index 918f579..d438e74 100644
>> --- a/drivers/scsi/ufs/Makefile
>> +++ b/drivers/scsi/ufs/Makefile
>> @@ -5,5 +5,6 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-d
>>   obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
>>   obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
>>   ufshcd-core-objs := ufshcd.o ufs-sysfs.o
>> +obj-$(CONFIG_CONFIGFS_FS) += ufs-configfs.o
>>   obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
>>   obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
>> diff --git a/drivers/scsi/ufs/ufs-configfs.c b/drivers/scsi/ufs/ufs-configfs.c
>> new file mode 100644
>> index 0000000..7d207fc
>> --- /dev/null
>> +++ b/drivers/scsi/ufs/ufs-configfs.c
>> @@ -0,0 +1,172 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +// Copyright (c) 2018, Linux Foundation.
>> +
>> +#include <linux/configfs.h>
>> +#include <linux/err.h>
>> +#include <linux/string.h>
>> +
>> +#include "ufs.h"
>> +#include "ufshcd.h"
>> +
>> +static inline struct ufs_hba *config_item_to_hba(struct config_item *item)
>> +{
>> +       struct config_group *group = to_config_group(item);
>> +       struct configfs_subsystem *subsys = to_configfs_subsystem(group);
>> +       struct ufs_hba *hba = container_of(subsys, struct ufs_hba, subsys);
>> +
>> +       return hba ? hba : NULL;
>> +}
>> +
>> +static ssize_t ufs_provision_show(struct config_item *item, char *buf)
>> +{
>> +       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
>> +       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
>> +       int i, ret, curr_len = 0;
>> +       struct ufs_hba *hba = config_item_to_hba(item);
>> +
>> +       if (!hba)
>> +               return snprintf(buf, PAGE_SIZE, "ufs hba is NULL!\n");
>> +
>> +       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
>> +               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
>> +
>> +       if (ret)
>> +               return snprintf(buf, PAGE_SIZE,
>> +               "Failed reading descriptor. desc_idn %d, opcode %x ret %d\n",
>> +               QUERY_DESC_IDN_CONFIGURATION,
>> +               UPIU_QUERY_OPCODE_READ_DESC, ret);
>> +
>> +       for (i = 0; i < buff_len; i++)
>> +               curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
>> +                               "0x%x ", desc_buf[i]);
>> +
>> +       return curr_len;
>> +}
>> +
>> +ssize_t ufshcd_desc_configfs_store(struct ufs_hba *hba,
>> +               const char *buf, size_t count)
>> +{
>> +       char *strbuf;
>> +       char *strbuf_copy;
>> +       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
>> +       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
>> +       char *token;
>> +       int i, ret, value;
>> +       u32 bConfigDescrLock = 0;
>> +
>> +       /* reserve one byte for null termination */
>> +       strbuf = kmalloc(count + 1, GFP_KERNEL);
>> +       if (!strbuf)
>> +               return -ENOMEM;
>> +
>> +       strbuf_copy = strbuf;
>> +       strlcpy(strbuf, buf, count + 1);
>> +
>> +       if (!hba)
>> +               goto out;
>> +
>> +       /* Just return if bConfigDescrLock is already set */
>> +       ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> +               QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &bConfigDescrLock);
>> +       if (ret) {
>> +               dev_err(hba->dev, "%s: Failed reading bConfigDescrLock %d, cannot re-provision device!\n",
>> +                       __func__, ret);
> I think ufshcd_query_attr has its own prints on failure, we probably
> don't need this one.
Will check and update.
>> +               goto out;
>> +       }
>> +       if (bConfigDescrLock) {
>> +               dev_err(hba->dev, "%s: bConfigDescrLock already set to %u, cannot re-provision device!\n",
>> +               __func__, bConfigDescrLock);
>> +               goto out;
>> +       }
>> +
>> +       for (i = 0; i < QUERY_DESC_CONFIGURATION_DEF_SIZE; i++) {
>> +               token = strsep(&strbuf, " ");
>> +               if (!token && i) {
>> +                       dev_dbg(hba->dev, "%s: token %s, i %d\n",
>> +                               __func__, token, i);
> You're printing token, which you know to be null. Is this print really useful?
Will check and update/remove this print.
>> +                       break;
>> +               }
>> +
>> +               ret = kstrtoint(token, 0, &value);
>> +               if (ret) {
>> +                       dev_err(hba->dev, "%s: kstrtoint failed %d %s\n",
>> +                               __func__, ret, token);
>> +                       break;
>> +               }
>> +               desc_buf[i] = (u8)value;
>> +               dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);
> This print is probably a bit noisy. If you really want to dump the
> contents in the debug spew there's a print_hex_dump you can do after
> you break out of the loop. Then you could also remove the print for
> "token %s, i %d".
Will check and update.
>> +       }
>> +
>> +       /* Write configuration descriptor to provision ufs */
>> +       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
>> +               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
>> +
>> +       if (ret)
>> +               dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x ret %d\n",
>> +               __func__, QUERY_DESC_IDN_CONFIGURATION,
>> +               UPIU_QUERY_OPCODE_WRITE_DESC, ret);
> This is basically already covered by prints inside
> ufshcd_query_descriptor_retry.
Will update.
>> +       else
>> +               dev_err(hba->dev, "%s: UFS Provisioning done, reboot now!\n",
>> +               __func__);
>> +
>> +out:
>> +       kfree(strbuf_copy);
>> +       return count;
>> +}
>> +
>> +static ssize_t ufs_provision_store(struct config_item *item,
>> +               const char *buf, size_t count)
>> +{
>> +       struct ufs_hba *hba = config_item_to_hba(item);
>> +
>> +       return ufshcd_desc_configfs_store(hba, buf, count);
>> +}
>> +
>> +static struct configfs_attribute ufshcd_attr_provision = {
>> +       .ca_name        = "ufs_provision",
>> +       .ca_mode        = 0666,
> This seems too permissive. You don't want just anybody to be able to
> re-provision the disk. Maybe 0644?
Agreed. will update.
>> +       .ca_owner       = THIS_MODULE,
>> +       .show           = ufs_provision_show,
>> +       .store          = ufs_provision_store,
>> +};
>> +
>> +static struct configfs_attribute *ufshcd_attrs[] = {
>> +       &ufshcd_attr_provision,
>> +       NULL,
>> +};
>> +
>> +static struct config_item_type ufscfg_type = {
>> +       .ct_attrs       = ufshcd_attrs,
>> +       .ct_owner       = THIS_MODULE,
>> +};
>> +
>> +static struct configfs_subsystem ufscfg_subsys = {
>> +       .su_group = {
>> +               .cg_item = {
>> +                       .ci_namebuf = "ufshcd",
>> +                       .ci_type = &ufscfg_type,
>> +               },
>> +       },
>> +};
>> +
>> +int ufshcd_configfs_init(struct ufs_hba *hba)
>> +{
>> +       int ret;
>> +       struct configfs_subsystem *subsys = &hba->subsys;
>> +
>> +       subsys->su_group = ufscfg_subsys.su_group;
>> +       config_group_init(&subsys->su_group);
>> +       mutex_init(&subsys->su_mutex);
>> +       ret = configfs_register_subsystem(subsys);
> Wait so for every host controller, you register a subsystem called
> "ufshcd"? Won't that result in a naming conflict when the second
> controller comes along? I was expecting to see subsystem registration
> happen once, probably in a driver init function, and then some sort of
> device specific registration happen here. You'd need a unique name for
> each controller, maybe the device name itself?
Agreed. Will check and pass device name itself during init.
> -Evan

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Evan Green]

Hi Sayali,
On Mon, Jul 16, 2018 at 1:10 AM Sayali Lokhande <sayalil@codeaurora.org> wrote:
>
> Hi Evan,
>
>
> On 7/9/2018 11:18 PM, Evan Green wrote:
> > Hi Sayali,
> > Thanks for the prompt spin.
> >
> > On Thu, Jul 5, 2018 at 11:21 PM Sayali Lokhande <sayalil@codeaurora.org> wrote:
> >> This patch adds configfs support to provision ufs device at
> > s/ufs/UFS/
> Will update.
> >> runtime. This feature can be primarily useful in factory or
> >> assembly line as some devices may be required to be configured
> >> multiple times during initial system development phase.
> >> Configuration Descriptors can be written multiple times until
> >> bConfigDescrLock attribute is zero.
> >>
> >> Configuration descriptor buffer consists of Device and Unit
> >> descriptor configurable parameters which are parsed from vendor
> >> specific provisioning file and then passed via configfs node at
> >> runtime to provision ufs device.
> >> CONFIG_CONFIGFS_FS needs to be enabled for using this feature.
> >>
> >> Usage:
> >> 1) To read current configuration descriptor :
> >>     cat /config/ufshcd/ufs_provision
> >> 2) To provision ufs device:
> >>     echo <config_desc_buf> > /config/ufshcd/ufs_provision
> >>
> >> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> >> ---
> >>   Documentation/ABI/testing/configfs-driver-ufs |  18 +++
> >>   drivers/scsi/ufs/Makefile                     |   1 +
> >>   drivers/scsi/ufs/ufs-configfs.c               | 172 ++++++++++++++++++++++++++
> >>   drivers/scsi/ufs/ufshcd.c                     |   2 +
> >>   drivers/scsi/ufs/ufshcd.h                     |  19 +++
> >>   5 files changed, 212 insertions(+)
> >>   create mode 100644 Documentation/ABI/testing/configfs-driver-ufs
> >>   create mode 100644 drivers/scsi/ufs/ufs-configfs.c
> >>
> >> diff --git a/Documentation/ABI/testing/configfs-driver-ufs b/Documentation/ABI/testing/configfs-driver-ufs
> >> new file mode 100644
> >> index 0000000..dd16842
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/configfs-driver-ufs
> >> @@ -0,0 +1,18 @@
> >> +What:          /config/ufshcd/ufs_provision
> >> +Date:          Jun 2018
> >> +KernelVersion: 4.14
> >> +Description:
> >> +               This file shows the current ufs configuration descriptor set in device.
> >> +               This can be used to provision ufs device if bConfigDescrLock is 0.
> >> +               For more details, refer 14.1.6.3 Configuration Descriptor and
> >> +               Table 14-12 — Unit Descriptor configurable parameters from specs for
> > Can this be a regular dash, rather than some sort of exotic 0xE2 byte.
> >
> >> +               description of each configuration descriptor parameter.
> >> +               Configuration descriptor buffer needs to be passed in space separated
> >> +               format specificied as below:
> >> +               echo <bLength> <bDescriptorIDN> <bConfDescContinue> <bBootEnable>
> >> +               <bDescrAccessEn> <bInitPowerMode> <bHighPriorityLUN>
> >> +               <bSecureRemovalType> <bInitActiveICCLevel> <wPeriodicRTCUpdate>
> >> +               <0Bh:0Fh_ReservedAs_0> <bLUEnable> <bBootLunID> <bLUWriteProtect>
> >> +               <bMemoryType> <dNumAllocUnits> <bDataReliability> <bLogicalBlockSize>
> >> +               <bProvisioningType> <wContextCapabilities> <0Dh:0Fh_ReservedAs_0>
> > I wonder if at this point we should be using a binary attribute rather
> > than a text one. Since now you're basically just converting text to
> > bytes, it's not very human anymore.
> Use of binary attr was ruled out before. Pasting previous comments from
> Greg :
> "binary attributes are meant for hardware value pass-through" type stuff.
> Unless this is "raw" data straight from the hardware, binary does not work.

But now that we've removed the software parameters like lun_to_grow,
and all the calculations, this is exactly a hardware value
pass-through, isn't it? I even see you have things like
<0Bh:0Fh_ReservedAs_0> done in order to match the hardware format.

I see Bart has chimed in on the next series with a suggestion to break
out each field into individual files within configfs. Bart, what are
your feelings about converting to a binary attribute? I remember when
I did my sysfs equivalent of this patch, somebody chimed in indicating
a "commit" file might be needed so that the new configuration could be
written in one fell swoop. One advantage of the binary attribute is
that it writes the configuration atomically.
-Evan

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Bart Van Assche]

On Mon, 2018-07-16 at 16:46 -0700, Evan Green wrote:
> I see Bart has chimed in on the next series with a suggestion to break
> out each field into individual files within configfs. Bart, what are
> your feelings about converting to a binary attribute? I remember when
> I did my sysfs equivalent of this patch, somebody chimed in indicating
> a "commit" file might be needed so that the new configuration could be
> written in one fell swoop. One advantage of the binary attribute is
> that it writes the configuration atomically.

Hello Evan,

I may be missing some UFS background information. But since a configfs interface
is being added I think the same rule applies as to all Linux kernel user space
interfaces, namely that it should be backwards compatible. Additionally, if
anyone ever will want to use this interface from a shell script, I think that
it will be much easier to write multiple ASCII attributes than a single binary
attribute.

Bart.

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Evan Green]

On Mon, Jul 16, 2018 at 5:04 PM Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>
> On Mon, 2018-07-16 at 16:46 -0700, Evan Green wrote:
> > I see Bart has chimed in on the next series with a suggestion to break
> > out each field into individual files within configfs. Bart, what are
> > your feelings about converting to a binary attribute? I remember when
> > I did my sysfs equivalent of this patch, somebody chimed in indicating
> > a "commit" file might be needed so that the new configuration could be
> > written in one fell swoop. One advantage of the binary attribute is
> > that it writes the configuration atomically.
>
> Hello Evan,
>
> I may be missing some UFS background information. But since a configfs interface
> is being added I think the same rule applies as to all Linux kernel user space
> interfaces, namely that it should be backwards compatible. Additionally, if
> anyone ever will want to use this interface from a shell script, I think that
> it will be much easier to write multiple ASCII attributes than a single binary
> attribute.
>

Hi Bart,
I'm unsure about the compatibility aspect for binary attributes that
essentially represent direct windows into hardware. I suppose this
comes down to who this interface is most useful to. Hypothetically
lets say a future revision of UFS adds fields to the configuration
descriptor, but is otherwise backwards compatible. If this interface
is primarily for OEMs initializing their devices in the factory, then
I'd argue they'd want the most direct window to the configuration
descriptor. These folks probably just have a configuration they want
to plunk into the hardware, and would prefer being able to write all
fields over having some sort of compatibility restriction. If, on the
other hand, this is used by long-running scripts that stick around for
years without modification, then yes, it seems like it would be more
important to stay compatible, and have smarts in the kernel to make
writes of old descriptors work in new devices.

At least for myself, I fall into the category of someone who just
needs to plunk a configuration descriptor in once, and would prefer
not to have to submit kernel changes if the descriptor evolves
slightly. It also seemed a little odd that this patch now spends a
bunch of energy converting ASCII into bytes, just to write it without
modification into the hardware, and convert back again to ASCII for
reads.

We plan to use a script for provisioning, and could easily handle
ASCII or rawbytes:

# Some bytes, ready to go with the interface today...
some_bytes="00 01 02 03"

# Same bytes, now in binary format
bytes_fmt=$(echo " $some_bytes" | sed 's/ /\\x/g')
/usr/bin/printf "$bytes_fmt" > /configfs/ufs_provision

I'm not dead set on binary, since as above I could do it either way,
but it seemed worth at least talking through. Let me know what you
think.
-Evan

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Bart Van Assche]

On Tue, 2018-07-17 at 13:23 -0700, Evan Green wrote:
> On Mon, Jul 16, 2018 at 5:04 PM Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> > 
> > On Mon, 2018-07-16 at 16:46 -0700, Evan Green wrote:
> > > I see Bart has chimed in on the next series with a suggestion to break
> > > out each field into individual files within configfs. Bart, what are
> > > your feelings about converting to a binary attribute? I remember when
> > > I did my sysfs equivalent of this patch, somebody chimed in indicating
> > > a "commit" file might be needed so that the new configuration could be
> > > written in one fell swoop. One advantage of the binary attribute is
> > > that it writes the configuration atomically.
> > 
> > Hello Evan,
> > 
> > I may be missing some UFS background information. But since a configfs interface
> > is being added I think the same rule applies as to all Linux kernel user space
> > interfaces, namely that it should be backwards compatible. Additionally, if
> > anyone ever will want to use this interface from a shell script, I think that
> > it will be much easier to write multiple ASCII attributes than a single binary
> > attribute.
> > 
> 
> Hi Bart,
> I'm unsure about the compatibility aspect for binary attributes that
> essentially represent direct windows into hardware. I suppose this
> comes down to who this interface is most useful to. Hypothetically
> lets say a future revision of UFS adds fields to the configuration
> descriptor, but is otherwise backwards compatible. If this interface
> is primarily for OEMs initializing their devices in the factory, then
> I'd argue they'd want the most direct window to the configuration
> descriptor. These folks probably just have a configuration they want
> to plunk into the hardware, and would prefer being able to write all
> fields over having some sort of compatibility restriction. If, on the
> other hand, this is used by long-running scripts that stick around for
> years without modification, then yes, it seems like it would be more
> important to stay compatible, and have smarts in the kernel to make
> writes of old descriptors work in new devices.
> 
> At least for myself, I fall into the category of someone who just
> needs to plunk a configuration descriptor in once, and would prefer
> not to have to submit kernel changes if the descriptor evolves
> slightly. It also seemed a little odd that this patch now spends a
> bunch of energy converting ASCII into bytes, just to write it without
> modification into the hardware, and convert back again to ASCII for
> reads.
> 
> We plan to use a script for provisioning, and could easily handle
> ASCII or rawbytes:
> 
> # Some bytes, ready to go with the interface today...
> some_bytes="00 01 02 03"
> 
> # Same bytes, now in binary format
> bytes_fmt=$(echo " $some_bytes" | sed 's/ /\\x/g')
> /usr/bin/printf "$bytes_fmt" > /configfs/ufs_provision
> 
> I'm not dead set on binary, since as above I could do it either way,
> but it seemed worth at least talking through. Let me know what you
> think.

The configfs documentation (Documentation/filesystems/configfs/configfs.txt)
is clear about this: "Preferably only one value per file should be used." So
I would like to hear the opinion of someone who has more authority than I
with regard to configfs.

Bart.

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[gregkh]

On Tue, Jul 17, 2018 at 09:06:35PM +0000, Bart Van Assche wrote:
> On Tue, 2018-07-17 at 13:23 -0700, Evan Green wrote:
> > On Mon, Jul 16, 2018 at 5:04 PM Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> > > 
> > > On Mon, 2018-07-16 at 16:46 -0700, Evan Green wrote:
> > > > I see Bart has chimed in on the next series with a suggestion to break
> > > > out each field into individual files within configfs. Bart, what are
> > > > your feelings about converting to a binary attribute? I remember when
> > > > I did my sysfs equivalent of this patch, somebody chimed in indicating
> > > > a "commit" file might be needed so that the new configuration could be
> > > > written in one fell swoop. One advantage of the binary attribute is
> > > > that it writes the configuration atomically.
> > > 
> > > Hello Evan,
> > > 
> > > I may be missing some UFS background information. But since a configfs interface
> > > is being added I think the same rule applies as to all Linux kernel user space
> > > interfaces, namely that it should be backwards compatible. Additionally, if
> > > anyone ever will want to use this interface from a shell script, I think that
> > > it will be much easier to write multiple ASCII attributes than a single binary
> > > attribute.
> > > 
> > 
> > Hi Bart,
> > I'm unsure about the compatibility aspect for binary attributes that
> > essentially represent direct windows into hardware. I suppose this
> > comes down to who this interface is most useful to. Hypothetically
> > lets say a future revision of UFS adds fields to the configuration
> > descriptor, but is otherwise backwards compatible. If this interface
> > is primarily for OEMs initializing their devices in the factory, then
> > I'd argue they'd want the most direct window to the configuration
> > descriptor. These folks probably just have a configuration they want
> > to plunk into the hardware, and would prefer being able to write all
> > fields over having some sort of compatibility restriction. If, on the
> > other hand, this is used by long-running scripts that stick around for
> > years without modification, then yes, it seems like it would be more
> > important to stay compatible, and have smarts in the kernel to make
> > writes of old descriptors work in new devices.
> > 
> > At least for myself, I fall into the category of someone who just
> > needs to plunk a configuration descriptor in once, and would prefer
> > not to have to submit kernel changes if the descriptor evolves
> > slightly. It also seemed a little odd that this patch now spends a
> > bunch of energy converting ASCII into bytes, just to write it without
> > modification into the hardware, and convert back again to ASCII for
> > reads.
> > 
> > We plan to use a script for provisioning, and could easily handle
> > ASCII or rawbytes:
> > 
> > # Some bytes, ready to go with the interface today...
> > some_bytes="00 01 02 03"
> > 
> > # Same bytes, now in binary format
> > bytes_fmt=$(echo " $some_bytes" | sed 's/ /\\x/g')
> > /usr/bin/printf "$bytes_fmt" > /configfs/ufs_provision
> > 
> > I'm not dead set on binary, since as above I could do it either way,
> > but it seemed worth at least talking through. Let me know what you
> > think.
> 
> The configfs documentation (Documentation/filesystems/configfs/configfs.txt)
> is clear about this: "Preferably only one value per file should be used." So
> I would like to hear the opinion of someone who has more authority than I
> with regard to configfs.

Don't we have "binary" files for configfs?  We have them for sysfs, they
are for files that are not touched by the kernel and just "pass-through"
to the hardware.  Would that work here as well?

thanks,

greg k-h

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Bart Van Assche]

On Wed, 2018-07-18 at 10:56 +0200, gregkh@linuxfoundation.org wrote:
> On Tue, Jul 17, 2018 at 09:06:35PM +0000, Bart Van Assche wrote:
> > On Tue, 2018-07-17 at 13:23 -0700, Evan Green wrote:
> > > I'm not dead set on binary, since as above I could do it either way,
> > > but it seemed worth at least talking through. Let me know what you
> > > think.
> > 
> > The configfs documentation (Documentation/filesystems/configfs/configfs.txt)
> > is clear about this: "Preferably only one value per file should be used." So
> > I would like to hear the opinion of someone who has more authority than I
> > with regard to configfs.
> 
> Don't we have "binary" files for configfs?  We have them for sysfs, they
> are for files that are not touched by the kernel and just "pass-through"
> to the hardware.  Would that work here as well?

If a new version of the UFS spec would be introduced and that new version of the
spec introduces a new layout for the binary descriptor, will it be possible for
user space software to figure out which version of the binary descriptor format
that has to be used?

Thanks,

Bart.

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[gregkh]

On Wed, Jul 18, 2018 at 05:30:07PM +0000, Bart Van Assche wrote:
> On Wed, 2018-07-18 at 10:56 +0200, gregkh@linuxfoundation.org wrote:
> > On Tue, Jul 17, 2018 at 09:06:35PM +0000, Bart Van Assche wrote:
> > > On Tue, 2018-07-17 at 13:23 -0700, Evan Green wrote:
> > > > I'm not dead set on binary, since as above I could do it either way,
> > > > but it seemed worth at least talking through. Let me know what you
> > > > think.
> > > 
> > > The configfs documentation (Documentation/filesystems/configfs/configfs.txt)
> > > is clear about this: "Preferably only one value per file should be used." So
> > > I would like to hear the opinion of someone who has more authority than I
> > > with regard to configfs.
> > 
> > Don't we have "binary" files for configfs?  We have them for sysfs, they
> > are for files that are not touched by the kernel and just "pass-through"
> > to the hardware.  Would that work here as well?
> 
> If a new version of the UFS spec would be introduced and that new version of the
> spec introduces a new layout for the binary descriptor, will it be possible for
> user space software to figure out which version of the binary descriptor format
> that has to be used?

If a new UFS spec was crazy enough to keep the same field name but
change the layout of the field, well, the UFS spec authors deserve all
of the pain and suffering that would cause to be heaped on them.

Seriously, it's not hard to do this right, go fix the spec before they
do something stupid.

And you are reporting the version of the UFS spec that your device
supports to userspace, right?  So is this really a problem even if the
spec authors are that foolish?

thanks,

greg k-h

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Sayali Lokhande]

Hi Evan,


On 7/18/2018 1:53 AM, Evan Green wrote:
> On Mon, Jul 16, 2018 at 5:04 PM Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>> On Mon, 2018-07-16 at 16:46 -0700, Evan Green wrote:
>>> I see Bart has chimed in on the next series with a suggestion to break
>>> out each field into individual files within configfs. Bart, what are
>>> your feelings about converting to a binary attribute? I remember when
>>> I did my sysfs equivalent of this patch, somebody chimed in indicating
>>> a "commit" file might be needed so that the new configuration could be
>>> written in one fell swoop. One advantage of the binary attribute is
>>> that it writes the configuration atomically.
>> Hello Evan,
>>
>> I may be missing some UFS background information. But since a configfs interface
>> is being added I think the same rule applies as to all Linux kernel user space
>> interfaces, namely that it should be backwards compatible. Additionally, if
>> anyone ever will want to use this interface from a shell script, I think that
>> it will be much easier to write multiple ASCII attributes than a single binary
>> attribute.
>>
> Hi Bart,
> I'm unsure about the compatibility aspect for binary attributes that
> essentially represent direct windows into hardware. I suppose this
> comes down to who this interface is most useful to. Hypothetically
> lets say a future revision of UFS adds fields to the configuration
> descriptor, but is otherwise backwards compatible. If this interface
> is primarily for OEMs initializing their devices in the factory, then
> I'd argue they'd want the most direct window to the configuration
> descriptor. These folks probably just have a configuration they want
> to plunk into the hardware, and would prefer being able to write all
> fields over having some sort of compatibility restriction. If, on the
> other hand, this is used by long-running scripts that stick around for
> years without modification, then yes, it seems like it would be more
> important to stay compatible, and have smarts in the kernel to make
> writes of old descriptors work in new devices.
>
> At least for myself, I fall into the category of someone who just
> needs to plunk a configuration descriptor in once, and would prefer
> not to have to submit kernel changes if the descriptor evolves
> slightly. It also seemed a little odd that this patch now spends a
> bunch of energy converting ASCII into bytes, just to write it without
> modification into the hardware, and convert back again to ASCII for
> reads.
>
> We plan to use a script for provisioning, and could easily handle
> ASCII or rawbytes:
>
> # Some bytes, ready to go with the interface today...
> some_bytes="00 01 02 03"
>
> # Same bytes, now in binary format
> bytes_fmt=$(echo " $some_bytes" | sed 's/ /\\x/g')
> /usr/bin/printf "$bytes_fmt" > /configfs/ufs_provision
>
> I'm not dead set on binary, since as above I could do it either way,
> but it seemed worth at least talking through. Let me know what you
> think.
> -Evan

I am using ASCII format for reading/writing to config desc as it will be 
more readable for users and easier/comfortable to compare any value to 
default spec value(if required).
So I don't really see any harm in using current ASCII format for 
provisioning purpose.
-Sayali

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Evan Green]

On Mon, Jul 30, 2018 at 12:46 AM Sayali Lokhande <sayalil@codeaurora.org> wrote:
>
> Hi Evan,
>
>
> On 7/18/2018 1:53 AM, Evan Green wrote:
...
> > I'm not dead set on binary, since as above I could do it either way,
> > but it seemed worth at least talking through. Let me know what you
> > think.
> > -Evan
>
> I am using ASCII format for reading/writing to config desc as it will be
> more readable for users and easier/comfortable to compare any value to
> default spec value(if required).
> So I don't really see any harm in using current ASCII format for
> provisioning purpose.

I'm not convinced by those arguments, but ultimately it's between you
and the maintainers. If you're going with the ASCII route, then I have
another review comment. Currently in your patch, if kstrtoint fails,
you print, but then break out of the loop and try to write the
partially parsed descriptor anyway. That "break" should probably be
changed to a "goto out".

-Evan

Re: [PATCH V5 2/2] scsi: ufs: Add configfs support for ufs provisioning

[Sayali Lokhande]

On 7/31/2018 5:09 AM, Evan Green wrote:
> On Mon, Jul 30, 2018 at 12:46 AM Sayali Lokhande <sayalil@codeaurora.org> wrote:
>> Hi Evan,
>>
>>
>> On 7/18/2018 1:53 AM, Evan Green wrote:
> ...
>>> I'm not dead set on binary, since as above I could do it either way,
>>> but it seemed worth at least talking through. Let me know what you
>>> think.
>>> -Evan
>> I am using ASCII format for reading/writing to config desc as it will be
>> more readable for users and easier/comfortable to compare any value to
>> default spec value(if required).
>> So I don't really see any harm in using current ASCII format for
>> provisioning purpose.
> I'm not convinced by those arguments, but ultimately it's between you
> and the maintainers. If you're going with the ASCII route, then I have
> another review comment. Currently in your patch, if kstrtoint fails,
> you print, but then break out of the loop and try to write the
> partially parsed descriptor anyway. That "break" should probably be
> changed to a "goto out".
>
> -Evan
Agreed. I will replace that break with "goto out" .