Message-ID: <4d3746bd-0b93-e2f5-8c4b-41cf16633c9c@codeaurora.org>
Date:   Wed, 4 Jul 2018 19:41:56 +0530
From:   Sayali Lokhande <sayalil@...eaurora.org>
To:     Evan Green <evgreen@...omium.org>
Cc:     subhashj@...eaurora.org, cang@...eaurora.org,
        vivek.gautam@...eaurora.org,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Vinayak Holikatti <vinholikatti@...il.com>,
        jejb@...ux.vnet.ibm.com, martin.petersen@...cle.com,
        asutoshd@...eaurora.org, riteshh@...eaurora.org,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4 2/3] scsi: ufs: Add ufs provisioning support

Hi Evan,


On 7/3/2018 5:12 AM, Evan Green wrote:
> Hi Sayali,
>
> On Mon, Jun 25, 2018 at 9:04 PM Sayali Lokhande <sayalil@...eaurora.org> wrote:
>> A new api ufshcd_do_config_device() is added in driver
>> to support UFS provisioning at runtime. Configfs support
>> is added to trigger provisioning.
>> Device and Unit descriptor configurable parameters are
>> parsed from vendor specific provisioning data or file and
>> passed via configfs node at runtime to provision ufs device.
>>
>> Signed-off-by: Sayali Lokhande <sayalil@...eaurora.org>
>> ---
>>   drivers/scsi/ufs/ufs.h    |  28 +++++++
>>   drivers/scsi/ufs/ufshcd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/scsi/ufs/ufshcd.h |   2 +
>>   3 files changed, 228 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index e15deb0..1f99904 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -333,6 +333,7 @@ enum {
>>          UFSHCD_AMP              = 3,
>>   };
>>
>> +#define UFS_BLOCK_SIZE 4096
> Is this not prescribed by UFS. You need to look at bMinAddrBlockSize,
> whose minimum value is 8, representing 4k blocks. But you can't assume
> they're all going to be that, the spec allows for larger block sizes.
Will get rid of it and also remove all the calculations that used it.
>
>>   #define POWER_DESC_MAX_SIZE                    0x62
>>   #define POWER_DESC_MAX_ACTV_ICC_LVLS           16
>>
>> @@ -425,6 +426,33 @@ enum {
>>          MASK_TM_SERVICE_RESP            = 0xFF,
>>   };
>>
>> +struct ufs_unit_desc {
>> +       u8     bLUEnable;              /* 1 for enabled LU */
>> +       u8     bBootLunID;             /* 0 for using this LU for boot */
>> +       u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
>> +       u8     bMemoryType;            /* 0 for enhanced memory type */
>> +       u32    dNumAllocUnits;         /* Number of alloc unit for this LU */
>> +       u8     bDataReliability;       /* 0 for reliable write support */
>> +       u8     bLogicalBlockSize;      /* See section 13.2.3 of UFS standard */
>> +       u8     bProvisioningType;      /* 0 for thin provisioning */
>> +       u16    wContextCapabilities;   /* refer Unit Descriptor Description */
>> +};
>> +
>> +struct ufs_config_descr {
>> +       u8     bNumberLU;              /* Total number of active LUs */
>> +       u8     bBootEnable;            /* enabling device for partial init */
>> +       u8     bDescrAccessEn;         /* desc access during partial init */
>> +       u8     bInitPowerMode;         /* Initial device power mode */
>> +       u8     bHighPriorityLUN;       /* LUN of the high priority LU */
>> +       u8     bSecureRemovalType;     /* Erase config for data removal */
>> +       u8     bInitActiveICCLevel;    /* ICC level after reset */
>> +       u16    wPeriodicRTCUpdate;     /* 0 to set a priodic RTC update rate */
>> +       u32     bConfigDescrLock;      /* 1 to lock Configation Descriptor */
>> +       u32    qVendorConfigCode;      /* Vendor specific configuration code */
>> +       struct ufs_unit_desc unit[8];
>> +       u8      lun_to_grow;
>> +};
> You've created a structure whose naming and members suggest that it
> matches the hardware, but it does not. I think you should make this
> structure match the hardware, and remove software members like
> lun_to_grow. It's okay with me if 8 is hardcoded for now, but bear in
> mind I think the spec allows for this to be a different number.
Will get rid of these structs.
>> +
>>   /* Task management service response */
>>   enum {
>>          UPIU_TASK_MANAGEMENT_FUNC_COMPL         = 0x00,
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 351f874..370a7c6 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -42,6 +42,7 @@
>>   #include <linux/nls.h>
>>   #include <linux/of.h>
>>   #include <linux/bitfield.h>
>> +#include <asm/unaligned.h>
> Alphabetize includes, though I think if you follow my other
> suggestions this will go away.
Agreed.
>>   #include "ufshcd.h"
>>   #include "ufs_quirks.h"
>>   #include "unipro.h"
>> @@ -3063,6 +3064,14 @@ static inline int ufshcd_read_power_desc(struct ufs_hba *hba,
>>          return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);
>>   }
>>
>> +static inline int ufshcd_read_geometry_desc(struct ufs_hba *hba,
>> +                                        u8 *buf,
>> +                                        u32 size)
>> +{
>> +       return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
>> +}
>> +
>> +
> Remove extra blank line.
>
>>   static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
>>   {
>>          return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
>> @@ -6363,6 +6372,195 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba)
>>   }
>>
>>   /**
>> + * ufshcd_do_config_device - API function for UFS provisioning
>> + * hba: per-adapter instance
>> + * Returns 0 for success, non-zero in case of failure.
>> + */
>> +int ufshcd_do_config_device(struct ufs_hba *hba)
>> +{
>> +       struct ufs_config_descr *cfg = &hba->cfgs;
>> +       int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
>> +       u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
>> +       int i, ret = 0;
>> +       int lun_to_grow = -1;
>> +       u64 qTotalRawDeviceCapacity;
>> +       u16 wEnhanced1CapAdjFac, wEnhanced2CapAdjFac;
>> +       u32 dEnhanced1MaxNAllocU, dEnhanced2MaxNAllocU;
>> +       size_t alloc_units, units_to_create = 0;
>> +       size_t capacity_to_alloc_factor;
>> +       size_t enhanced1_units = 0, enhanced2_units = 0;
>> +       size_t conversion_ratio = 1;
>> +       u8 *pt;
>> +       u32 blocks_per_alloc_unit = 1024;
> Is this really hardcoded by the spec? I think it probably isn't, but
> if it is, make it a define.
Will get rid of these calculations here.
>> +       int geo_len = hba->desc_size.geom_desc;
>> +       u8 geo_buf[hba->desc_size.geom_desc];
>> +       unsigned int max_partitions = 8;
> Magic value. Perhaps a define here as well.
Will get rid of these calculations here.
>> +
>> +       WARN_ON(!hba || !cfg);
>> +
>> +       pm_runtime_get_sync(hba->dev);
>> +       scsi_block_requests(hba->host);
> Is this needed? Why is it bad to have requests going through during
> this function, given that you re-enable them at the end of this
> function?
>
>> +       if (ufshcd_wait_for_doorbell_clr(hba, U64_MAX)) {
> Why is this needed?
This is just to ensure stable operation while doing runtime provisioning.
>> +               dev_err(hba->dev, "%s: Timed out waitig for DB to clear\n",
>> +                       __func__);
>> +               ret = -EBUSY;
>> +               goto out;
>> +       }
>> +
>> +       ret = ufshcd_read_geometry_desc(hba, geo_buf, geo_len);
>> +       if (ret) {
>> +               dev_err(hba->dev, "%s: Failed getting geometry_desc %d\n",
>> +                       __func__, ret);
>> +               goto out;
>> +       }
>> +
>> +       /*
>> +        * Get Geomtric parameters like total configurable memory
>> +        * quantity (Offset 0x04 to 0x0b), Capacity Adjustment
>> +        * Factors (Offset 0x30, 0x31, 0x36, 0x37), Max allocation
>> +        * units (Offset 0x2c to 0x2f, 0x32 to 0x35) used to configure
>> +        * the device logical units.
>> +        */
>> +       qTotalRawDeviceCapacity = get_unaligned_be64(&geo_buf[0x04]);
>> +       wEnhanced1CapAdjFac = get_unaligned_be16(&geo_buf[0x30]);
>> +       wEnhanced2CapAdjFac = get_unaligned_be16(&geo_buf[0x36]);
>> +       dEnhanced1MaxNAllocU = get_unaligned_be32(&geo_buf[0x2c]);
>> +       dEnhanced2MaxNAllocU = get_unaligned_be32(&geo_buf[0x32]);
> Magic values, these should be done as other descriptors do it (see
> enum device_desc_param).
Will get rid of these calculations here.
>> +
>> +       capacity_to_alloc_factor =
>> +               (blocks_per_alloc_unit * UFS_BLOCK_SIZE) / 512;
>> +
>> +       if (qTotalRawDeviceCapacity % capacity_to_alloc_factor != 0) {
>> +               dev_err(hba->dev,
>> +                       "%s: Raw capacity(%llu) not multiple of alloc factor(%zu)\n",
>> +                       __func__, qTotalRawDeviceCapacity,
>> +                       capacity_to_alloc_factor);
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>> +       alloc_units = (qTotalRawDeviceCapacity / capacity_to_alloc_factor);
>> +       units_to_create = 0;
>> +       enhanced1_units = enhanced2_units = 0;
>> +
>> +       /*
>> +        * Calculate number of allocation units to be assigned to a logical unit
>> +        * considering the capacity adjustment factor of respective memory type.
>> +        */
>> +       for (i = 0; i < (max_partitions - 1) &&
>> +               units_to_create <= alloc_units; i++) {
>> +               if ((cfg->unit[i].dNumAllocUnits % blocks_per_alloc_unit) == 0)
>> +                       cfg->unit[i].dNumAllocUnits /= blocks_per_alloc_unit;
>> +               else
>> +                       cfg->unit[i].dNumAllocUnits =
>> +                       cfg->unit[i].dNumAllocUnits / blocks_per_alloc_unit + 1;
>> +
>> +               if (cfg->unit[i].bMemoryType == 0)
>> +                       units_to_create += cfg->unit[i].dNumAllocUnits;
>> +               else if (cfg->unit[i].bMemoryType == 3) {
>> +                       enhanced1_units += cfg->unit[i].dNumAllocUnits;
>> +                       cfg->unit[i].dNumAllocUnits *=
>> +                               (wEnhanced1CapAdjFac / 0x100);
>> +                       units_to_create += cfg->unit[i].dNumAllocUnits;
>> +               } else if (cfg->unit[i].bMemoryType == 4) {
>> +                       enhanced2_units += cfg->unit[i].dNumAllocUnits;
>> +                       cfg->unit[i].dNumAllocUnits *=
>> +                               (wEnhanced1CapAdjFac / 0x100);
>> +                       units_to_create += cfg->unit[i].dNumAllocUnits;
>> +               } else {
>> +                       dev_err(hba->dev, "%s: Unsupported memory type %d\n",
>> +                               __func__, cfg->unit[i].bMemoryType);
>> +                       ret = -EINVAL;
>> +                       goto out;
>> +               }
>> +       }
>> +       if (enhanced1_units > dEnhanced1MaxNAllocU) {
>> +               dev_err(hba->dev, "%s: size %zu exceeds max enhanced1 area size %u\n",
>> +                       __func__, enhanced1_units, dEnhanced1MaxNAllocU);
>> +               ret = -ERANGE;
>> +               goto out;
>> +       }
>> +       if (enhanced2_units > dEnhanced2MaxNAllocU) {
>> +               dev_err(hba->dev, "%s: size %zu exceeds max enhanced2 area size %u\n",
>> +                       __func__, enhanced2_units, dEnhanced2MaxNAllocU);
>> +               ret = -ERANGE;
>> +               goto out;
>> +       }
>> +       if (units_to_create > alloc_units) {
>> +               dev_err(hba->dev, "%s: Specified size %zu exceeds device size %zu\n",
>> +                       __func__, units_to_create, alloc_units);
>> +               ret = -ERANGE;
>> +               goto out;
>> +       }
>> +       lun_to_grow = cfg->lun_to_grow;
>> +       if (lun_to_grow != -1) {
>> +               if (cfg->unit[i].bMemoryType == 0)
>> +                       conversion_ratio = 1;
>> +               else if (cfg->unit[i].bMemoryType == 3)
>> +                       conversion_ratio = (wEnhanced1CapAdjFac / 0x100);
>> +               else if (cfg->unit[i].bMemoryType == 4)
>> +                       conversion_ratio = (wEnhanced2CapAdjFac / 0x100);
>> +
>> +               cfg->unit[lun_to_grow].dNumAllocUnits +=
>> +               ((alloc_units - units_to_create) / conversion_ratio);
>> +               dev_dbg(hba->dev, "%s: conversion_ratio %zu for lun %d\n",
>> +                       __func__, conversion_ratio, i);
>> +       }
> I really don't like all this wizardry in here. I think the kernel
> should just accept config descriptor bytes through configfs that it
> more or less passes along. These calculations could all be done in
> user mode.
Agreed. Will get rid of these calculations here. We can just accept 
configuaration descriptor params (as in spec)
Calculations (if required) can be done beforehand (in user mode).
>> +
>> +       /* Fill in the buffer with configuration data */
>> +       pt = desc_buf;
>> +       *pt++ = 0x90;        // bLength
>> +       *pt++ = 0x01;        // bDescriptorType
>> +       *pt++ = 0;           // Reserved in UFS2.0 and onward
>> +       *pt++ = cfg->bBootEnable;
>> +       *pt++ = cfg->bDescrAccessEn;
>> +       *pt++ = cfg->bInitPowerMode;
>> +       *pt++ = cfg->bHighPriorityLUN;
>> +       *pt++ = cfg->bSecureRemovalType;
>> +       *pt++ = cfg->bInitActiveICCLevel;
>> +       put_unaligned_be16(cfg->wPeriodicRTCUpdate, pt);
>> +       pt = pt + 7; // Reserved fields set to 0
>> +
>> +       /* Fill in the buffer with per logical unit data */
>> +       for (i = 0; i < UFS_UPIU_MAX_GENERAL_LUN; i++) {
>> +               *pt++ = cfg->unit[i].bLUEnable;
>> +               *pt++ = cfg->unit[i].bBootLunID;
>> +               *pt++ = cfg->unit[i].bLUWriteProtect;
>> +               *pt++ = cfg->unit[i].bMemoryType;
>> +               put_unaligned_be32(cfg->unit[i].dNumAllocUnits, pt);
>> +               pt = pt + 4;
>> +               *pt++ = cfg->unit[i].bDataReliability;
>> +               *pt++ = cfg->unit[i].bLogicalBlockSize;
>> +               *pt++ = cfg->unit[i].bProvisioningType;
>> +               put_unaligned_be16(cfg->unit[i].wContextCapabilities, pt);
>> +               pt = pt + 5; // Reserved fields set to 0
>> +       }
> Yikes. If your structure reflected the hardware, then you could use
> actual members. Barring that, you could create and use enum values for
> the descriptor members. The way it stands this is very brittle and
> full of magic values and offsets.
Will get rid of this code here.
>> +
>> +       ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
>> +               QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
> I'm not sure this works out of the box, especially if what you're
> writing isn't exactly the descriptor size (which it might be in your
> tests, but might suddenly start failing later). Please consider
> absorbing my change [1] which properly refactors the function for
> reading and writing.
I know the desc size (saved in buff_len). And via configfs we are 
passing same size of configuration descriptor.
So it works fine.
>
>> +
>> +       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);
>> +               goto out;
>> +       }
>> +
>> +       if (cfg->bConfigDescrLock == 1) {
>> +               ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> +               QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
>> +               if (ret)
>> +                       dev_err(hba->dev, "%s: Failed writing bConfigDescrLock %d\n",
>> +                               __func__, ret);
>> +       }
> It might be better not to merge this functionality in with the act of
> writing provisioning data, and instead make the sysfs attributes
> writable like [2].
Will remove writing decriptor lock here (as it is not a part of 
configuration descriptor anyway and also we wont be passing it via 
configfs).

>
>> +
>> +out:
>> +       scsi_unblock_requests(hba->host);
>> +       pm_runtime_put_sync(hba->dev);
>> +
>> +       return ret;
>> +}
>> +
>> +/**
>>    * ufshcd_probe_hba - probe hba to detect device and initialize
>>    * @hba: per-adapter instance
>>    *
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 101a75c..0e6bf30 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -549,6 +549,7 @@ struct ufs_hba {
>>          unsigned int irq;
>>          bool is_irq_enabled;
>>          u32 dev_ref_clk_freq;
>> +       struct ufs_config_descr cfgs;
> How come you store this here, rather than as a local in
> ufshcd_do_config_device()?
Will remove this entry.
>>          /* Interrupt aggregation support is broken */
>>          #define UFSHCD_QUIRK_BROKEN_INTR_AGGR                   0x1
>> @@ -867,6 +868,7 @@ 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);
>> +int ufshcd_do_config_device(struct ufs_hba *hba);
>>
>>   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
>>
> [1] https://patchwork.kernel.org/patch/10467669/
> [2] https://patchwork.kernel.org/patch/10467665/
Thanks,
Sayali

Powered by blists - more mailing lists