linux-kernel - Re: [PATCH v2 2/3] scsi: ufs: core: Add ufshcd_wb_buf_resize function to enable WB buffer resize

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e03f1e62-1f22-422d-9bf8-fab480ccefb7@acm.org>
Date:   Fri, 8 Sep 2023 08:38:10 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Lu Hongfei <luhongfei@...o.com>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Avri Altman <avri.altman@....com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Can Guo <quic_cang@...cinc.com>, Bean Huo <beanhuo@...ron.com>,
        Arthur Simchaev <arthur.simchaev@....com>,
        Stanley Chu <stanley.chu@...iatek.com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Asutosh Das <quic_asutoshd@...cinc.com>,
        "Bao D. Nguyen" <quic_nguyenb@...cinc.com>,
        zhanghui <zhanghui31@...omi.com>,
        Po-Wen Kao <powen.kao@...iatek.com>,
        Eric Biggers <ebiggers@...gle.com>,
        Keoseong Park <keosung.park@...sung.com>,
        linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org
Cc:     opensource.kernel@...o.com
Subject: Re: [PATCH v2 2/3] scsi: ufs: core: Add ufshcd_wb_buf_resize function
 to enable WB buffer resize

On 9/8/23 03:20, Lu Hongfei wrote:
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index f42d99ce5bf1..85caefa421f7 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -98,6 +98,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
>   			     enum query_opcode desc_op);
>   
>   int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
> +int ufshcd_wb_buf_resize(struct ufs_hba *hba, u32 resize_op);
>   
>   /* Wrapper functions for safely calling variant operations */
>   static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 93417518c04d..7e4461360cbd 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6045,6 +6045,27 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
>   	return ufshcd_wb_presrv_usrspc_keep_vcc_on(hba, avail_buf);
>   }
>   
> +int ufshcd_wb_buf_resize(struct ufs_hba *hba, u32 resize_op)
> +{
> +	int ret;
> +	u8 index;
> +
> +	ufshcd_scsi_block_requests(hba);
> +	if (ufshcd_wait_for_doorbell_clr(hba, 1 * USEC_PER_SEC))
> +		goto out;
> +
> +	index = ufshcd_wb_get_query_index(hba);
> +	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +		QUERY_ATTR_IDN_WB_BUF_RESIZE_EN, index, 0, &resize_op);
> +	if (ret)
> +		dev_err(hba->dev,
> +			"%s: Enable WB buf resize operation failed %d\n",
> +			__func__, ret);
> +out:
> +	ufshcd_scsi_unblock_requests(hba);
> +	return ret;
> +}
> +
>   static void ufshcd_rpm_dev_flush_recheck_work(struct work_struct *work)
>   {
>   	struct ufs_hba *hba = container_of(to_delayed_work(work),
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 7d07b256e906..7dd560dc22c6 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1381,6 +1381,7 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
>   				     struct ufs_ehs *ehs_rsp, int sg_cnt,
>   				     struct scatterlist *sg_list, enum dma_data_direction dir);
>   int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
> +int ufshcd_wb_buf_resize(struct ufs_hba *hba, u32 resize_op);
>   int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable);
>   int ufshcd_suspend_prepare(struct device *dev);
>   int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm);

My feedback about this patch is as follows:
- If a new function (ufshcd_wb_buf_resize()) is introduced, a caller for 
that function should be introduced in the same patch.
- Function declarations should be added either in the private header 
file or in the public header file (include/ufs/ufshcd.h) but not in both.
- The name of the ufshcd_wb_buf_resize() function seems misleading to 
me. To me that name suggests that this functions resizes the 
WriteBooster buffer. That's not what that function does - what it does 
it to configure whether or not the UFS device is allowed to resize the 
WB buffer.
- Please remove the ufshcd_scsi_block_requests(), 
ufshcd_wait_for_doorbell_clr() and ufshcd_scsi_unblock_requests() calls. 
I'm not aware of any requirement to pause SCSI command submission while 
changing whether or not WB buffer resizing is enabled or disabled.

Thanks,

Bart.