diff mbox series

[6/9,v2] crypto: atmel-ecc: Marshal the command while sending

Message ID 20180628130729.17589-6-linus.walleij@linaro.org
State New
Headers show
Series [1/9,v2] crypto: atmel-ecc: Make available for other platforms | expand

Commit Message

Linus Walleij June 28, 2018, 1:07 p.m. UTC 
Instead of casting the struct for the command into (u8 *)
which is problematic in many ways, and instead of
calculating the CRC sum in a separate function, marshal,
checksum and send the command in one single function.

Instead of providing the length of the whole command
in defines, it makes more sense to provide the length
of the data buffer used with the command.

This avoids the hazzle to try to keep the command
structure in the device endianness, we fix up the
endianness when marshalling the command instead.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v1->v2:
- Rebase on other changes.
---
 drivers/crypto/atmel-ecc.c | 72 ++++++++++++++++++--------------------
 drivers/crypto/atmel-ecc.h | 13 +++----
 2 files changed, 41 insertions(+), 44 deletions(-)

-- 
2.17.0

Comments

Tudor Ambarus July 2, 2018, 11:01 a.m. UTC | #1 
Hi, Linus,

On 06/28/2018 04:07 PM, Linus Walleij wrote:
> Instead of casting the struct for the command into (u8 *)

> which is problematic in many ways, and instead of

> calculating the CRC sum in a separate function, marshal,

> checksum and send the command in one single function.

> 

> Instead of providing the length of the whole command

> in defines, it makes more sense to provide the length

> of the data buffer used with the command.

> 

> This avoids the hazzle to try to keep the command

> structure in the device endianness, we fix up the

> endianness when marshalling the command instead.

> 

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog v1->v2:

> - Rebase on other changes.

> ---

>  drivers/crypto/atmel-ecc.c | 72 ++++++++++++++++++--------------------

>  drivers/crypto/atmel-ecc.h | 13 +++----

>  2 files changed, 41 insertions(+), 44 deletions(-)

> 

> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c

> index 2ec570d06a27..f3322fae454e 100644

> --- a/drivers/crypto/atmel-ecc.c

> +++ b/drivers/crypto/atmel-ecc.c

> @@ -113,29 +113,6 @@ struct atmel_ecc_work_data {

>  	struct atmel_ecc_cmd cmd;

>  };

>  

> -static u16 atmel_ecc_crc16(u16 crc, const u8 *buffer, size_t len)

> -{

> -	return cpu_to_le16(bitrev16(crc16(crc, buffer, len)));

> -}

> -

> -/**

> - * atmel_ecc_checksum() - Generate 16-bit CRC as required by ATMEL ECC.

> - * CRC16 verification of the count, opcode, param1, param2 and data bytes.

> - * The checksum is saved in little-endian format in the least significant

> - * two bytes of the command. CRC polynomial is 0x8005 and the initial register

> - * value should be zero.

> - *

> - * @cmd : structure used for communicating with the device.

> - */

> -static void atmel_ecc_checksum(struct atmel_ecc_cmd *cmd)

> -{

> -	u8 *data = &cmd->count;

> -	size_t len = cmd->count - CRC_SIZE;

> -	u16 *crc16 = (u16 *)(data + len);

> -

> -	*crc16 = atmel_ecc_crc16(0, data, len);

> -}

> -

>  static void atmel_ecc_init_read_config_word(struct atmel_ecc_cmd *cmd,

>  					    u16 config_word)

