Re: [PATCH 1/3] crypto: ux500: Add driver for CRYP hardware.

On Wed, 14 Mar 2012 09:45:46 +0100
Andreas Westin [off-list ref] wrote:

+config CRYPTO_DEV_UX500
+	tristate "Driver for ST-Ericsson UX500 crypto hardware acceleration"

+	#depends on ARCH_U8500

guessing this should be either removed or modified to include u5500?

+config CRYPTO_DEV_UX500_CRYP
+	tristate "UX500 crypto driver for CRYP block"
+	depends on CRYPTO_DEV_UX500
+	select CRYPTO_DES
+	help
+	  This is the driver for the crypto block CRYP.

what will its module be called?

+config CRYPTO_DEV_UX500_DEBUG
+	bool "Activate ux500 platform debug-mode for crypto and hash block"
+	depends on CRYPTO_DEV_UX500_CRYP || CRYPTO_DEV_UX500_HASH
+	default n
+	help
+	  Say Y if you want to add debug prints to ux500_hash and
+	  ux500_cryp devices.

there are better ways to do this now - see dynamic-debug-howto.txt.

+	int peripheralID2 = 0;

mixed-case name

+
+	if (NULL == device_data)
+		return -EINVAL;
+
+	if (cpu_is_u8500())
+		peripheralID2 = CRYP_PERIPHERAL_ID2_DB8500;
+	else if (cpu_is_u5500())
+		peripheralID2 = CRYP_PERIPHERAL_ID2_DB5500;
+
+	/* Check Peripheral and Pcell Id Register for CRYP */
+	if ((CRYP_PERIPHERAL_ID0 ==
+		readl_relaxed(&device_data->base->periphId0))
+	    && (CRYP_PERIPHERAL_ID1 ==
+		    readl_relaxed(&device_data->base->periphId1))
+	    && (peripheralID2 ==
+		    readl_relaxed(&device_data->base->periphId2))
+	    && (CRYP_PERIPHERAL_ID3 ==
+		    readl_relaxed(&device_data->base->periphId3))
+	    && (CRYP_PCELL_ID0 ==
+		    readl_relaxed(&device_data->base->pcellId0))
+	    && (CRYP_PCELL_ID1 ==
+		    readl_relaxed(&device_data->base->pcellId1))
+	    && (CRYP_PCELL_ID2 ==
+		    readl_relaxed(&device_data->base->pcellId2))
+	    && (CRYP_PCELL_ID3 ==
+		    readl_relaxed(&device_data->base->pcellId3))) {

mixed-case names

+		return 0;
+	}
+
+	return -EPERM;
+}
+
+/**
+ * cryp_activity - This routine enables/disable the cryptography function.
+ * @device_data: Pointer to the device data struct for base address.
+ * @cryp_activity: Enable/Disable functionality
+ */
+void cryp_activity(struct cryp_device_data *device_data,
+		   enum cryp_crypen cryp_crypen)

comment should say @cryp_crypen:

