linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Satya Tangirala <satyat@google.com>
Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
	Kuohong Wang <kuohong.wang@mediatek.com>,
	Kim Boojin <boojin.kim@samsung.com>
Subject: Re: [PATCH v5 5/9] scsi: ufs: UFS crypto API
Date: Thu, 31 Oct 2019 11:23:14 -0700	[thread overview]
Message-ID: <20191031182314.GD23601@infradead.org> (raw)
In-Reply-To: <20191028072032.6911-6-satyat@google.com>

> +static size_t get_keysize_bytes(enum ufs_crypto_key_size size)
> +{
> +	switch (size) {
> +	case UFS_CRYPTO_KEY_SIZE_128: return 16;
> +	case UFS_CRYPTO_KEY_SIZE_192: return 24;
> +	case UFS_CRYPTO_KEY_SIZE_256: return 32;
> +	case UFS_CRYPTO_KEY_SIZE_512: return 64;
> +	default: return 0;
> +	}
> +}

Please fix the indentation and move all the returns to their own
lines.  There are various more spots that will need to be fixed
like this as well later in the patch.

> +
> +static int ufshcd_crypto_cap_find(void *hba_p,
> +			   enum blk_crypto_mode_num crypto_mode,
> +			   unsigned int data_unit_size)
> +{
> +	struct ufs_hba *hba = hba_p;

Please properly type the first argument.

> +	case UFS_CRYPTO_ALG_BITLOCKER_AES_CBC: // fallthrough

Please don't use // comments.

> +static void program_key(struct ufs_hba *hba,
> +			const union ufs_crypto_cfg_entry *cfg,
> +			int slot)

The function name needs a ufshcd prefix.

> +	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();

wmb() has no meaning for MMIO operations, something looks very fishy
here.

> +static int ufshcd_crypto_keyslot_program(void *hba_p, const u8 *key,
> +					 enum blk_crypto_mode_num crypto_mode,
> +					 unsigned int data_unit_size,
> +					 unsigned int slot)
> +{
> +	struct ufs_hba *hba = hba_p;

This is not a very type safe API.  I think the proper thing to do
would be to allocte the struct keyslot_manager in the driver (ufshcd)
as part of the containing structure (ufs_hba) and then just have
a keyslot_manager_init that initializes the field.  Then pass the
struct keyslot_manager to the methods, which can use container_of
to get the containing structure.

> +#define NUM_KEYSLOTS(hba) (hba->crypto_capabilities.config_count + 1)

Please make this an inline function.

  reply	other threads:[~2019-10-31 18:23 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-28  7:20 [PATCH v5 0/9] Inline Encryption Support Satya Tangirala
2019-10-28  7:20 ` [PATCH v5 1/9] block: Keyslot Manager for Inline Encryption Satya Tangirala
2019-10-31 18:04   ` Christoph Hellwig
2019-10-28  7:20 ` [PATCH v5 2/9] block: Add encryption context to struct bio Satya Tangirala
2019-10-31 18:16   ` Christoph Hellwig
2019-10-28  7:20 ` [PATCH v5 3/9] block: blk-crypto for Inline Encryption Satya Tangirala
2019-10-31 17:57   ` Christoph Hellwig
2019-10-31 20:50     ` Theodore Y. Ts'o
2019-10-31 21:22       ` Christoph Hellwig
2019-11-05  2:01         ` Eric Biggers
2019-11-05 15:39           ` Christoph Hellwig
2019-10-28  7:20 ` [PATCH v5 4/9] scsi: ufs: UFS driver v2.1 spec crypto additions Satya Tangirala
2019-10-28  7:20 ` [PATCH v5 5/9] scsi: ufs: UFS crypto API Satya Tangirala
2019-10-31 18:23   ` Christoph Hellwig [this message]
2019-10-28  7:20 ` [PATCH v5 6/9] scsi: ufs: Add inline encryption support to UFS Satya Tangirala
2019-10-31 18:26   ` Christoph Hellwig
2019-10-28  7:20 ` [PATCH v5 7/9] fscrypt: add inline encryption support Satya Tangirala
2019-10-31 18:32   ` Christoph Hellwig
2019-10-31 20:21     ` Eric Biggers
2019-10-31 21:21       ` Christoph Hellwig
2019-10-31 22:25         ` Eric Biggers
2019-11-05  0:15           ` Christoph Hellwig
2019-11-05  1:03             ` Eric Biggers
2019-11-05  3:12         ` Eric Biggers
2019-10-28  7:20 ` [PATCH v5 8/9] f2fs: " Satya Tangirala
2019-10-31 17:14   ` Jaegeuk Kim
2019-10-28  7:20 ` [PATCH v5 9/9] ext4: " Satya Tangirala

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191031182314.GD23601@infradead.org \
    --to=hch@infradead.org \
    --cc=bmuthuku@qti.qualcomm.com \
    --cc=boojin.kim@samsung.com \
    --cc=kuohong.wang@mediatek.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=satyat@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).