Messages in this thread | | | Subject | Re: [PATCH V10 2/2] scsi: ufs: Add configfs support for UFS provisioning | From | Sayali Lokhande <> | Date | Tue, 28 Aug 2018 18:08:01 +0530 |
| |
On 8/28/2018 5:36 PM, Adrian Hunter wrote: > On 28/08/18 14:40, Sayali Lokhande wrote: >> On 8/28/2018 4:56 PM, Adrian Hunter wrote: >>> On 28/08/18 13:23, Sayali Lokhande wrote: >>>> Hi Adrian, >>>> >>>> On 8/24/2018 2:15 PM, Adrian Hunter wrote: >>>>> On 24/08/18 08:33, 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. >>>>>> >>>>>> Usage: >>>>>> 1) To read current configuration descriptor : >>>>>> cat /config/XXXX.ufshc/ufs_provision >>>>>> 2) To provision ufs device: >>>>>> echo <config_desc_buf> > /config/XXXX.ufshc/ufs_provision >>>>>> >>>>>> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org> >>>>>> --- >>>>>> Documentation/ABI/testing/configfs-driver-ufs | 18 +++ >>>>>> drivers/scsi/ufs/Kconfig | 10 ++ >>>>>> drivers/scsi/ufs/Makefile | 1 + >>>>>> drivers/scsi/ufs/ufs-configfs.c | 155 >>>>>> ++++++++++++++++++++++++++ >>>>>> drivers/scsi/ufs/ufshcd.c | 3 +- >>>>>> drivers/scsi/ufs/ufshcd.h | 17 +++ >>>>>> 6 files changed, 203 insertions(+), 1 deletion(-) >>>>>> 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..eeee499c >>>>>> --- /dev/null >>>>>> +++ b/Documentation/ABI/testing/configfs-driver-ufs >>>>>> @@ -0,0 +1,18 @@ >>>>>> +What: /config/ufshcd/ufs_provision >>>>> You are using the device name as the config subsystem name, so this is >>>>> really: >>>>> >>>>> What: /config/*/ufs_provision >>>> Agree. Will update. >>>>>> +Date: Jun 2018 >>>>>> +KernelVersion: 4.14 >>>>> 4.19 maybe >>>> Done. >>>>>> +Description: >>>>>> + This file shows the current ufs configuration descriptor set in >>>>>> device. >>>>> It is only the first config descriptor though isn't it? There is no >>>>> support >>>>> for the other 3. Shouldn't we support config descriptors fully? >>>> Yes, we should support config descriptors fully. My current implementation >>>> only supports config desc (index = 00h). >>>> I m thinking I can add separate "index" field along with "ufs_provision" >>>> field and can use this index (instead of hard coding to 0) >>>> to write repective config_desc. In this way, we should first set the "index" >>>> and then read/write to "ufs_provision". >>>> Let me know if you have any concern with this approach ? >>> One file for each config descriptor would seem more normal. >> I can make separate file for each config descriptor index(say >> ufs_provision_0/1/2/3), but still show() and store() api's can remain common. >> Will try it out and update. >>>>>> + 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: >>>>> specificied -> specified >>>> Done >>>>>> + 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> >>>>> This format is not exactly correct anymore because each byte has to be >>>>> represented separately. Also theoretically it can be change, so maybe >>>>> better just to refer the user to the UFS specification. >>>> Agree. Will update to just refer to UFS spec (for Config_desc format). >>>> >>>>>> + > /config/XXXX.ufshc/ufs_provision >>>>>> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig >>>>>> index e27b4d4..a9a0a58 100644 >>>>>> --- a/drivers/scsi/ufs/Kconfig >>>>>> +++ b/drivers/scsi/ufs/Kconfig >>>>>> @@ -100,3 +100,13 @@ config SCSI_UFS_QCOM >>>>>> Select this if you have UFS controller on QCOM chipset. >>>>>> If unsure, say N. >>>>>> + >>>>>> +config UFS_PROVISION >>>>>> + tristate "Runtime UFS Provisioning support" >>>>>> + depends on SCSI_UFSHCD && CONFIGFS_FS >>>>>> + help >>>>>> + This enables runtime UFS provisioning support. This can be used >>>>>> + primarily during assembly line as some devices may be required to >>>>>> + be configured multiple times during initial development phase. >>>>>> + >>>>>> + If unsure, say N. >>>>>> diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile >>>>>> index 918f579..00110ea 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_UFS_PROVISION) += 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..74563a4 >>>>>> --- /dev/null >>>>>> +++ b/drivers/scsi/ufs/ufs-configfs.c >>>>>> @@ -0,0 +1,155 @@ >>>>>> +// 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 subsys ? hba : NULL; >>>>>> +} >>>>>> + >>>>>> +static ssize_t ufs_provision_show(struct config_item *item, char *buf) >>>>>> +{ >>>>>> + u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0}; >>>>> Is it really ok to hard-code the descriptor size? >>>> I will use ufshcd_read_desc_length() api for each index as required. Will >>>> remove this hard-code. >>> Alternatively, I see there is already hba->desc_size.conf_desc >> hba->desc_size.conf_desc is actually size for index 0. I thought what if >> desc size various per index. >> So better to read for each index. What say ? > Based on the 'B' and 'L' values defined in the spec., all configuration > descriptors have the same length. Ok. Got it. Then I will just use "hba->desc_size.conf_desc" as desc length. >>>>>> + int desc_buf_len = QUERY_DESC_CONFIGURATION_DEF_SIZE; >>>>>> + int i, ret, curr_len = 0; >>>>>> + struct ufs_hba *hba = config_item_to_hba(item); >>>>>> + >>>>>> + ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC, >>>>>> + QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &desc_buf_len); >>>>>> + >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + for (i = 0; i < desc_buf_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 desc_buf_len = QUERY_DESC_CONFIGURATION_DEF_SIZE; >>>>>> + char *token; >>>>>> + int i, ret, value; >>>>>> + u32 config_desc_lock = 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); >>>>>> + >>>>>> + /* 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, &config_desc_lock); >>>>>> + if (ret) >>>>>> + goto out; >>>>>> + >>>>>> + if (config_desc_lock) { >>>>>> + dev_err(hba->dev, "%s: bConfigDescrLock already set to %u, >>>>>> cannot re-provision device!\n", >>>>>> + __func__, config_desc_lock); >>>>>> + ret = -EINVAL; >>>>>> + goto out; >>>>>> + } >>>>>> + >>>>>> + for (i = 0; i < QUERY_DESC_CONFIGURATION_DEF_SIZE; i++) { >>>>>> + token = strsep(&strbuf, " "); >>>>>> + if (!token && i) >>>>>> + break; >>>>>> + >>>>>> + ret = kstrtoint(token, 0, &value); >>>>>> + if (ret) { >>>>>> + dev_err(hba->dev, "%s: kstrtoint failed %d %s\n", >>>>>> + __func__, ret, token); >>>>>> + ret = -EINVAL; >>>>>> + goto out; >>>>>> + } >>>>>> + desc_buf[i] = (u8)value; >>>>>> + } >>>>>> + >>>>>> + /* 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, &desc_buf_len); >>>>>> + >>>>>> + if (!ret) >>>>>> + dev_info(hba->dev, "%s: UFS Provisioning done, reboot now!\n", >>>>> That is not necessarily true - bConfDescContinue might have been used or >>>>> bConfigDescrLock might need to be set. Better just to say that >>>>> configuration descriptor 0 was written. >>>> Agree. Will update. >>>>>> + __func__); >>>>>> + >>>>>> +out: >>>>>> + kfree(strbuf_copy); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + 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 = 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_type = &ufscfg_type, >>>>>> + }, >>>>>> + }, >>>>>> +}; >>>>>> + >>>>>> +void ufshcd_configfs_init(struct ufs_hba *hba, const char *name) >>>>>> +{ >>>>>> + int ret; >>>>>> + struct configfs_subsystem *subsys = &hba->subsys; >>>>>> + >>>>>> + strncpy(ufscfg_subsys.su_group.cg_item.ci_namebuf, name, >>>>>> strlen(name)); >>>>>> + subsys->su_group = ufscfg_subsys.su_group; >>>>> configfs_subsystem cannot share su_group with other configfs_subsystem i.e >>>>> you are not allowing for there being more than 1 hba >>>> Agree. I think I will have to move above >>>> declarations(ufscfg_subsys/ufscfg_type/ufshcd_attrs) in driver .c/.h file >>>> (to make it per ufs hba/probe). >>>> Will check and update. >>>>>> + 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); >>>>>> +} >>>>>> + >>>>>> +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 e01cdc0..32a67b0 100644 >>>>>> --- a/drivers/scsi/ufs/ufshcd.c >>>>>> +++ b/drivers/scsi/ufs/ufshcd.c >>>>>> @@ -7711,6 +7711,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); >>>>>> @@ -7965,7 +7966,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, dev_name(hba->dev)); >>>>>> return 0; >>>>>> out_remove_scsi_host: >>>>>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >>>>>> index 45013b6..2275032 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_UFS_PROVISION >>>>>> + struct configfs_subsystem subsys; >>>>>> +#endif >>>>>> /* >>>>>> * This field is to keep a reference to "scsi_device" >>>>>> corresponding to >>>>>> * "UFS device" W-LU. >>>>>> @@ -868,6 +872,19 @@ 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 */ >>>>>> +#ifndef CONFIG_UFS_PROVISION >>>>>> +static inline void ufshcd_configfs_init(struct ufs_hba *hba, const char >>>>>> *name) >>>>>> +{ >>>>>> +} >>>>>> +static inline void ufshcd_configfs_exit(void) >>>>>> +{ >>>>>> +} >>>>>> +#else >>>>>> +void ufshcd_configfs_init(struct ufs_hba *hba, const char *name); >>>>>> +void ufshcd_configfs_exit(void); >>>>>> +#endif >>>>>> + >>>>>> int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn >>>>>> desc_id, >>>>>> int *desc_length); >>>>>> >>
| |