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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> ---
> 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__ */
> 



[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux