Re: [PATCH v13 06/12] scsi: ufs: UFS driver v2.1 spec crypto additions

[Stanley Chu <stanley.chu@mediatek.com>]

Hi Satya,

On Thu, 2020-05-14 at 00:37 +0000, Satya Tangirala wrote:
> Add the crypto registers and structs defined in v2.1 of the JEDEC UFSHCI
> specification in preparation to add support for inline encryption to
> UFS.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> Reviewed-by: Eric Biggers <ebiggers@google.com>
> ---
>  drivers/scsi/ufs/ufshcd.c |  2 ++
>  drivers/scsi/ufs/ufshcd.h |  6 ++++
>  drivers/scsi/ufs/ufshci.h | 67 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 698e8d20b4bac..2435c600cb2d9 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4767,6 +4767,8 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>  	case OCS_MISMATCH_RESP_UPIU_SIZE:
>  	case OCS_PEER_COMM_FAILURE:
>  	case OCS_FATAL_ERROR:
> +	case OCS_INVALID_CRYPTO_CONFIG:
> +	case OCS_GENERAL_CRYPTO_ERROR:
>  	default:
>  		result |= DID_ERROR << 16;
>  		dev_err(hba->dev,
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 6ffc08ad85f63..835b9a844aa21 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -555,6 +555,12 @@ enum ufshcd_caps {
>  	 * for userspace to control the power management.
>  	 */
>  	UFSHCD_CAP_RPM_AUTOSUSPEND			= 1 << 6,
> +
> +	/*
> +	 * This capability allows the host controller driver to use the
> +	 * inline crypto engine, if it is present
> +	 */
> +	UFSHCD_CAP_CRYPTO				= 1 << 7,
>  };
>  
>  /**
> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
> index c2961d37cc1cf..c0651fe6dbbc6 100644
> --- a/drivers/scsi/ufs/ufshci.h
> +++ b/drivers/scsi/ufs/ufshci.h
> @@ -90,6 +90,7 @@ enum {
>  	MASK_64_ADDRESSING_SUPPORT		= 0x01000000,
>  	MASK_OUT_OF_ORDER_DATA_DELIVERY_SUPPORT	= 0x02000000,
>  	MASK_UIC_DME_TEST_MODE_SUPPORT		= 0x04000000,
> +	MASK_CRYPTO_SUPPORT			= 0x10000000,
>  };
>  
>  #define UFS_MASK(mask, offset)		((mask) << (offset))
> @@ -143,6 +144,7 @@ enum {
>  #define DEVICE_FATAL_ERROR			0x800
>  #define CONTROLLER_FATAL_ERROR			0x10000
>  #define SYSTEM_BUS_FATAL_ERROR			0x20000
> +#define CRYPTO_ENGINE_FATAL_ERROR		0x40000
>  
>  #define UFSHCD_UIC_HIBERN8_MASK	(UIC_HIBERNATE_ENTER |\
>  				UIC_HIBERNATE_EXIT)
> @@ -155,11 +157,13 @@ enum {
>  #define UFSHCD_ERROR_MASK	(UIC_ERROR |\
>  				DEVICE_FATAL_ERROR |\
>  				CONTROLLER_FATAL_ERROR |\
> -				SYSTEM_BUS_FATAL_ERROR)
> +				SYSTEM_BUS_FATAL_ERROR |\
> +				CRYPTO_ENGINE_FATAL_ERROR)
>  
>  #define INT_FATAL_ERRORS	(DEVICE_FATAL_ERROR |\
>  				CONTROLLER_FATAL_ERROR |\
> -				SYSTEM_BUS_FATAL_ERROR)
> +				SYSTEM_BUS_FATAL_ERROR |\
> +				CRYPTO_ENGINE_FATAL_ERROR)
>  
>  /* HCS - Host Controller Status 30h */
>  #define DEVICE_PRESENT				0x1
> @@ -318,6 +322,61 @@ enum {
>  	INTERRUPT_MASK_ALL_VER_21	= 0x71FFF,
>  };
>  
> +/* CCAP - Crypto Capability 100h */
> +union ufs_crypto_capabilities {
> +	__le32 reg_val;
> +	struct {
> +		u8 num_crypto_cap;
> +		u8 config_count;
> +		u8 reserved;
> +		u8 config_array_ptr;
> +	};
> +};
> +
> +enum ufs_crypto_key_size {
> +	UFS_CRYPTO_KEY_SIZE_INVALID	= 0x0,
> +	UFS_CRYPTO_KEY_SIZE_128		= 0x1,
> +	UFS_CRYPTO_KEY_SIZE_192		= 0x2,
> +	UFS_CRYPTO_KEY_SIZE_256		= 0x3,
> +	UFS_CRYPTO_KEY_SIZE_512		= 0x4,
> +};
> +
> +enum ufs_crypto_alg {
> +	UFS_CRYPTO_ALG_AES_XTS			= 0x0,
> +	UFS_CRYPTO_ALG_BITLOCKER_AES_CBC	= 0x1,
> +	UFS_CRYPTO_ALG_AES_ECB			= 0x2,
> +	UFS_CRYPTO_ALG_ESSIV_AES_CBC		= 0x3,
> +};
> +
> +/* x-CRYPTOCAP - Crypto Capability X */
> +union ufs_crypto_cap_entry {
> +	__le32 reg_val;
> +	struct {
> +		u8 algorithm_id;
> +		u8 sdus_mask; /* Supported data unit size mask */
> +		u8 key_size;
> +		u8 reserved;
> +	};
> +};
> +
> +#define UFS_CRYPTO_CONFIGURATION_ENABLE (1 << 7)
> +#define UFS_CRYPTO_KEY_MAX_SIZE 64
> +/* x-CRYPTOCFG - Crypto Configuration X */
> +union ufs_crypto_cfg_entry {
> +	__le32 reg_val[32];
> +	struct {
> +		u8 crypto_key[UFS_CRYPTO_KEY_MAX_SIZE];
> +		u8 data_unit_size;
> +		u8 crypto_cap_idx;
> +		u8 reserved_1;
> +		u8 config_enable;
> +		u8 reserved_multi_host;
> +		u8 reserved_2;
> +		u8 vsb[2];
> +		u8 reserved_3[56];
> +	};
> +};
> +
>  /*
>   * Request Descriptor Definitions
>   */
> @@ -339,6 +398,7 @@ enum {
>  	UTP_NATIVE_UFS_COMMAND		= 0x10000000,
>  	UTP_DEVICE_MANAGEMENT_FUNCTION	= 0x20000000,
>  	UTP_REQ_DESC_INT_CMD		= 0x01000000,
> +	UTP_REQ_DESC_CRYPTO_ENABLE_CMD	= 0x00800000,
>  };
>  
>  /* UTP Transfer Request Data Direction (DD) */
> @@ -358,6 +418,9 @@ enum {
>  	OCS_PEER_COMM_FAILURE		= 0x5,
>  	OCS_ABORTED			= 0x6,
>  	OCS_FATAL_ERROR			= 0x7,
> +	OCS_DEVICE_FATAL_ERROR		= 0x8,
> +	OCS_INVALID_CRYPTO_CONFIG	= 0x9,
> +	OCS_GENERAL_CRYPTO_ERROR	= 0xA,

A nit is that OCS_DEVICE_FATAL_ERROR definition is added here but not
added in ufshcd_transfer_rsp_status().

This will not have any side effect because OCS_DEVICE_FATAL_ERROR was
already handled by "default" label in ufshcd_transfer_rsp_status().

>  	OCS_INVALID_COMMAND_STATUS	= 0x0F,
>  	MASK_OCS			= 0x0F,
>  };

Otherwise looks good to me.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Thanks.