Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
On Sun, Feb 04, 2018 at 09:03:25AM +, Stanislav Nijnikov wrote:
> > -Original Message-
> > From: Bart Van Assche
> > Sent: Friday, February 2, 2018 6:32 PM
> > To: gre...@linuxfoundation.org
> > Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> > jaeg...@kernel.org; Alex Lemberg ; Stanislav
> > Nijnikov
> > Subject: Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
> >
> > On Fri, 2018-02-02 at 08:17 +0100, gre...@linuxfoundation.org wrote:
> > > On Fri, Feb 02, 2018 at 12:25:46AM +, Bart Van Assche wrote:
> > > > On Thu, 2018-02-01 at 18:15 +0200, Stanislav Nijnikov wrote:
> > > > > +enum ufs_desc_param_size {
> > > > > + UFS_PARAM_BYTE_SIZE = 1,
> > > > > + UFS_PARAM_WORD_SIZE = 2,
> > > > > + UFS_PARAM_DWORD_SIZE= 4,
> > > > > + UFS_PARAM_QWORD_SIZE= 8,
> > > > > +};
> > > >
> > > > Please do not copy bad naming choices from the Windows kernel into
> > > > the Linux kernel. Using names like WORD / DWORD / QWORD is much less
> > > > readable than using the numeric constants 2, 4, 8. Hence my proposal
> > > > to leave out the above enum completely.
> > >
> > > Are you sure those do not come from the spec itself? It's been a
> > > while since I last read it, but for some reason I remember those types
> > > of names being in there. But I might be confusing specs here.
> >
> > Hello Greg,
> >
> > That's a good question. However, a quick search on the Internet for the
> > search phrase "Universal Flash Storage" "qword" did not yield any results
> > about UFS in the first ten search hits. And I haven't found any references
> > to
> > the DWORD / QWORD terminology in the "UNIVERSAL FLASH STORAGE HOST
> > CONTROLLER INTERFACE (UFSHCI), UNIFIED MEMORY EXTENSION, Version
> > 1.1" document either. Maybe that means that I was looking at the wrong
> > document?
> >
> > Thanks,
> >
> > Bart.
> >
> >
> The UFS spec 2.1 specifies size as first letter in names of the descriptor
> parameters and attributes (e.g. bDeviceClass, wSpecVersion, dPSAMaxDataSize,
> qTotalRawDeviceCapacity, ...). But usage of the enum could be easily removed.
It matches the naming scheme of the spec, so in my opinion, it's fine
as-is. But as I'm not the author here, it's up to you what you want to
use, you have to maintain this, not me :)
thanks,
greg k-h
RE: [PATCH v4 02/10] ufs: sysfs: device descriptor
> -Original Message-
> From: Bart Van Assche
> Sent: Friday, February 2, 2018 6:32 PM
> To: gre...@linuxfoundation.org
> Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org;
> jaeg...@kernel.org; Alex Lemberg ; Stanislav
> Nijnikov
> Subject: Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
>
> On Fri, 2018-02-02 at 08:17 +0100, gre...@linuxfoundation.org wrote:
> > On Fri, Feb 02, 2018 at 12:25:46AM +, Bart Van Assche wrote:
> > > On Thu, 2018-02-01 at 18:15 +0200, Stanislav Nijnikov wrote:
> > > > +enum ufs_desc_param_size {
> > > > + UFS_PARAM_BYTE_SIZE = 1,
> > > > + UFS_PARAM_WORD_SIZE = 2,
> > > > + UFS_PARAM_DWORD_SIZE= 4,
> > > > + UFS_PARAM_QWORD_SIZE= 8,
> > > > +};
> > >
> > > Please do not copy bad naming choices from the Windows kernel into
> > > the Linux kernel. Using names like WORD / DWORD / QWORD is much less
> > > readable than using the numeric constants 2, 4, 8. Hence my proposal
> > > to leave out the above enum completely.
> >
> > Are you sure those do not come from the spec itself? It's been a
> > while since I last read it, but for some reason I remember those types
> > of names being in there. But I might be confusing specs here.
>
> Hello Greg,
>
> That's a good question. However, a quick search on the Internet for the
> search phrase "Universal Flash Storage" "qword" did not yield any results
> about UFS in the first ten search hits. And I haven't found any references to
> the DWORD / QWORD terminology in the "UNIVERSAL FLASH STORAGE HOST
> CONTROLLER INTERFACE (UFSHCI), UNIFIED MEMORY EXTENSION, Version
> 1.1" document either. Maybe that means that I was looking at the wrong
> document?
>
> Thanks,
>
> Bart.
>
>
The UFS spec 2.1 specifies size as first letter in names of the descriptor
parameters and attributes (e.g. bDeviceClass, wSpecVersion, dPSAMaxDataSize,
qTotalRawDeviceCapacity, ...). But usage of the enum could be easily removed.
Regards
Stanislav
Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
On Fri, 2018-02-02 at 08:17 +0100, gre...@linuxfoundation.org wrote:
> On Fri, Feb 02, 2018 at 12:25:46AM +, Bart Van Assche wrote:
> > On Thu, 2018-02-01 at 18:15 +0200, Stanislav Nijnikov wrote:
> > > +enum ufs_desc_param_size {
> > > + UFS_PARAM_BYTE_SIZE = 1,
> > > + UFS_PARAM_WORD_SIZE = 2,
> > > + UFS_PARAM_DWORD_SIZE= 4,
> > > + UFS_PARAM_QWORD_SIZE= 8,
> > > +};
> >
> > Please do not copy bad naming choices from the Windows kernel into the Linux
> > kernel. Using names like WORD / DWORD / QWORD is much less readable than
> > using
> > the numeric constants 2, 4, 8. Hence my proposal to leave out the above enum
> > completely.
>
> Are you sure those do not come from the spec itself? It's been a while
> since I last read it, but for some reason I remember those types of
> names being in there. But I might be confusing specs here.
Hello Greg,
That's a good question. However, a quick search on the Internet for the search
phrase "Universal Flash Storage" "qword" did not yield any results about UFS in
the first ten search hits. And I haven't found any references to the DWORD /
QWORD terminology in the "UNIVERSAL FLASH STORAGE HOST CONTROLLER INTERFACE
(UFSHCI), UNIFIED MEMORY EXTENSION, Version 1.1" document either. Maybe that
means that I was looking at the wrong document?
Thanks,
Bart.
Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
On Fri, Feb 02, 2018 at 12:25:46AM +, Bart Van Assche wrote:
> On Thu, 2018-02-01 at 18:15 +0200, Stanislav Nijnikov wrote:
> > +enum ufs_desc_param_size {
> > + UFS_PARAM_BYTE_SIZE = 1,
> > + UFS_PARAM_WORD_SIZE = 2,
> > + UFS_PARAM_DWORD_SIZE= 4,
> > + UFS_PARAM_QWORD_SIZE= 8,
> > +};
>
> Please do not copy bad naming choices from the Windows kernel into the Linux
> kernel. Using names like WORD / DWORD / QWORD is much less readable than using
> the numeric constants 2, 4, 8. Hence my proposal to leave out the above enum
> completely.
Are you sure those do not come from the spec itself? It's been a while
since I last read it, but for some reason I remember those types of
names being in there. But I might be confusing specs here.
thanks,
greg k-h
Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
On Thu, 2018-02-01 at 18:15 +0200, Stanislav Nijnikov wrote:
> +ssize_t ufs_sysfs_read_desc_param(struct ufs_hba *hba,
> + enum desc_idn desc_id,
> + u8 desc_index,
> + u8 param_offset,
> + u8 *sysfs_buf,
> + u8 param_size)
> +{
> + u8 desc_buf[UFS_PARAM_QWORD_SIZE] = {0};
> + int ret;
> +
> + if (param_size > UFS_PARAM_QWORD_SIZE)
> + return -EINVAL;
> +
> + ret = ufshcd_read_desc_param(hba, desc_id, desc_index,
> + param_offset, desc_buf, param_size);
> + if (ret)
> + return -EINVAL;
> + switch (param_size) {
> + case UFS_PARAM_BYTE_SIZE:
> + ret = sprintf(sysfs_buf, "0x%02X\n", *desc_buf);
> + break;
> + case UFS_PARAM_WORD_SIZE:
> + ret = sprintf(sysfs_buf, "0x%04X\n",
> + be16_to_cpu(*((u16 *)desc_buf)));
> + break;
> + case UFS_PARAM_DWORD_SIZE:
> + ret = sprintf(sysfs_buf, "0x%08X\n",
> + be32_to_cpu(*((u32 *)desc_buf)));
> + break;
> + case UFS_PARAM_QWORD_SIZE:
> + ret = sprintf(sysfs_buf, "0x%016llX\n",
> + be64_to_cpu(*((u64 *)desc_buf)));
> + break;
> + }
> +
> + return ret;
> +}
Seeing code like this makes me wonder whether this patch series has been
verified
with sparse? I think sparse will complain about all three be*_to_cpu() casts
above.
Please use get_unaligned_be*() instead of open-coding these functions.
Thanks,
Bart.
Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
On Thu, 2018-02-01 at 18:15 +0200, Stanislav Nijnikov wrote:
> +enum ufs_desc_param_size {
> + UFS_PARAM_BYTE_SIZE = 1,
> + UFS_PARAM_WORD_SIZE = 2,
> + UFS_PARAM_DWORD_SIZE= 4,
> + UFS_PARAM_QWORD_SIZE= 8,
> +};
Please do not copy bad naming choices from the Windows kernel into the Linux
kernel. Using names like WORD / DWORD / QWORD is much less readable than using
the numeric constants 2, 4, 8. Hence my proposal to leave out the above enum
completely.
Thanks,
Bart.
Re: [PATCH v4 02/10] ufs: sysfs: device descriptor
On Thu, Feb 01, 2018 at 06:15:38PM +0200, Stanislav Nijnikov wrote:
> +#define UFS_DESC_PARAM(_name, _puname, _duname, _size)
> \
> +static ssize_t _name##_show(struct device *dev,
> \
> + struct device_attribute *attr, char *buf) \
> +{
> \
> + struct ufs_hba *hba = dev_get_drvdata(dev); \
> + return ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname, \
> + 0, _duname##_DESC_PARAM##_puname, \
> + buf, UFS_PARAM_##_size##_SIZE); \
> +}
> \
> +static DEVICE_ATTR_RO(_name)
Nit, use tabs in your lines here to line up the trailing \
Same for other places in this patch series.
thanks,
greg k-h
[PATCH v4 02/10] ufs: sysfs: device descriptor
This patch introduces a sysfs group entry for the UFS device descriptor parameters. The group adds "device_descriptor" folder under the UFS driver sysfs entry (/sys/bus/platform/drivers/ufshcd/*). The parameters are shown as hexadecimal numbers. The full information about the parameters could be found at UFS specifications 2.1. Signed-off-by: Stanislav Nijnikov --- Documentation/ABI/testing/sysfs-driver-ufs | 223 + drivers/scsi/ufs/ufs-sysfs.c | 123 drivers/scsi/ufs/ufs.h | 8 ++ drivers/scsi/ufs/ufshcd.c | 12 +- drivers/scsi/ufs/ufshcd.h | 6 + 5 files changed, 366 insertions(+), 6 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-driver-ufs diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs new file mode 100644 index 000..8da7b84 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-driver-ufs @@ -0,0 +1,223 @@ +What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/device_type +Date: February 2018 +Contact: Stanislav Nijnikov +Description: This file shows the device type. This is one of the UFS + device descriptor parameters. The full information about + the descriptor could be found at UFS specifications 2.1. + The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/device_class +Date: February 2018 +Contact: Stanislav Nijnikov +Description: This file shows the device class. This is one of the UFS + device descriptor parameters. The full information about + the descriptor could be found at UFS specifications 2.1. + The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/device_sub_class +Date: February 2018 +Contact: Stanislav Nijnikov +Description: This file shows the UFS storage subclass. This is one of + the UFS device descriptor parameters. The full information + about the descriptor could be found at UFS specifications 2.1. + The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/protocol +Date: February 2018 +Contact: Stanislav Nijnikov +Description: This file shows the protocol supported by an UFS device. + This is one of the UFS device descriptor parameters. + The full information about the descriptor could be found + at UFS specifications 2.1. + The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/number_of_luns +Date: February 2018 +Contact: Stanislav Nijnikov +Description: This file shows number of logical units. This is one of + the UFS device descriptor parameters. The full information + about the descriptor could be found at UFS specifications 2.1. + The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/number_of_wluns +Date: February 2018 +Contact: Stanislav Nijnikov +Description: This file shows number of well known logical units. + This is one of the UFS device descriptor parameters. + The full information about the descriptor could be found + at UFS specifications 2.1. + The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/boot_enable +Date: February 2018 +Contact: Stanislav Nijnikov +Description: This file shows value that indicates whether the device is + enabled for boot. This is one of the UFS device descriptor + parameters. The full information about the descriptor could + be found at UFS specifications 2.1. + The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/descriptor_access_enable +Date: February 2018 +Contact: Stanislav Nijnikov +Description: This file shows value that indicates whether the device + descriptor could be read after partial initialization phase + of the boot sequence. This is one of the UFS device descriptor + parameters. The full information about the descriptor could + be found at UFS specifications 2.1. + The file is read only. + +What: /sys/bus/platform/drivers/ufshcd/*/device_descriptor/initial_power_mode +Date: February 2018 +Contact: Stanislav Nijnikov +Description: This file shows value that defines the power mode after + device initialization or hardware reset. This is one of + the UFS device descriptor parameters. The full information + about the descriptor coul
