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

Christoph Hellwig
2020-01-17T13:51:41Z

> 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.