mbox series

[0/4] Re-use device management code fragments

Message ID 20240304092346.654-1-avri.altman@wdc.com
Headers show
Series Re-use device management code fragments | expand

Message

Avri Altman March 4, 2024, 9:23 a.m. UTC 
Device management commands are constructed for query commands that are
being issued by the driver, but also for raw device management commands
originated by the bsg module, and recently, by the advanced rpmb
handler. Thus, the same code fragments, e.g. locking, composing the
command, composing the upiu etc., appear over and over. Remove those
duplications.  Theoretically, there should be no functional change.

Avri Altman (4):
  scsi: ufs: Re-use device management locking code
  scsi: ufs: Re-use exec_dev_cmd
  scsi: ufs: Re-use compose_dev_cmd
  scsi: ufs: Re-use compose_devman_upiu

 drivers/ufs/core/ufshcd.c | 200 ++++++++++++++++----------------------
 1 file changed, 83 insertions(+), 117 deletions(-)

Comments

Bart Van Assche March 5, 2024, 7:04 p.m. UTC | #1 
On 3/4/24 01:23, Avri Altman wrote:
>   /**
>    * ufshcd_exec_dev_cmd - API for sending device management requests
>    * @hba: UFS hba
> @@ -3291,11 +3305,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>   	struct ufshcd_lrb *lrbp;
>   	int err;
>   
> -	/* Protects use of hba->reserved_slot. */
> -	lockdep_assert_held(&hba->dev_cmd.lock);
> -
> -	down_read(&hba->clk_scaling_lock);
> -
>   	lrbp = &hba->lrb[tag];
>   	lrbp->cmd = NULL;
>   	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);

Please restore the lockdep_assert_held() call.

> -	/* Protects use of hba->reserved_slot. */
> -	lockdep_assert_held(&hba->dev_cmd.lock);
> -
> -	down_read(&hba->clk_scaling_lock);
> -
>   	lrbp = &hba->lrb[tag];
>   	lrbp->cmd = NULL;
>   	lrbp->task_tag = tag;

Same comment here - please restore the lockdep_assert_held() call.

Otherwise this patch looks good to me.

Thanks,

Bart.
Bart Van Assche March 5, 2024, 7:13 p.m. UTC | #2 
On 3/4/24 01:23, Avri Altman wrote:
> -static int ufshcd_compose_dev_cmd(struct ufs_hba *hba,
> -		struct ufshcd_lrb *lrbp, enum dev_cmd_type cmd_type, int tag)
> +static void __compose_dev_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
> +			     enum dev_cmd_type cmd_type, u8 lun, int tag)
>   {
>   	lrbp->cmd = NULL;
>   	lrbp->task_tag = tag;
> -	lrbp->lun = 0; /* device management cmd is not specific to any LUN */
> +	lrbp->lun = lun;
>   	lrbp->intr_cmd = true; /* No interrupt aggregation */
>   	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
>   	hba->dev_cmd.type = cmd_type;
> +}

Please chose a better function name than __compose_dev_cmd() and please 
make sure that the function name starts with the ufshcd_ prefix.

Thanks,

Bart.