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
