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

[Bart Van Assche]

On 6/10/26 6:43 AM, Daniel Lee wrote:
> +static const char *ufs_wb_pfm_to_string(u32 pfm)
> +{
> +	if (pfm < NUM_WB_PARTIAL_FLUSH_MODES)
> +		return wb_partial_flush_modes[pfm];
> +
> +	return "unknown";
> +}

Is a new function really required for something that can be written as a
single ternary expression? Please consider folding this function into 
its caller since the above function only has one caller.

> +
> +

Please follow the convention used elsewhere in the Linux kernel and only 
insert a single blank line between definitions.

> +	ret = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,
> +		idn, index, &flag);
> +	ufshcd_rpm_put_sync(hba);
> +	if (ret) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

Why is the error code returned by ufshcd_query_flag() changed into
-EINVAL? Isn't this something that should only happen if
ufshcd_query_flag() returns a positive value? From the comment block
above the definition of ufshcd_query_flag():

 * Return: 0 upon success; > 0 in case the UFS device reported an OCS 
error;
 * < 0 if another error occurred.

> +	ret = ufshcd_query_flag(hba,
> +		value ? UPIU_QUERY_OPCODE_SET_FLAG : UPIU_QUERY_OPCODE_CLEAR_FLAG,
> +		idn, index, NULL);
> +	ufshcd_rpm_put_sync(hba);
> +	if (ret) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

Same comment here.

> +	if (ufshcd_is_qword_attr(idn))
> +		ret = ufshcd_query_attr_qword(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			idn, index, 0, &qword_value);
> +	else
> +		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			idn, index, 0, &value);
> +	ufshcd_rpm_put_sync(hba);
> +	if (ret) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

Same comment here.

> +	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +		idn, index, 0, &value);
> +	ufshcd_rpm_put_sync(hba);
> +	if (ret) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

Same comment here.

> +	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);
> +	if (ret)
> +		return -EINVAL;

Same comment here.

> +	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +		QUERY_ATTR_IDN_WB_PFM, ufshcd_wb_get_query_index(hba), 0, &value);
> +	ufshcd_rpm_put_sync(hba);
> +	up(&hba->host_sem);
> +
> +	return ret ? -EINVAL : count;

Same comment here.

Otherwise this patch looks good to me.

Thanks,

Bart.