Re: [PATCH v5 6/9] scsi: ufs: Add inline encryption support to UFS

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 6/9] scsi: ufs: Add inline encryption support to UFS
Date: Thu, 31 Oct 2019 11:26:29 -0700	[thread overview]
Message-ID: <20191031182629.GE23601@infradead.org> (raw)
In-Reply-To: <20191028072032.6911-7-satyat@google.com>

>  	/* Transfer request descriptor header fields */
> +	if (lrbp->crypto_enable) {

Maybe we want a little inline function so that we can use IS_ENABLED
to make sure the compiler eliminates the dead code if crypt config
option is not set.

 a) don't have to define the crypto_enable if the config options are
    not set

> +		dword_0 |= UTP_REQ_DESC_CRYPTO_ENABLE_CMD;
> +		dword_0 |= lrbp->crypto_key_slot;
> +		req_desc->header.dword_1 =
> +			cpu_to_le32((u32)lrbp->data_unit_num);
> +		req_desc->header.dword_3 =
> +			cpu_to_le32((u32)(lrbp->data_unit_num >> 32));

This should use ther upper_32_bits / lower_32_bits helpers.

> +static inline int ufshcd_prepare_lrbp_crypto(struct ufs_hba *hba,
> +					     struct scsi_cmnd *cmd,
> +					     struct ufshcd_lrb *lrbp)
> +{
> +	int key_slot;
> +
> +	if (!cmd->request->bio ||
> +	    !bio_crypt_should_process(cmd->request->bio, cmd->request->q)) {
> +		lrbp->crypto_enable = false;
> +		return 0;
> +	}
> +
> +	if (WARN_ON(!ufshcd_is_crypto_enabled(hba))) {
> +		/*
> +		 * Upper layer asked us to do inline encryption
> +		 * but that isn't enabled, so we fail this request.
> +		 */
> +		return -EINVAL;
> +	}
> +	key_slot = bio_crypt_get_keyslot(cmd->request->bio);
> +	if (!ufshcd_keyslot_valid(hba, key_slot))
> +		return -EINVAL;
> +
> +	lrbp->crypto_enable = true;
> +	lrbp->crypto_key_slot = key_slot;
> +	lrbp->data_unit_num = bio_crypt_data_unit_num(cmd->request->bio);
> +
> +	return 0;

I think this should go into ufshcd-crypto.c so that it can be stubbed
out for non-crypto builds.  That also means we can remove various
stubs for the block layer helpers and just dereference the fields
directly, helping with code readability.