On Wed, 2017-07-26 at 16:55 +0200, Michal Potomski wrote:
> Previous implementation was obsolete and needed to be updated.
> Additionally device_create_file() call occurs after a UFS device
> has been made visible in sysfs and hence will cause trouble
> (race condition) if a udev rule tries to set this attribute
> from inside a rule that is triggered by device creation.
Hello Michal,
Thank you for having done this work!
> +static ssize_t rpm_lvl_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ufs_hba *hba = dev_get_drvdata(dev);
> + int curr_len;
> + u8 lvl;
> +
> + curr_len = snprintf(buf, PAGE_SIZE,
> + "\nCurrent Runtime PM level [%d] => dev_state [%s] link_state [%s]\n",
> + hba->rpm_lvl,
> + ufschd_ufs_dev_pwr_mode_to_string(
> + ufs_pm_lvl_states[hba->rpm_lvl].dev_state),
> + ufschd_uic_link_state_to_string(
> + ufs_pm_lvl_states[hba->rpm_lvl].link_state));
> +
> + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
> + "\nAll available Runtime PM levels info:\n");
> + for (lvl = UFS_PM_LVL_0; lvl < UFS_PM_LVL_MAX; lvl++)
> + curr_len += snprintf((buf + curr_len), (PAGE_SIZE - curr_len),
> + "\tRuntime PM level [%d] => dev_state [%s] link_state [%s]\n",
> + lvl,
> + ufschd_ufs_dev_pwr_mode_to_string(
> + ufs_pm_lvl_states[lvl].dev_state),
> + ufschd_uic_link_state_to_string(
> + ufs_pm_lvl_states[lvl].link_state));
> +
> + return curr_len;
> +}
I am aware that this code is existing code that has been moved up. But are
the UFS contributors aware that this kind of output violates the sysfs rules?
Please consider either to move this attribute to debugfs (the one value per
file rule only applies to sysfs and configfs) or to split it into multiple
sysfs attributes if that is possible without breaking existing user space
applications. A quote from
https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt: "Attributes
should be ASCII text files, preferably with only one value per file. It is
noted that it may not be efficient to contain only one value per file, so it
is socially acceptable to express an array of values of the same type."
Anyway, since this patch is an improvement compared to the current code base:
Reviewed-by: Bart Van Assche <bart.vanassche@xxxxxxx>
