[v1] ufs: core: adjust config_scsi_dev usage

Commit Message

Peter Wang (王信友) Feb. 20, 2024, 9:42 a.m. UTC

From: Peter Wang <[e-mail]>

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

Signed-off-by: Peter Wang <[e-mail]>
---
 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(-)

Comments

Bart Van Assche Feb. 20, 2024, 5:17 p.m. UTC | #1

On 2/20/24 01:42, [e-mail] 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.

Peter Wang (王信友) Feb. 21, 2024, 2:44 a.m. UTC | #2

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, [e-mail] 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

Bart Van Assche Feb. 21, 2024, 6:01 p.m. UTC | #3

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.

Peter Wang (王信友) Feb. 29, 2024, 7:52 a.m. UTC | #4

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

Patch

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;