Re: [PATCH v3] ufs: sysfs: Add WB partial flush mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unhandled pm_runtime_get_sync error leads to potential hardware access on suspended device
--

commit 75ac0206b35904f19c6179a4c935e644238e4421
Author: Daniel Lee <chullee@google.com>
Subject: ufs: sysfs: Add WB partial flush mode support

This commit adds sysfs entries for JEDEC UFS 4.1 Extended WriteBooster
attributes and flags to support FIFO and Pinned partial flush modes.
It introduces UFS_ATTRIBUTE_RW and UFS_FLAG_RW macros to support
writable attributes and flags.

> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index d9dc4cc3452ee..fbf7404d6d78a 100644
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
[ ... ]
> @@ -1527,41 +1533,94 @@ static const struct attribute_group ufs_sysfs_string_descriptors_group = {
>  
>  static inline bool ufshcd_is_wb_flags(enum flag_idn idn)
>  {
> -	return idn >= QUERY_FLAG_IDN_WB_EN &&
> -		idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8;
> +	return (idn >= QUERY_FLAG_IDN_WB_EN &&
> +		idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8) ||
> +	       idn == QUERY_FLAG_IDN_WB_UNPIN_EN;
> +}
> +
> +static ssize_t ufs_sysfs_flag_show(struct device *dev,
> +	struct device_attribute *attr, char *buf, enum flag_idn idn)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	u8 index = 0;
> +	bool flag;
> +	int ret;
> +
> +	down(&hba->host_sem);
> +	if (!ufshcd_is_user_access_allowed(hba)) {
> +		up(&hba->host_sem);
> +		return -EBUSY;
> +	}
> +	if (ufshcd_is_wb_flags(idn))
> +		index = ufshcd_wb_get_query_index(hba);
> +	ufshcd_rpm_get_sync(hba);
> +	ret = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,
> +		idn, index, &flag);
> +	ufshcd_rpm_put_sync(hba);

[Severity: High]
This isn't a bug introduced by this patch, but this newly refactored function
ignores the return value of ufshcd_rpm_get_sync(). If the device W-LUN and
underlying HBA fail to resume from runtime suspend (returning a negative error
code), does it make sense to blindly proceed to call ufshcd_query_flag()?

Writing to or accessing MMIO registers of an unclocked or unpowered UFS
controller can trigger a synchronous external abort or PCI master abort.

[ ... ]
> @@ -1804,6 +1916,71 @@ UFS_ATTRIBUTE(wb_avail_buf, _AVAIL_WB_BUFF_SIZE);
>  UFS_ATTRIBUTE(wb_life_time_est, _WB_BUFF_LIFE_TIME_EST);
>  UFS_ATTRIBUTE(wb_cur_buf, _CURR_WB_BUFF_SIZE);
>  
> +static ssize_t wb_partial_flush_mode_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
[ ... ]
> +	down(&hba->host_sem);
> +	if (!ufshcd_is_user_access_allowed(hba)) {
> +		up(&hba->host_sem);
> +		return -EBUSY;
> +	}
> +	ufshcd_rpm_get_sync(hba);
> +	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +		QUERY_ATTR_IDN_WB_PFM, ufshcd_wb_get_query_index(hba), 0, &value);
> +	ufshcd_rpm_put_sync(hba);
> +	up(&hba->host_sem);

[Severity: High]
This is a pre-existing issue in the older sysfs macros that has been propagated
here to the newly created functions. Should we check the return value of
ufshcd_rpm_get_sync() here as well? The same pattern is present in
wb_partial_flush_mode_store(), ufs_sysfs_attr_show(), ufs_sysfs_attr_store(),
and ufs_sysfs_flag_store().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612150136.2263477-1-chullee@google.com?part=1