ufs: core: print capabilities in controller's sysfs node

Commit Message

Daniil Lunev July 28, 2022, 4:49 a.m. UTC

Allows userspace to check supported by the controller/device
functionality, e.g. write booster.

Signed-off-by: Daniil Lunev <[e-mail]>

---

 drivers/ufs/core/ufs-sysfs.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Greg Kroah-Hartman July 28, 2022, 6:50 a.m. UTC | #1

On Thu, Jul 28, 2022 at 02:49:25PM +1000, Daniil Lunev wrote:
> Allows userspace to check supported by the controller/device
> functionality, e.g. write booster.
> 
> Signed-off-by: Daniil Lunev <[e-mail]>

You have to also document this in Documentation/ABI/ when you add a new
sysfs file.  Please do so here.

thanks,

greg k-h

Bart Van Assche July 28, 2022, 1:37 p.m. UTC | #2

On 7/27/22 21:49, Daniil Lunev wrote:
> +static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "0x%08llx\n", hba->caps);
> +}

This code change includes all of the UFSHCD_CAP_* constants in the 
kernel ABI. Is that really what we want? I'm wondering whether it 
perhaps would be better only to export those capabilities to user space 
that user space needs to know about.

Thanks,

Bart.

Daniil Lunev July 28, 2022, 11:08 p.m. UTC | #3

On Thu, Jul 28, 2022 at 11:37 PM Bart Van Assche <[e-mail]> wrote:
> This code change includes all of the UFSHCD_CAP_* constants in the
> kernel ABI. Is that really what we want? I'm wondering whether it
> perhaps would be better only to export those capabilities to user space
> that user space needs to know about.
Adding the filtering would introduce an extra maintenance burden and
will likely go out
of sync. I don't see harm in exposing all capabilities, both intrinsic
to the controller and
negotiated with the device. Do you see any scenario where that would be harmful?
--Daniil

Bart Van Assche July 28, 2022, 11:21 p.m. UTC | #4

On 7/28/22 16:08, Daniil Lunev wrote:
> On Thu, Jul 28, 2022 at 11:37 PM Bart Van Assche <[e-mail]> wrote:
>> This code change includes all of the UFSHCD_CAP_* constants in the
>> kernel ABI. Is that really what we want? I'm wondering whether it
>> perhaps would be better only to export those capabilities to user space
>> that user space needs to know about.
> Adding the filtering would introduce an extra maintenance burden and
> will likely go out
> of sync. I don't see harm in exposing all capabilities, both intrinsic
> to the controller and
> negotiated with the device. Do you see any scenario where that would be harmful?

Please only export what you need instead of exporting all capabilities.

Thanks,

Bart.

Patch

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 0a088b47d5570..b0c294c367519 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -254,6 +254,14 @@  static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
 	return res < 0 ? res : count;
 }
 
+static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "0x%08llx\n", hba->caps);
+}
+
 static DEVICE_ATTR_RW(rpm_lvl);
 static DEVICE_ATTR_RO(rpm_target_dev_state);
 static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -262,6 +270,7 @@  static DEVICE_ATTR_RO(spm_target_dev_state);
 static DEVICE_ATTR_RO(spm_target_link_state);
 static DEVICE_ATTR_RW(auto_hibern8);
 static DEVICE_ATTR_RW(wb_on);
+static DEVICE_ATTR_RO(caps);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -272,6 +281,7 @@  static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_spm_target_link_state.attr,
 	&dev_attr_auto_hibern8.attr,
 	&dev_attr_wb_on.attr,
+	&dev_attr_caps.attr,
 	NULL
 };