>  {

> @@ -143,10 +120,7 @@ static void atmel_ecc_init_read_config_word(struct atmel_ecc_cmd *cmd,

>  	cmd->opcode = OPCODE_READ;

>  	cmd->param1 = CONFIG_ZONE;

>  	cmd->param2 = config_word;

> -	cmd->count = READ_COUNT;

> -

> -	atmel_ecc_checksum(cmd);

> -

> +	cmd->datasz = READ_DATASZ;

>  	cmd->msecs = MAX_EXEC_TIME_READ;

>  	cmd->rxsize = READ_RSP_SIZE;

>  }

> @@ -154,14 +128,11 @@ static void atmel_ecc_init_read_config_word(struct atmel_ecc_cmd *cmd,

>  static void atmel_ecc_init_genkey_cmd(struct atmel_ecc_cmd *cmd, u16 keyid)

>  {

>  	cmd->word_addr = COMMAND;

> -	cmd->count = GENKEY_COUNT;

> +	cmd->datasz = GENKEY_DATASZ;

>  	cmd->opcode = OPCODE_GENKEY;

>  	cmd->param1 = GENKEY_MODE_PRIVATE;

>  	/* a random private key will be generated and stored in slot keyID */

> -	cmd->param2 = cpu_to_le16(keyid);

> -

> -	atmel_ecc_checksum(cmd);

> -

> +	cmd->param2 = keyid;

>  	cmd->msecs = MAX_EXEC_TIME_GENKEY;

>  	cmd->rxsize = GENKEY_RSP_SIZE;

>  }

> @@ -172,11 +143,11 @@ static int atmel_ecc_init_ecdh_cmd(struct atmel_ecc_cmd *cmd,

>  	size_t copied;

>  

>  	cmd->word_addr = COMMAND;

> -	cmd->count = ECDH_COUNT;

> +	cmd->datasz = ECDH_DATASZ;

>  	cmd->opcode = OPCODE_ECDH;

>  	cmd->param1 = ECDH_PREFIX_MODE;

>  	/* private key slot */

> -	cmd->param2 = cpu_to_le16(DATA_SLOT_2);

> +	cmd->param2 = DATA_SLOT_2;

>  

>  	/*

>  	 * The device only supports NIST P256 ECC keys. The public key size will

> @@ -186,9 +157,6 @@ static int atmel_ecc_init_ecdh_cmd(struct atmel_ecc_cmd *cmd,

>  	copied = sg_copy_to_buffer(pubkey, 1, cmd->data, ATMEL_ECC_PUBKEY_SIZE);

>  	if (copied != ATMEL_ECC_PUBKEY_SIZE)

>  		return -EINVAL;

> -

> -	atmel_ecc_checksum(cmd);

> -

>  	cmd->msecs = MAX_EXEC_TIME_ECDH;

>  	cmd->rxsize = ECDH_RSP_SIZE;

>  

> @@ -302,7 +270,11 @@ static int atmel_ecc_send_receive(struct i2c_client *client,

>  				  struct atmel_ecc_cmd *cmd)

>  {

>  	struct atmel_ecc_i2c_client_priv *i2c_priv = i2c_get_clientdata(client);

> +	u8 buf[MAX_CMD_SIZE];

> +	u16 cmdcrc;

> +	u8 cmdlen;

>  	int ret;

> +	int i;

>  

>  	mutex_lock(&i2c_priv->lock);

>  

> @@ -312,7 +284,31 @@ static int atmel_ecc_send_receive(struct i2c_client *client,

>  		goto err;

>  	}

>  

> -	ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);

> +	/* Marshal the command */

> +	cmdlen = 6 + cmd->datasz + CRC_SIZE;

> +	buf[0] = cmd->word_addr;

> +	/* This excludes the word address, includes CRC */

> +	buf[1] = cmdlen - 1;

> +	buf[2] = cmd->opcode;

> +	buf[3] = cmd->param1;

> +	/* Enforce little-endian byte order */

> +	buf[4] = cmd->param2 & 0xff;

> +	buf[5] = (cmd->param2 >> 8);

> +	/* Copy over the data array */

> +	for (i = 0; i < cmd->datasz; i++)

> +		buf[6+i] = cmd->data[i];

> +	/*

> +	 * CRC sum the command, do not include word addr or CRC. The

> +	 * bit order in the CRC16 algorithm inside the chip is reversed,

> +	 * so we need to swizzle the bits with bitrev16().

> +	 */

> +	cmdcrc = bitrev16(crc16(0x0000, buf+1, cmdlen - 1 - CRC_SIZE));

> +	/* Enforce little-endian byte order */

> +	buf[6+i] = (cmdcrc & 0xff);

> +	buf[6+i+1] = (cmdcrc >> 8);

> +

> +	/* send the command */

> +	ret = i2c_master_send(client, buf, cmdlen);


Maybe it's just me, but I don't like this because you are allocating a temporary
buffer and do the initialization all over again. A compromise would be to init
those two fields here, directly on the received cmd buffer, without allocating a
temporary one. But then, atmel_ecc_send_receive() should be renamed to
atmel_ecc_init_send_receive(), or something similar, because you are also doing
initialization in it. Your reasons were code readability, centralization and
avoiding code duplication. cmd->word_addr = COMMAND; is common for all the init
cmds, the word_addr field should also be set once in atmel_ecc_send_receive(),
right?

I like it better how the code is now, but I guess it's Herbert's decision.

Thanks, Linus,
ta

>  	if (ret < 0) {

>  		dev_err(&client->dev, "command send failed\n");

>  		goto err;

> diff --git a/drivers/crypto/atmel-ecc.h b/drivers/crypto/atmel-ecc.h

> index d941c4f3d28f..4458585ab588 100644

> --- a/drivers/crypto/atmel-ecc.h

> +++ b/drivers/crypto/atmel-ecc.h

> @@ -41,29 +41,30 @@

>  					 CMD_OVERHEAD_SIZE)

>  #define READ_RSP_SIZE			(4 + CMD_OVERHEAD_SIZE)

>  #define MAX_RSP_SIZE			GENKEY_RSP_SIZE

> +#define MAX_CMD_SIZE			(9 + MAX_RSP_SIZE)

>  

>  /**

>   * atmel_ecc_cmd - structure used for communicating with the device.

>   * @word_addr: indicates the function of the packet sent to the device. This

>   *             byte should have a value of COMMAND for normal operation.

> - * @count    : number of bytes to be transferred to (or from) the device.

>   * @opcode   : the command code.

>   * @param1   : the first parameter; always present.

>   * @param2   : the second parameter; always present.

> + * @datasz   : size of the data field

>   * @data     : optional remaining input data. Includes a 2-byte CRC.

>   * @rxsize   : size of the data received from i2c client.

>   * @msecs    : command execution time in milliseconds

>   */

>  struct atmel_ecc_cmd {

>  	u8 word_addr;

> -	u8 count;

>  	u8 opcode;

>  	u8 param1;

>  	u16 param2;

> +	u8 datasz;

>  	u8 data[MAX_RSP_SIZE];

>  	u8 msecs;

>  	u16 rxsize;

> -} __packed;

> +};

>  

>  /* Status/Error codes */

>  #define STATUS_SIZE			0x04

> @@ -120,14 +121,14 @@ static const struct {

>  #define OPCODE_READ			0x02

>  

>  /* Definitions for the READ Command */

> -#define READ_COUNT			7

> +#define READ_DATASZ			0

>  

>  /* Definitions for the GenKey Command */

> -#define GENKEY_COUNT			7

> +#define GENKEY_DATASZ			0

>  #define GENKEY_MODE_PRIVATE		0x04

>  

>  /* Definitions for the ECDH Command */

> -#define ECDH_COUNT			71

> +#define ECDH_DATASZ			64

>  #define ECDH_PREFIX_MODE		0x00

>  

>  #endif /* __ATMEL_ECC_H__ */

>
diff mbox series

Patch

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 2ec570d06a27..f3322fae454e 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -113,29 +113,6 @@  struct atmel_ecc_work_data {
 	struct atmel_ecc_cmd cmd;
 };
 
-static u16 atmel_ecc_crc16(u16 crc, const u8 *buffer, size_t len)
-{
-	return cpu_to_le16(bitrev16(crc16(crc, buffer, len)));
-}
-
-/**
- * atmel_ecc_checksum() - Generate 16-bit CRC as required by ATMEL ECC.
- * CRC16 verification of the count, opcode, param1, param2 and data bytes.
- * The checksum is saved in little-endian format in the least significant
- * two bytes of the command. CRC polynomial is 0x8005 and the initial register
- * value should be zero.
- *
- * @cmd : structure used for communicating with the device.
- */
-static void atmel_ecc_checksum(struct atmel_ecc_cmd *cmd)
-{
-	u8 *data = &cmd->count;
-	size_t len = cmd->count - CRC_SIZE;
-	u16 *crc16 = (u16 *)(data + len);
-
-	*crc16 = atmel_ecc_crc16(0, data, len);
-}
-
 static void atmel_ecc_init_read_config_word(struct atmel_ecc_cmd *cmd,
 					    u16 config_word)
 {
@@ -143,10 +120,7 @@  static void atmel_ecc_init_read_config_word(struct atmel_ecc_cmd *cmd,
 	cmd->opcode = OPCODE_READ;
 	cmd->param1 = CONFIG_ZONE;
 	cmd->param2 = config_word;
-	cmd->count = READ_COUNT;
-
-	atmel_ecc_checksum(cmd);
-
+	cmd->datasz = READ_DATASZ;
 	cmd->msecs = MAX_EXEC_TIME_READ;
 	cmd->rxsize = READ_RSP_SIZE;
 }
@@ -154,14 +128,11 @@  static void atmel_ecc_init_read_config_word(struct atmel_ecc_cmd *cmd,
 static void atmel_ecc_init_genkey_cmd(struct atmel_ecc_cmd *cmd, u16 keyid)
 {
 	cmd->word_addr = COMMAND;
-	cmd->count = GENKEY_COUNT;
+	cmd->datasz = GENKEY_DATASZ;
 	cmd->opcode = OPCODE_GENKEY;
 	cmd->param1 = GENKEY_MODE_PRIVATE;
 	/* a random private key will be generated and stored in slot keyID */
-	cmd->param2 = cpu_to_le16(keyid);
-
-	atmel_ecc_checksum(cmd);
-
+	cmd->param2 = keyid;
 	cmd->msecs = MAX_EXEC_TIME_GENKEY;
 	cmd->rxsize = GENKEY_RSP_SIZE;
 }
@@ -172,11 +143,11 @@  static int atmel_ecc_init_ecdh_cmd(struct atmel_ecc_cmd *cmd,
 	size_t copied;
 
 	cmd->word_addr = COMMAND;
-	cmd->count = ECDH_COUNT;
+	cmd->datasz = ECDH_DATASZ;
 	cmd->opcode = OPCODE_ECDH;
 	cmd->param1 = ECDH_PREFIX_MODE;
 	/* private key slot */
-	cmd->param2 = cpu_to_le16(DATA_SLOT_2);
+	cmd->param2 = DATA_SLOT_2;
 
 	/*
 	 * The device only supports NIST P256 ECC keys. The public key size will
@@ -186,9 +157,6 @@  static int atmel_ecc_init_ecdh_cmd(struct atmel_ecc_cmd *cmd,
 	copied = sg_copy_to_buffer(pubkey, 1, cmd->data, ATMEL_ECC_PUBKEY_SIZE);
 	if (copied != ATMEL_ECC_PUBKEY_SIZE)
 		return -EINVAL;
-
-	atmel_ecc_checksum(cmd);
-
 	cmd->msecs = MAX_EXEC_TIME_ECDH;
 	cmd->rxsize = ECDH_RSP_SIZE;
 
@@ -302,7 +270,11 @@  static int atmel_ecc_send_receive(struct i2c_client *client,
 				  struct atmel_ecc_cmd *cmd)
 {
 	struct atmel_ecc_i2c_client_priv *i2c_priv = i2c_get_clientdata(client);
+	u8 buf[MAX_CMD_SIZE];
+	u16 cmdcrc;
+	u8 cmdlen;
 	int ret;
+	int i;
 
 	mutex_lock(&i2c_priv->lock);
 
@@ -312,7 +284,31 @@  static int atmel_ecc_send_receive(struct i2c_client *client,
 		goto err;
 	}
 
-	ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);
+	/* Marshal the command */
+	cmdlen = 6 + cmd->datasz + CRC_SIZE;
+	buf[0] = cmd->word_addr;
+	/* This excludes the word address, includes CRC */
+	buf[1] = cmdlen - 1;
+	buf[2] = cmd->opcode;
+	buf[3] = cmd->param1;
+	/* Enforce little-endian byte order */
+	buf[4] = cmd->param2 & 0xff;
+	buf[5] = (cmd->param2 >> 8);
+	/* Copy over the data array */
+	for (i = 0; i < cmd->datasz; i++)
+		buf[6+i] = cmd->data[i];
+	/*
+	 * CRC sum the command, do not include word addr or CRC. The
+	 * bit order in the CRC16 algorithm inside the chip is reversed,
+	 * so we need to swizzle the bits with bitrev16().
+	 */
+	cmdcrc = bitrev16(crc16(0x0000, buf+1, cmdlen - 1 - CRC_SIZE));
+	/* Enforce little-endian byte order */
+	buf[6+i] = (cmdcrc & 0xff);
+	buf[6+i+1] = (cmdcrc >> 8);
+
+	/* send the command */
+	ret = i2c_master_send(client, buf, cmdlen);
 	if (ret < 0) {
 		dev_err(&client->dev, "command send failed\n");
 		goto err;
diff --git a/drivers/crypto/atmel-ecc.h b/drivers/crypto/atmel-ecc.h
index d941c4f3d28f..4458585ab588 100644
--- a/drivers/crypto/atmel-ecc.h
+++ b/drivers/crypto/atmel-ecc.h
@@ -41,29 +41,30 @@ 
 					 CMD_OVERHEAD_SIZE)
 #define READ_RSP_SIZE			(4 + CMD_OVERHEAD_SIZE)
 #define MAX_RSP_SIZE			GENKEY_RSP_SIZE
+#define MAX_CMD_SIZE			(9 + MAX_RSP_SIZE)
 
 /**
  * atmel_ecc_cmd - structure used for communicating with the device.
  * @word_addr: indicates the function of the packet sent to the device. This
  *             byte should have a value of COMMAND for normal operation.
- * @count    : number of bytes to be transferred to (or from) the device.
  * @opcode   : the command code.
  * @param1   : the first parameter; always present.
  * @param2   : the second parameter; always present.
+ * @datasz   : size of the data field
  * @data     : optional remaining input data. Includes a 2-byte CRC.
  * @rxsize   : size of the data received from i2c client.
  * @msecs    : command execution time in milliseconds
  */
 struct atmel_ecc_cmd {
 	u8 word_addr;
-	u8 count;
 	u8 opcode;
 	u8 param1;
 	u16 param2;
+	u8 datasz;
 	u8 data[MAX_RSP_SIZE];
 	u8 msecs;
 	u16 rxsize;
-} __packed;
+};
 
 /* Status/Error codes */
 #define STATUS_SIZE			0x04
@@ -120,14 +121,14 @@  static const struct {
 #define OPCODE_READ			0x02
 
 /* Definitions for the READ Command */
-#define READ_COUNT			7
+#define READ_DATASZ			0
 
 /* Definitions for the GenKey Command */
-#define GENKEY_COUNT			7
+#define GENKEY_DATASZ			0
 #define GENKEY_MODE_PRIVATE		0x04
 
 /* Definitions for the ECDH Command */
-#define ECDH_COUNT			71
+#define ECDH_DATASZ			64
 #define ECDH_PREFIX_MODE		0x00
 
 #endif /* __ATMEL_ECC_H__ */