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