Re: [PATCH v3 3/3] scsi: ufs: Add inline encryption support to UFS

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Jul 06, 2020 at 07:10:16PM +0000, Satya Tangirala wrote:
> +/**
> + * ufshcd_init_crypto - Initialize crypto hardware
> + * @hba: Per adapter instance
> + */
> +void ufshcd_init_crypto(struct ufs_hba *hba)
> +{
> +	int slot = 0;
> +
> +	if (!(hba->caps & UFSHCD_CAP_CRYPTO))
> +		return;
> +
> +	/* Clear all keyslots - the number of keyslots is (CFGC + 1) */
> +	for (slot = 0; slot < hba->crypto_capabilities.config_count + 1; slot++)
> +		ufshcd_clear_keyslot(hba, slot);
> +}

This should just use 'int slot;'.  No need to initialize to 0.

> diff --git a/drivers/scsi/ufs/ufshcd-crypto.h b/drivers/scsi/ufs/ufshcd-crypto.h
> index 22677619de59..2512aef03f76 100644
> --- a/drivers/scsi/ufs/ufshcd-crypto.h
> +++ b/drivers/scsi/ufs/ufshcd-crypto.h
> @@ -10,9 +10,36 @@
>  #include "ufshcd.h"
>  #include "ufshci.h"
>  
> +static inline void ufshcd_prepare_lrbp_crypto(struct ufs_hba *hba,
> +					      struct request *rq,
> +					      struct ufshcd_lrb *lrbp)
> +{
> +	if (!rq || !rq->crypt_keyslot) {
> +		lrbp->crypto_key_slot = -1;
> +		return;
> +	}
> +
> +	lrbp->crypto_key_slot = blk_ksm_get_slot_idx(rq->crypt_keyslot);
> +	lrbp->data_unit_num = rq->crypt_ctx->bc_dun[0];
> +}

This function doesn't use the 'hba' argument, so it should be removed.

> +
> +static inline void
> +ufshcd_prepare_req_desc_hdr_crypto(struct ufshcd_lrb *lrbp, u32 *dword_0,
> +				   u32 *dword_1, u32 *dword_3)
> +{
> +	if (lrbp->crypto_key_slot >= 0) {
> +		*dword_0 |= UTP_REQ_DESC_CRYPTO_ENABLE_CMD;
> +		*dword_0 |= lrbp->crypto_key_slot;
> +		*dword_1 = lower_32_bits(lrbp->data_unit_num);
> +		*dword_3 = upper_32_bits(lrbp->data_unit_num);
> +	}
> +}
> +
>  bool ufshcd_crypto_enable(struct ufs_hba *hba);
>  
> -int ufshcd_hba_init_crypto(struct ufs_hba *hba);
> +void ufshcd_init_crypto(struct ufs_hba *hba);
> +
> +int ufshcd_hba_init_crypto_capabilities(struct ufs_hba *hba);

The .c file defines ufshcd_hba_init_crypto_capabilities() before
ufshcd_init_crypto().  It would be nice to put the declarations in the same
order.

> @@ -2554,6 +2577,7 @@ static int ufshcd_compose_dev_cmd(struct ufs_hba *hba,
>  	lrbp->task_tag = tag;
>  	lrbp->lun = 0; /* device management cmd is not specific to any LUN */
>  	lrbp->intr_cmd = true; /* No interrupt aggregation */
> +	ufshcd_prepare_lrbp_crypto(hba, NULL, lrbp);
>  	hba->dev_cmd.type = cmd_type;
>  
>  	return ufshcd_comp_devman_upiu(hba, lrbp);
> @@ -4650,6 +4674,8 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>  	if (ufshcd_is_rpm_autosuspend_allowed(hba))
>  		sdev->rpm_autosuspend = 1;
>  
> +	ufshcd_crypto_setup_rq_keyslot_manager(hba, q);
> +
>  	return 0;
>  }
>  
> @@ -6115,6 +6141,9 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
>  	lrbp->task_tag = tag;
>  	lrbp->lun = 0;
>  	lrbp->intr_cmd = true;
> +#ifdef CONFIG_SCSI_UFS_CRYPTO
> +	lrbp->crypto_key_slot = -1; /* No crypto operations */
> +#endif

This should use ufshcd_prepare_lrbp_crypto(hba, NULL, lrbp),
like ufshcd_compose_dev_cmd() does.

- Eric