[PATCH v1] ufs: core: adjust config_scsi_dev usage

[PATCH v1] ufs: core: adjust config_scsi_dev usage

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

Adjust the usage of config_scis_dev to mach the existing usage of
other vops in ufs driver.

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd-priv.h | 7 +++++++
 drivers/ufs/core/ufshcd.c      | 3 +--
 include/ufs/ufshcd.h           | 1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index f42d99ce5bf1..72d5287c15ee 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -288,6 +288,13 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba)
 	return -EOPNOTSUPP;
 }
 
+static inline void ufshcd_vops_config_scsi_dev(struct ufs_hba *hba,
+					       struct scsi_device *sdev)
+{
+	if (hba->vops && hba->vops->config_scsi_dev)
+		hba->vops->config_scsi_dev(sdev);
+}
+
 extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[];
 
 /**
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 16d76325039a..92f9de1d3152 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5211,8 +5211,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
 	 */
 	sdev->silence_suspend = 1;
 
-	if (hba->vops && hba->vops->config_scsi_dev)
-		hba->vops->config_scsi_dev(sdev);
+	ufshcd_vops_config_scsi_dev(hba, sdev);
 
 	ufshcd_crypto_register(hba, q);
 
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 7f0b2c5599cd..a19d87e7980f 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -327,6 +327,7 @@ struct ufs_pwr_mode_info {
  * @op_runtime_config: called to config Operation and runtime regs Pointers
  * @get_outstanding_cqs: called to get outstanding completion queues
  * @config_esi: called to config Event Specific Interrupt
+ * @config_scsi_dev: called to configure scsi device parameters
  */
 struct ufs_hba_variant_ops {
 	const char *name;
-- 
2.18.0

Re: [PATCH v1] ufs: core: adjust config_scsi_dev usage

[Bart Van Assche]

On 2/20/24 01:42, peter.wang@mediatek.com wrote:
> Adjust the usage of config_scis_dev to mach the existing usage of
> other vops in ufs driver.

I'm not sure the above is sufficient as motivation to make this change.
Why to introduce the helper function ufshcd_vops_config_scsi_dev() since it
only has one caller? Is this patch really an improvement of the UFS driver
code base?

Thanks,

Bart.

Re: [PATCH v1] ufs: core: adjust config_scsi_dev usage

[Peter Wang (王信友)]

On Tue, 2024-02-20 at 09:17 -0800, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 2/20/24 01:42, peter.wang@mediatek.com wrote:
> > Adjust the usage of config_scis_dev to mach the existing usage of
> > other vops in ufs driver.
> 
> I'm not sure the above is sufficient as motivation to make this
> change.
> Why to introduce the helper function ufshcd_vops_config_scsi_dev()
> since it
> only has one caller? Is this patch really an improvement of the UFS
> driver
> code base?
> 
> Thanks,
> 
> Bart.

Hi Bart,

This help function is designed to assist users eliminating the check.
Formalize the usage can make the code more concise and easier to read.
Such as ufshcd_vops_init/ufshcd_vops_exit is also only one caller.
Besides, it also need a comment of config_scsi_dev in ufshcd.h

This patch dose not alter the logic, it simply makes the code easier to
use and read. Pkease consider merging it. 

Thanks.
Peter 

Re: [PATCH v1] ufs: core: adjust config_scsi_dev usage

[Bart Van Assche]

On 2/20/24 18:44, Peter Wang (王信友) wrote:
> This help function is designed to assist users eliminating the check.
> Formalize the usage can make the code more concise and easier to read.
> Such as ufshcd_vops_init/ufshcd_vops_exit is also only one caller.
> Besides, it also need a comment of config_scsi_dev in ufshcd.h

My personal opinion is that the helper function makes code harder to
read because the definition of a helper function needs to be looked up
to understand the code. There are many examples in the changelog of the
kernel tree that show that there is a preference in the Linux kernel
code base to inline short functions rather than keeping or introducing
short helper functions.

Regarding the config_scsi_dev comment, shouldn't that be a separate
patch since that change is unrelated to the introduction of a helper
function?

Thanks,

Bart.

Re: [PATCH v1] ufs: core: adjust config_scsi_dev usage

[Peter Wang (王信友)]

On Wed, 2024-02-21 at 10:01 -0800, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 2/20/24 18:44, Peter Wang (王信友) wrote:
> > This help function is designed to assist users eliminating the
> check.
> > Formalize the usage can make the code more concise and easier to
> read.
> > Such as ufshcd_vops_init/ufshcd_vops_exit is also only one caller.
> > Besides, it also need a comment of config_scsi_dev in ufshcd.h
> 
> My personal opinion is that the helper function makes code harder to
> read because the definition of a helper function needs to be looked
> up
> to understand the code. There are many examples in the changelog of
> the
> kernel tree that show that there is a preference in the Linux kernel
> code base to inline short functions rather than keeping or
> introducing
> short helper functions.
> 
> Regarding the config_scsi_dev comment, shouldn't that be a separate
> patch since that change is unrelated to the introduction of a helper
> function?
> 
> Thanks,
> 
> Bart.

Hi Bart,

Got it, I will update patch and add config_scsi_dev comment only.

Thanks.
Peter