Re: [PATCH v6 5/9] scsi: ufs: UFS crypto API

[Christoph Hellwig <hch@infradead.org>]

> index 94c6c5d7334b..e88cdcde83fd 100644
> --- a/drivers/scsi/ufs/Makefile
> +++ b/drivers/scsi/ufs/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
>  obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
>  obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
>  obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o
> +ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o

This line should be moved up to just below the previous statement adding
and object to fshcd-core-.

> +static bool ufshcd_cap_idx_valid(struct ufs_hba *hba, unsigned int cap_idx)
> +{
> +	return cap_idx < hba->crypto_capabilities.num_crypto_cap;
> +}
> +
> +static u8 get_data_unit_size_mask(unsigned int data_unit_size)
> +{
> +	if (data_unit_size < 512 || data_unit_size > 65536 ||
> +	    !is_power_of_2(data_unit_size))
> +		return 0;
> +
> +	return data_unit_size / 512;
> +}
> +
> +static size_t get_keysize_bytes(enum ufs_crypto_key_size size)

Please add ufshcd_ prefixes to all the helpers.

> +	pm_runtime_get_sync(hba->dev);
> +	ufshcd_hold(hba, false);
> +	/* Clear the dword 16 */
> +	ufshcd_writel(hba, 0, slot_offset + 16 * sizeof(cfg->reg_val[0]));
> +	/* Ensure that CFGE is cleared before programming the key */
> +	wmb();
> +	for (i = 0; i < 16; i++) {
> +		ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[i]),
> +			      slot_offset + i * sizeof(cfg->reg_val[0]));
> +		/* Spec says each dword in key must be written sequentially */
> +		wmb();
> +	}
> +	/* Write dword 17 */
> +	ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[17]),
> +		      slot_offset + 17 * sizeof(cfg->reg_val[0]));
> +	/* Dword 16 must be written last */
> +	wmb();
> +	/* Write dword 16 */
> +	ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[16]),
> +		      slot_offset + 16 * sizeof(cfg->reg_val[0]));
> +	wmb();

All these wmb calls look bogus as writel itself orders mmio writes,
while wmb is not guaranteed to have any effect on mmio space.

> +EXPORT_SYMBOL_GPL(ufshcd_crypto_enable);

> +}
> +EXPORT_SYMBOL_GPL(ufshcd_crypto_disable);

None of the exported symbols in this file is used outside
ufshcd-core.ko, so all the exports caan be dropped.

> +	if (ufs_crypto_alg == UFS_CRYPTO_ALG_AES_XTS &&
> +		key_size == UFS_CRYPTO_KEY_SIZE_256)
> +		return BLK_ENCRYPTION_MODE_AES_256_XTS;

Please don't indent continuation lines of conditional with a single tab.

> +static inline int ufshcd_num_keyslots(struct ufs_hba *hba)

> +static inline bool ufshcd_keyslot_valid(struct ufs_hba *hba, unsigned int slot)

The two functions are only used in ufshcd-crypto.c and can be moved
there.

> +static inline bool ufshcd_is_crypto_enabled(struct ufs_hba *hba)
> +{
> +	return hba->caps & UFSHCD_CAP_CRYPTO;
> +}

I think this one would be clearer to just open code in the three callers.