Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org D7A9160388
Subject: Re: [PATCH V10 2/2] scsi: ufs: Add configfs support for UFS
 provisioning
To:     Adrian Hunter <adrian.hunter@intel.com>, subhashj@codeaurora.org,
        cang@codeaurora.org, vivek.gautam@codeaurora.org,
        rnayak@codeaurora.org, vinholikatti@gmail.com,
        jejb@linux.vnet.ibm.com, martin.petersen@oracle.com,
        asutoshd@codeaurora.org, evgreen@chromium.org,
        riteshh@codeaurora.org
Cc:     linux-scsi@vger.kernel.org,
        open list <linux-kernel@vger.kernel.org>
References: <1535088810-20989-1-git-send-email-sayalil@codeaurora.org>
 <1535088810-20989-3-git-send-email-sayalil@codeaurora.org>
 <5d2a6dea-e524-fca7-3c4b-a45bc9e6516a@intel.com>
 <8ee0372b-d930-208d-520b-5a38642a2844@codeaurora.org>
 <a8405e52-ab28-f7ad-e150-4a633d0ad4bc@intel.com>
 <49d1be22-b85d-abfb-3b4e-d8b4b7176d14@codeaurora.org>
 <e68f1f7c-4692-744c-6049-abd3061f398b@intel.com>
From:   Sayali Lokhande <sayalil@codeaurora.org>
Message-ID: <5ea4a7bc-28df-b485-4e2d-c0ab59a8facf@codeaurora.org>
Date:   Tue, 28 Aug 2018 18:08:01 +0530
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <e68f1f7c-4692-744c-6049-abd3061f398b@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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);
>>>>>>    
>>