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