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

[Evan Green]

Hi Sayali,
On Mon, Jul 16, 2018 at 1:10 AM Sayali Lokhande <sayalil@xxxxxxxxxxxxxx> wrote:
>
> Hi Evan,
>
>
> On 7/9/2018 11:18 PM, Evan Green wrote:
> > Hi Sayali,
> > Thanks for the prompt spin.
> >
> > On Thu, Jul 5, 2018 at 11:21 PM Sayali Lokhande <sayalil@xxxxxxxxxxxxxx> wrote:
> >> This patch adds configfs support to provision ufs device at
> > s/ufs/UFS/
> Will update.
> >> runtime. This feature can be primarily useful in factory or
> >> assembly line as some devices may be required to be configured
> >> multiple times during initial system development phase.
> >> Configuration Descriptors can be written multiple times until
> >> bConfigDescrLock attribute is zero.
> >>
> >> Configuration descriptor buffer consists of Device and Unit
> >> descriptor configurable parameters which are parsed from vendor
> >> specific provisioning file and then passed via configfs node at
> >> runtime to provision ufs device.
> >> CONFIG_CONFIGFS_FS needs to be enabled for using this feature.
> >>
> >> Usage:
> >> 1) To read current configuration descriptor :
> >>     cat /config/ufshcd/ufs_provision
> >> 2) To provision ufs device:
> >>     echo <config_desc_buf> > /config/ufshcd/ufs_provision
> >>
> >> Signed-off-by: Sayali Lokhande <sayalil@xxxxxxxxxxxxxx>
> >> ---
> >>   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