Re: [PATCH v2] scsi: ufs: Implement Auto-Hibern8 setup

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Tue, 2017-07-25 at 11:45 +0200, Michal Potomski wrote:
> Since Auto-Hibern8 feature has to be enabled by the user
> proper API has been given via SysFS.
> 
> We expose this API to user-space, since we don't know
> in driver, what kind of additional Power Management rules
> user wants to use. Due to that we want to expose API, to
> let user or high level S/W decide, whether it wants to
> use Auto-Hibern8 feature for Power Saving and give him
> "slider" to decide between performance and power efficiency.
> This is important because different platforms using
> the same UFS host and/or device might want different
> options on this one, e.g. High-End Desktop PC might
> want to have this feature disabled, while Mobile or
> Server platforms might want to have this feature enabled,
> but levels will vary depending on what's to be acheived.
> 
> As this feature is meant to be transparent for driver,
> we'd like to let user decide whether he wants this enabled
> or not.

Less than ten minutes after v1 of this patch was posted version v2 was
posted. What is the difference between v1 and v2? Second and later versions
of a patch or patch series are expected to include a changelog.

Since I'm not familiar with the Auto-Hibern8 feature: what impact does it
have on command processing? Does it e.g. cause SCSI or TMF commands that are
sent to the UFS device to be ignored, to fail or to time out?

> +static ssize_t ufshcd_auto_hibern8_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	u32 val;
> +	unsigned long timer;
> +	u8 scale;
> +	char *unit;
> +
> +	val = ufshcd_read_auto_hibern8_state(hba);
> +	timer = val & UFSHCI_AHIBERN8_TIMER_MASK;
> +	scale =	(val & UFSHCI_AHIBERN8_SCALE_MASK)
> +			>> UFSHCI_AHIBERN8_SCALE_OFFSET;
> +
> +	unit = scale >= UFSHCI_AHIBERN8_SCALE_1MS ? "ms" : "us";
> +
> +	for (scale %= UFSHCI_AHIBERN8_SCALE_1MS; scale > 0; --scale)
> +		timer *= UFS_AHIBERN8_SCALE_STEP_MAGNITUDE;
> +
> +	return snprintf(buf, PAGE_SIZE, "%ld %s\n", timer, unit);
> +}

>From Documentation/filesystems/sysfs.txt: "All new sysfs attributes must be
documented in Documentation/ABI. See also Documentation/ABI/README for more
information." Please add proper documentation of the attributes added by this
patch when you resubmit this patch.

Regarding this attribute: units should be documented in Documentation/ABI/*/*
instead of specifying these in the attribute itself. In other words, the
"ms" / "us" suffix should be left out.

> @@ -7693,16 +7766,34 @@ static void ufshcd_add_spm_lvl_sysfs_nodes(struct ufs_hba *hba)
>  		dev_err(hba->dev, "Failed to create sysfs for spm_lvl\n");
>  }
>  
> +static void ufshcd_add_ahibern8_sysfs_node(struct ufs_hba *hba)
> +{
> +	if (!(hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT))
> +		return;
> +
> +	hba->ahibern8_attr.show = ufshcd_auto_hibern8_show;
> +	hba->ahibern8_attr.store = ufshcd_auto_hibern8_store;
> +	sysfs_attr_init(&hba->ahibern8_attr.attr);
> +	hba->ahibern8_attr.attr.name = "ufs_auto_hibern8";
> +	hba->ahibern8_attr.attr.mode = 0600;
> +	if (device_create_file(hba->dev, &hba->ahibern8_attr))
> +		dev_err(hba->dev, "Failed to create sysfs for ufs_auto_hibern8\n");
> +}

Please use a static variable at file scope and DRIVER_ATTR_RW() to initialize
the sysfs attributes instead of initializing the sysfs attributes explicitly.

Additionally, please set sdev_attrs in struct scsi_host_template instead of
calling device_create_file() explicitly. The above 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.

I am aware the above code follows the style that is used for other UFS sysfs
attributes. If you would have the time to convert the code for creating the
existing UFS sysfs attributes that would be great.

Thanks,

Bart.