| Message ID | 20220728144710.1.Id612b86fd30936dfd4c456b3341547c15cecf321@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ufs: core: print capabilities in controller's sysfs node | expand |
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 <dlunev@chromium.org> You have to also document this in Documentation/ABI/ when you add a new sysfs file. Please do so here. thanks, greg k-h
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.
On Thu, Jul 28, 2022 at 11:37 PM Bart Van Assche <bvanassche@acm.org> 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
On 7/28/22 16:08, Daniil Lunev wrote: > On Thu, Jul 28, 2022 at 11:37 PM Bart Van Assche <bvanassche@acm.org> 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.
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 };
Allows userspace to check supported by the controller/device functionality, e.g. write booster. Signed-off-by: Daniil Lunev <dlunev@chromium.org> --- drivers/ufs/core/ufs-sysfs.c | 10 ++++++++++ 1 file changed, 10 insertions(+)