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