| Message ID | 20180605134950.6605-6-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
Hi, Linus, On 06/05/2018 04:49 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> > --- > drivers/crypto/atmel-ecc.c | 71 ++++++++++++++++++-------------------- > drivers/crypto/atmel-ecc.h | 13 +++---- > 2 files changed, 40 insertions(+), 44 deletions(-) > > diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c > index d89b69d228ac..603e29bdcb97 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,8 +284,31 @@ static int atmel_ecc_send_receive(struct i2c_client *client, > goto err; > } > > + /* 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, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE); > + ret = i2c_master_send(client, buf, cmdlen); The struct atmel_ecc_cmd (__packed) is composed of u8 members with only 2 exceptions, u16 param2 and u16 crc that were written in little endian, as the device expects. The (u8 *) cast will point to the first member, which is u8 as well, so we're safe. I2C transfers are done at a per-byte level, so no problems here either. You are allocating a temporary buffer and duplicate the initialization, without an obvious benefit. Can you please explain what do want to fix or what are the potential problems? Thanks, 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 6d586a3e443d..2a378bccc213 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 > @@ -136,14 +137,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__ */ >
On Tue, Jun 12, 2018 at 12:19 PM Tudor Ambarus <tudor.ambarus@microchip.com> wrote: > The struct atmel_ecc_cmd (__packed) is composed of u8 members with only > 2 exceptions, u16 param2 and u16 crc that were written in little endian, > as the device expects. The (u8 *) cast will point to the first member, > which is u8 as well, so we're safe. I2C transfers are done at a per-byte > level, so no problems here either. This is not helpful for the developer, who has to start thinking like the computer before they understand what is going on. It makes the code hard to read IMHO. The i2c transfers are done at a byte level, but the code goes to call cpu_to_le16() to lay out the bytes in the struct in the right way which is confusing if you don't immediately know that the whole struct is going to be serialized. > You are allocating a temporary buffer and duplicate the initialization, > without an obvious benefit. Can you please explain what do want to fix > or what are the potential problems? The buffer is allocated on the stack, which is fine, this is definately not on any hotpath as we're dealing with i2c traffic in the end, but if you are worried about the buffer being reallocated every time we enter the function I can certainly mark it static. It is fundamentally about readability and code centralization, avoiding code duplication I would say. It is pretty hard to see how endianness and marshalling across to the chip happens unless the code dealing with that is collected in one spot. As it is now, that procedure is spread out, and it is hard to figure out how endianness is dealt with. First problem: we have several calls to atmel_ecc_checksum(cmd); spread out all over the place instead of doing the intuitive thing, which is to call that right before we send the command. This is code duplication. Second problem: we have two instances of inlined cmd->param2 = cpu_to_le16(keyid); dealing with endianness instead of doing this as part of marshalling the command. This is also code duplication. Yours, Linus Walleij
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c index d89b69d228ac..603e29bdcb97 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,8 +284,31 @@ static int atmel_ecc_send_receive(struct i2c_client *client, goto err; } + /* 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, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE); + 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 6d586a3e443d..2a378bccc213 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 @@ -136,14 +137,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__ */
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> --- drivers/crypto/atmel-ecc.c | 71 ++++++++++++++++++-------------------- drivers/crypto/atmel-ecc.h | 13 +++---- 2 files changed, 40 insertions(+), 44 deletions(-)