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