+void cryp_flush_inoutfifo(struct cryp_device_data *device_data)
+{
+	/*
+	 * We always need to disble the hardware before trying to flush the

                             disable

+		 * After the keyprepartion for decrypting is done you should set

                             key preparation

+int cryp_configure_key_values(struct cryp_device_data *device_data,
+			      enum cryp_key_reg_index key_reg_index,
+			      struct cryp_key_value key_value)
+{
+	while (cryp_is_logic_busy(device_data))
+		cpu_relax();
+
+	switch (key_reg_index) {
+	case CRYP_KEY_REG_1:
+		writel_relaxed(key_value.key_value_left,
+		       &device_data->base->key_1_l);

alignment issues, presumably after a s/writel/writel_relaxed/g
during development (should make it easy to re-check all occurrences).

+/**
+ * cryp_write_indata - This routine writes 32 bit data into the data input
+ *		       register of the cryptography IP.
+ * @device_data: Pointer to the device data struct for base address.
+ * @write_data: Data word to write
+ */
+int cryp_write_indata(struct cryp_device_data *device_data, u32 write_data)
+{
+	writel_relaxed(write_data, &device_data->base->din);
+
+	return 0;
+}
+
+/**
+ * cryp_read_outdata - This routine reads the data from the data output
+ *		       register of the CRYP logic
+ * @device_data: Pointer to the device data struct for base address.
+ * @read_data: Read the data from the output FIFO.
+ */
+int cryp_read_outdata(struct cryp_device_data *device_data, u32 *read_data)
+{
+	*read_data = readl_relaxed(&device_data->base->dout);
+
+	return 0;
+}

I would say these should be made void, static, inline, etc., but
they only have one callsite each - are they necessary at all?

+ * Protection configuration structure for setting privilage access

                                                     privileged

+ * CRYP power management specifc structure.

                            specific

+/**
+ * uint8p_to_uint32_be - 4*uint8 to uint32 big endian
+ * @in: Data to convert.
+ */
+static inline u32 uint8p_to_uint32_be(u8 *in)
+{
+	return  (u32)in[0]<<24 |
+		((u32)in[1]<<16) |
+		((u32)in[2]<<8) |
+		((u32)in[3]);
+}

use cpu_to_be32

+/**
+ * swap_bits_in_byte - mirror the bits in a byte
+ * @b: the byte to be mirrored
+ * The bits are swapped the following way:
+ *  Byte b include bits 0-7, nibble 1 (n1) include bits 0-3 and
+ *  nibble 2 (n2) bits 4-7.
+ *
+ *  Nibble 1 (n1):
+ *  (The "old" (moved) bit is replaced with a zero)
+ *  1. Move bit 6 and 7, 4 positions to the left.
+ *  2. Move bit 3 and 5, 2 positions to the left.
+ *  3. Move bit 1-4, 1 position to the left.
+ *
+ *  Nibble 2 (n2):
+ *  1. Move bit 0 and 1, 4 positions to the right.
+ *  2. Move bit 2 and 4, 2 positions to the right.
+ *  3. Move bit 3-6, 1 position to the right.
+ *
+ *  Combine the two nibbles to a complete and swapped byte.
+ */
+
+static inline u8 swap_bits_in_byte(u8 b)

can't exactly tell - is this because the h/w needs its keys
provided in bitwise-reverse order?  and for decryption only?

+{
+#define R_SHIFT_4_MASK  (0xc0) /* Bits 6 and 7, right shift 4 */
+#define R_SHIFT_2_MASK  (0x28) /* (After right shift 4) Bits 3 and 5,
+				  right shift 2 */
+#define R_SHIFT_1_MASK  (0x1e) /* (After right shift 2) Bits 1-4,
+				  right shift 1 */
+#define L_SHIFT_4_MASK  (0x03) /* Bits 0 and 1, left shift 4 */
+#define L_SHIFT_2_MASK  (0x14) /* (After left shift 4) Bits 2 and 4,
+				  left shift 2 */
+#define L_SHIFT_1_MASK  (0x78) /* (After left shift 1) Bits 3-6,
+				  left shift 1 */

unnecessary parens

+static irqreturn_t cryp_interrupt_handler(int irq, void *param)
+{
+	struct cryp_ctx *ctx;
+	int i;
+	struct cryp_device_data *device_data;
+
+	if (param == NULL) {
+		BUG_ON(!param);
+		return IRQ_HANDLED;
+	}

this makes no sense - plus, when would !param be true in the first
place?

+
+	/* The device is coming from the one found in hw_crypt_noxts. */
+	device_data = (struct cryp_device_data *)param;
+
+	ctx = device_data->current_ctx;
+
+	if (ctx == NULL) {
+		BUG_ON(!ctx);
+		return IRQ_HANDLED;
+	}

same here

+
+	dev_dbg(ctx->device->dev, "[%s] (len: %d) %s, ", __func__, ctx->outlen,
+		cryp_pending_irq_src(device_data, CRYP_IRQ_SRC_OUTPUT_FIFO) ?
+		"out" : "in");
+
+	if (cryp_pending_irq_src(device_data,
+				 CRYP_IRQ_SRC_OUTPUT_FIFO)) {
+		if (ctx->outlen / ctx->blocksize > 0) {
+			for (i = 0; i < ctx->blocksize / 4; i++) {
+				cryp_read_outdata(device_data,
+						 (u32 *)ctx->outdata);
+				ctx->outdata += 4;
+				ctx->outlen -= 4;
+			}
+
+			if (ctx->outlen == 0) {
+				cryp_disable_irq_src(device_data,
+						     CRYP_IRQ_SRC_OUTPUT_FIFO);
+			}
+		}
+	} else if (cryp_pending_irq_src(device_data,
+					CRYP_IRQ_SRC_INPUT_FIFO)) {
+		if (ctx->datalen / ctx->blocksize > 0) {
+			for (i = 0 ; i < ctx->blocksize / 4; i++) {
+				cryp_write_indata(device_data,
+						 *((u32 *)ctx->indata));
+				ctx->indata += 4;
+				ctx->datalen -= 4;
+			}
+
+			if (ctx->datalen == 0)
+				cryp_disable_irq_src(device_data,
+						   CRYP_IRQ_SRC_INPUT_FIFO);
+
+			if (ctx->config.algomode == CRYP_ALGO_AES_XTS) {
+				CRYP_PUT_BITS(&device_data->base->cr,
+					      CRYP_START_ENABLE,
+					      CRYP_CR_START_POS,
+					      CRYP_CR_START_MASK);
+
+				cryp_wait_until_done(device_data);
+			}
+		}
+	}
+
+	return IRQ_HANDLED;
+}

curious again - why are irq's being disabled in the IRQ handler, and
then only reenabled upon a new request - why can't they stay
enabled?  Is this due to the 'polling mode' option somehow not being
mutually exclusive with an interrupt-based request?

+static int mode_is_aes(enum cryp_algo_mode mode)
+{
+	return	(CRYP_ALGO_AES_ECB == mode) ||
+		(CRYP_ALGO_AES_CBC == mode) ||
+		(CRYP_ALGO_AES_CTR == mode) ||
+		(CRYP_ALGO_AES_XTS == mode);

unnecessary inner parens

+	if (ctx->updated == 0) {
+		cryp_flush_inoutfifo(device_data);
+		if (cfg_keys(ctx) != 0) {
+			dev_err(ctx->device->dev, "[%s]: cfg_keys failed!",
+				__func__);
+			return -EPERM;

-EINVAL?

+		}
+
+		if ((ctx->iv) &&
+		    (CRYP_ALGO_AES_ECB != ctx->config.algomode) &&
+		    (CRYP_ALGO_DES_ECB != ctx->config.algomode) &&
+		    (CRYP_ALGO_TDES_ECB != ctx->config.algomode)) {

unnecessary inner parens

+		if (!ctx->device->dma.sg_dst_len) {
+			dev_dbg(ctx->device->dev,
+				"[%s]: Could not map the sg list "
+				"(FROM_DEVICE)", __func__);

message text strings are allowed to exceed the 80 char line limit,
for grep-ability reasons.

+			regulator_disable(
+					       device_data->pwr_regulator);

join last 2 lines

+		/*
+		 * ctx->outlen is decremented in the cryp_interrupt_handler
+		 * function. We had to add cpu_relax() (barrier) to make sure
+		 * that gcc didn't optimze away this variable.

                                   optimize

+		 */
+		while (ctx->outlen > 0)
+			cpu_relax();

do we need a timeout check in case h/w goes in the weeds?

+static void aes_encrypt(struct crypto_tfm *tfm, u8 *out, const u8 *in)
+{
+	struct cryp_ctx *ctx = crypto_tfm_ctx(tfm);
+
+	pr_debug(DEV_DBG_NAME " [%s]", __func__);
+
+	ctx->blocksize = crypto_tfm_alg_blocksize(tfm);
+
+	ctx->config.algodir = CRYP_ALGORITHM_ENCRYPT;
+	ctx->config.algomode = CRYP_ALGO_AES_ECB;
+
+	ctx->indata = in;
+	ctx->outdata = out;
+	ctx->datalen = ctx->blocksize;
+
+	if (cryp_hw_calculate(ctx))
+		pr_err("ux500_cryp:crypX: [%s]: cryp_hw_calculate() failed!",
+				__func__);

shouldn't this error be propagated to the higher level API?

+}

+/**
+ * struct crypto_alg aes_alg
+ */
+static struct crypto_alg aes_alg = {
+	.cra_name		=	"aes",
+	.cra_driver_name	=	"aes-ux500",
+	.cra_priority		=	300,
+	.cra_flags		=	CRYPTO_ALG_TYPE_CIPHER,
+	.cra_blocksize		=	AES_BLOCK_SIZE,
+	.cra_ctxsize		=	sizeof(struct cryp_ctx),
+	.cra_alignmask		=	3,
+	.cra_module		=	THIS_MODULE,
+	.cra_list		=	LIST_HEAD_INIT(aes_alg.cra_list),
+	.cra_u			=	{
+		.cipher = {
+			.cia_min_keysize	=	AES_MIN_KEY_SIZE,
+			.cia_max_keysize	=	AES_MAX_KEY_SIZE,
+			.cia_setkey		=	aes_setkey,
+			.cia_encrypt		=	aes_encrypt,
+			.cia_decrypt		=	aes_decrypt
+		}
+	}
+};

each of the entry points already assign algodir, algomode, and
blocksize; why not add the corresponding values in a template struct
here, that embeds the crypto_alg structs instead (see struct
talitos_alg_template).  The code would be greatly simplified.
Don't know if it would help, but see also commit 4b00434 "crypto:
Add bulk algorithm registration interface" (in Herbert's crypto-2.6
tree).

Kim