| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<DM6PR04MB65751DE4930E7198279F6FE6FC842@DM6PR04MB6575.namprd04.prod.outlook.com>
Date: Sun, 11 Aug 2024 06:19:49 +0000
From: Avri Altman <Avri.Altman@....com>
To: Manivannan Sadhasivam <manisadhasivam.linux@...il.com>
CC: "Martin K . Petersen" <martin.petersen@...cle.com>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Bart Van
Assche <bvanassche@....org>, Keoseong Park <keosung.park@...sung.com>
Subject: RE: [PATCH v3 2/2] scsi: ufs: Add HCI capabilities sysfs group
> On Fri, Aug 09, 2024 at 10:23:31AM +0300, Avri Altman wrote:
> > The standard register map of UFSHCI is comprised of several groups.
> > The first group (starting from offset 0x00), is the host capabilities group.
> > It contains some interesting information, that otherwise is not
> > available, e.g. the UFS version of the platform etc.
> >
> > Reviewed-by: Keoseong Park <keosung.park@...sung.com>
> > Reviewed-by: Bart Van Assche <bvanassche@....org>
> > Signed-off-by: Avri Altman <avri.altman@....com>
> > ---
> > Documentation/ABI/testing/sysfs-driver-ufs | 42 ++++++++++
> > drivers/ufs/core/ufs-sysfs.c | 95 ++++++++++++++++++++++
> > 2 files changed, 137 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
> > b/Documentation/ABI/testing/sysfs-driver-ufs
> > index fe943ce76c60..b6e0c3b806fd 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-ufs
> > +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> > @@ -1532,3 +1532,45 @@ Contact: Bean Huo <beanhuo@...ron.com>
> > Description:
> > rtc_update_ms indicates how often the host should synchronize or
> update the
> > UFS RTC. If set to 0, this will disable UFS RTC periodic update.
> > +
> > +What: /sys/devices/platform/.../ufshci_capabilities/capabilities
> > +Date: August 2024
> > +Contact: Avri Altman <avri.altman@....com>
> > +Description:
> > + Host Capabilities register group: host controller capabilities register.
> > + Symbol - CAP. Offset: 0x00 - 0x03.
>
> This doesn't look like an ABI description. You are merely specifying the register
> name and offset that gets accessed while reading this attribute.
>
> Also, I'm not sure if we really want to expose HCI/MCQ capabilities as sysfs ABI.
Why not?
Like the cover letter say, this info is otherwise unavailable.
> This just prints the hex value without even telling users how to interpret it.
Each register typically carries several parts of info encoded withing its 32 bits.
I am not sure that copying the spec into the documentation is useful.
>
> > +
> > +What: /sys/devices/platform/.../ufshci_capabilities/mcq_cap
> > +Date: August 2024
> > +Contact: Avri Altman <avri.altman@....com>
> > +Description:
> > + Host Capabilities register group: multi-circular queue capability
> register.
> > + Symbol - MCQCAP. Offset: 0x04 - 0x07.
> > +
> > +What: /sys/devices/platform/.../ufshci_capabilities/version
> > +Date: August 2024
> > +Contact: Avri Altman <avri.altman@....com>
> > +Description:
> > + Host Capabilities register group: UFS version register.
> > + Symbol - VER. Offset: 0x08 - 0x0B.
>
> This and below attributes are fine.
Still fail to see the difference.
>But the description should be changed. No
> need to put register name and offset here, that is not relevant to the ABI.
> Description should clearly state what is the purpose of the attribute, how to
> interpret (if necessary) and read/write capability. Like,
>
> ```
> This file shows the UFSHCD version.
>
> The file is read only.
> ```
>
> Applies to other attributes below.
OK. Thanks.
Avri
>
> - Mani
>
> > +
> > +What: /sys/devices/platform/.../ufshci_capabilities/ext_capabilities
> > +Date: August 2024
> > +Contact: Avri Altman <avri.altman@....com>
> > +Description:
> > + Host Capabilities register group: extended controller capabilities
> register.
> > + Symbol - EXT_CAP. Offset: 0x0C - 0x0F.
> > +
> > +What: /sys/devices/platform/.../ufshci_capabilities/product_id
> > +Date: August 2024
> > +Contact: Avri Altman <avri.altman@....com>
> > +Description:
> > + Host Capabilities register group: product ID register.
> > + Symbol - HCPID. Offset: 0x10 - 0x13.
> > +
> > +What: /sys/devices/platform/.../ufshci_capabilities/man_id
> > +Date: August 2024
> > +Contact: Avri Altman <avri.altman@....com>
> > +Description:
> > + Host Capabilities register group: manufacturer ID register.
> > + Symbol - HCMID. Offset: 0x14 - 0x17.
> > diff --git a/drivers/ufs/core/ufs-sysfs.c
> > b/drivers/ufs/core/ufs-sysfs.c index dec7746c98e0..751d5ff406da 100644
> > --- a/drivers/ufs/core/ufs-sysfs.c
> > +++ b/drivers/ufs/core/ufs-sysfs.c
> > @@ -525,6 +525,100 @@ static const struct attribute_group
> ufs_sysfs_capabilities_group = {
> > .attrs = ufs_sysfs_capabilities_attrs, };
> >
> > +static ssize_t capabilities_show(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "0x%x\n", hba->capabilities); }
> > +
> > +static ssize_t mcq_cap_show(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > + if (hba->ufs_version < ufshci_version(4, 0))
> > + return -EOPNOTSUPP;
> > +
> > + return sysfs_emit(buf, "0x%x\n", hba->mcq_capabilities); }
> > +
> > +static ssize_t version_show(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > + return sysfs_emit(buf, "0x%x\n", hba->ufs_version); }
> > +
> > +static ssize_t ext_capabilities_show(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + int ret;
> > + u32 val;
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > + if (hba->ufs_version < ufshci_version(4, 0))
> > + return -EOPNOTSUPP;
> > +
> > + ret = ufshcd_read_hci_reg(hba, &val,
> REG_EXT_CONTROLLER_CAPABILITIES);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "0x%x\n", val); }
> > +
> > +static ssize_t product_id_show(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + int ret;
> > + u32 val;
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > + ret = ufshcd_read_hci_reg(hba, &val, REG_CONTROLLER_PID);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "0x%x\n", val); }
> > +
> > +static ssize_t man_id_show(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + int ret;
> > + u32 val;
> > + struct ufs_hba *hba = dev_get_drvdata(dev);
> > +
> > + ret = ufshcd_read_hci_reg(hba, &val, REG_CONTROLLER_MID);
> > + if (ret)
> > + return ret;
> > +
> > + return sysfs_emit(buf, "0x%x\n", val); }
> > +
> > +static DEVICE_ATTR_RO(capabilities);
> > +static DEVICE_ATTR_RO(mcq_cap);
> > +static DEVICE_ATTR_RO(version);
> > +static DEVICE_ATTR_RO(ext_capabilities); static
> > +DEVICE_ATTR_RO(product_id); static DEVICE_ATTR_RO(man_id);
> > +
> > +static struct attribute *ufs_sysfs_ufshci_cap_attrs[] = {
> > + &dev_attr_capabilities.attr,
> > + &dev_attr_mcq_cap.attr,
> > + &dev_attr_version.attr,
> > + &dev_attr_ext_capabilities.attr,
> > + &dev_attr_product_id.attr,
> > + &dev_attr_man_id.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group ufs_sysfs_ufshci_group = {
> > + .name = "ufshci_capabilities",
> > + .attrs = ufs_sysfs_ufshci_cap_attrs, };
> > +
> > static ssize_t monitor_enable_show(struct device *dev,
> > struct device_attribute *attr, char
> > *buf) { @@ -1508,6 +1602,7 @@ static const struct attribute_group
> > ufs_sysfs_attributes_group = { static const struct attribute_group
> > *ufs_sysfs_groups[] = {
> > &ufs_sysfs_default_group,
> > &ufs_sysfs_capabilities_group,
> > + &ufs_sysfs_ufshci_group,
> > &ufs_sysfs_monitor_group,
> > &ufs_sysfs_power_info_group,
> > &ufs_sysfs_device_descriptor_group,
> > --
> > 2.25.1
> >
> >
>
> --
> மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists