RE: [PATCH RFC 0/3] Add ufs provisioning support in driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Alex,

Thanks for your comments.
Yes, I will check the feasibility of using existing sysfs infrastructure and making it writable for configuring provisioning data.
Will update my patches accordingly.

Thanks,
Sayali
-----Original Message-----
From: Alex Lemberg [mailto:Alex.Lemberg@xxxxxxx] 
Sent: Sunday, May 27, 2018 8:55 PM
To: sayali <sayalil@xxxxxxxxxxxxxx>; Stanislav Nijnikov <Stanislav.Nijnikov@xxxxxxx>; subhashj@xxxxxxxxxxxxxx; cang@xxxxxxxxxxxxxx; vivek.gautam@xxxxxxxxxxxxxx; rnayak@xxxxxxxxxxxxxx; vinholikatti@xxxxxxxxx; jejb@xxxxxxxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx; asutoshd@xxxxxxxxxxxxxx; Avri Altman <Avri.Altman@xxxxxxx>
Cc: linux-scsi@xxxxxxxxxxxxxxx
Subject: Re: [PATCH RFC 0/3] Add ufs provisioning support in driver

Hi Sayali,

Same as Stanislav, I also think you should use the existing sysfs entries as much as possible rather than defining a new ones. 
You are right, the existing sysfs infrastructure is read only, but we shouldn't create a new ones just for making them writable.

In general, I believe we need to consider adding more generic infrastructure for all UFS device management functionality, and try to prevent implementing them one by one separately - this could cause a lot of mess in UFS driver code. 

Thanks,
Alex
 
On 5/25/18, 5:09 PM, "linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of sayali" <linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of sayalil@xxxxxxxxxxxxxx> wrote:

    Hi Stanislav,
    
    Thank you for the comments.
    
    Please check my answers below:
    1. New descriptors structures are redefinition of the descriptors info (see
    definitions of  device_desc_param, unit_desc_param, etc in
    drivers/scsi/ufs/ufs.h)
    [Sayali] I checked enum device_desc_param, unit_desc_param in ufs.h. These
    enum's are actually defining parameter offsets (not values) for device/unit
    descriptors.
    What we want is to assign/get certain value for each configurable parameter
    and then write it to Configuration descriptor.
    The new descriptors structures and its members added in my patches are meant
    for Configuration descriptor (which is combination of configurable
    device/unit desc parameters)
    New structs added:
    ufs_unit_desc ==> correspond to Table 14-12 - Unit Descriptor configurable
    parameters (from specs)
    ufs_config_descr ==>  correspond to Table 14-10 - Configuration Descr.
    Header and Device Descr. Conf. parameters (INDEX = 00h)
    
    These structs are required and will contain the actual values (parsed via
    sysfs buffer) of configurable parameters of device/unit descriptors.
    
    2. You didn't took into consideration the recently added UFS sysfs
    infostructure.
    [Sayali] Do you mean below commits ?
    commit d829fc8a1058851f1058b4a29ea02da125c1684a  (scsi: ufs: sysfs: unit
    descriptor)
    commit 45bced87e79316ecd868aee8f187284025792c5f  (scsi: ufs: sysfs: device
    descriptor)
    As per my understanding, these are read only interface to show unit/device
    descriptors/parameters via sysfs.
    There is no sysfs api to write configurable parameters of unit/device desc.
    Please correct me if I am missing something here.
    
    3. The idea of passing the binary buffer as a string is a bit flawed and the
    parser doesn't provide any protection against bad data (e.g just send the
    string "100").
    [Sayali] Read only interface is already present upstream to get descriptor
    data from device (as you pointed above)
    In similar lines, I thought of using sysfs write interface to provision UFS
    device.
    
    In my current implementation and testing , the buffer (being passed via
    sysfs) is obtained using a python script.
    The script provides sanitized buffer after parsing vendor specific
    provisioning data/file. 
    Still I agree I should check and add more protection against bad data
    handling in my code. Will update this is my next patch set.
    
    Regards,
    Sayali
    
    -----Original Message-----
    From: Stanislav Nijnikov [mailto:Stanislav.Nijnikov@xxxxxxx] 
    Sent: Thursday, May 24, 2018 5:56 PM
    To: Sayali Lokhande <sayalil@xxxxxxxxxxxxxx>; subhashj@xxxxxxxxxxxxxx;
    cang@xxxxxxxxxxxxxx; vivek.gautam@xxxxxxxxxxxxxx; rnayak@xxxxxxxxxxxxxx;
    vinholikatti@xxxxxxxxx; jejb@xxxxxxxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx;
    asutoshd@xxxxxxxxxxxxxx
    Cc: linux-scsi@xxxxxxxxxxxxxxx
    Subject: RE: [PATCH RFC 0/3] Add ufs provisioning support in driver
    
    Hi Sayali,
    I have several notes about the patches:
    1. New descriptors structures are redefinition of the descriptors info (see
    definitions of  device_desc_param, unit_desc_param, etc in
    drivers/scsi/ufs/ufs.h) 2. You didn't took into consideration the recently
    added UFS sysfs infostructure.
    3. The idea of passing the binary buffer as a string is a bit flawed and the
    parser doesn't provide any protection against bad data (e.g just send the
    string "100").
    
    Regards
    Stanislav
    
    > -----Original Message-----
    > From: linux-scsi-owner@xxxxxxxxxxxxxxx 
    > <linux-scsi-owner@xxxxxxxxxxxxxxx> On Behalf Of Sayali Lokhande
    > Sent: Tuesday, May 22, 2018 7:22 AM
    > To: subhashj@xxxxxxxxxxxxxx; cang@xxxxxxxxxxxxxx; 
    > vivek.gautam@xxxxxxxxxxxxxx; rnayak@xxxxxxxxxxxxxx; 
    > vinholikatti@xxxxxxxxx; jejb@xxxxxxxxxxxxxxxxxx; 
    > martin.petersen@xxxxxxxxxx; asutoshd@xxxxxxxxxxxxxx
    > Cc: linux-scsi@xxxxxxxxxxxxxxx; Sayali Lokhande 
    > <sayalil@xxxxxxxxxxxxxx>
    > Subject: [PATCH RFC 0/3] Add ufs provisioning support in driver
    > 
    > This change adds a new API ufshcd_do_config_device() to write 
    > configuration descriptor with the provisioning data.
    > Sysfs support is added in driver to trigger ufs provisioning at 
    > runtime. Provisioning data is parsed from vendor specific provisioning 
    > file. This parsed data is passed as a buffer via sysfs to provision 
    > ufs device.
    > 
    > Sayali Lokhande (2):
    >   scsi: ufs: Add ufs provisioning support
    >   scsi: ufs: Add sysfs support for ufs provision
    > 
    > Subhash Jadavani (1):
    >   scsi: ufs: set the device reference clock setting
    > 
    >  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      |   8 +
    >  drivers/scsi/ufs/ufs.h                             |  39 +++
    >  drivers/scsi/ufs/ufshcd-pltfrm.c                   |  20 ++
    >  drivers/scsi/ufs/ufshcd.c                          | 383
    +++++++++++++++++++++
    >  drivers/scsi/ufs/ufshcd.h                          |   9 +
    >  5 files changed, 459 insertions(+)
    > 
    > --
    > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
    > Forum, a Linux Foundation Collaborative Project
    
    
    






[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux