[PATCH 5/9 v2] crypto: atmel-ecc: Create a config word reader
Rewrite the function atmel_ecc_init_read_cmd() into a more
general atmel_ecc_init_read_config_word() function to read
any word from the configuration zone, and use this
parameterized with what we want to read out.
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Rebased
---
drivers/crypto/atmel-ecc.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 98f9322ca303..2ec570d06a27 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -136,16 +136,13 @@ static void atmel_ecc_checksum(struct atmel_ecc_cmd *cmd)
*crc16 = atmel_ecc_crc16(0, data, len);
}
-static void atmel_ecc_init_read_cmd(struct atmel_ecc_cmd *cmd)
+static void atmel_ecc_init_read_config_word(struct atmel_ecc_cmd *cmd,
+ u16 config_word)
{
cmd->word_addr = COMMAND;
cmd->opcode = OPCODE_READ;
- /*
-* Read the word from Configuration zone that contains the lock bytes
-* (UserExtra, Selector, LockValue, LockConfig).
-*/
cmd->param1 = CONFIG_ZONE;
- cmd->param2 = CONFIG_ZONE_FOOTER;
+ cmd->param2 = config_word;
cmd->count = READ_COUNT;
atmel_ecc_checksum(cmd);
@@ -626,7 +623,7 @@ static int device_sanity_check(struct i2c_client *client)
if (!cmd)
return -ENOMEM;
- atmel_ecc_init_read_cmd(cmd);
+ atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_FOOTER);
ret = atmel_ecc_send_receive(client, cmd);
if (ret) {
--
2.17.0
[PATCH 9/9 v2] crypto: atmel-ecc: Break out lock check helper
This breaks out a lock status checker to be used with further
refactorings.
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Rebased
---
drivers/crypto/atmel-ecc.c | 38 ++
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 0dfea6eadc56..549ec7190287 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -666,7 +666,9 @@ static int atmel_ecc_get_serial(struct i2c_client *client)
return ret;
}
-static int device_sanity_check(struct i2c_client *client)
+static int atmel_ecc_get_lock_status(struct i2c_client *client,
+bool *config_is_locked,
+bool *otp_data_is_locked)
{
struct atmel_ecc_cmd *cmd;
int ret;
@@ -680,21 +682,49 @@ static int device_sanity_check(struct i2c_client *client)
ret = atmel_ecc_send_receive(client, cmd);
if (ret) {
dev_err(>dev,
- "failed to send ECC init command\n");
+ "failed to send config read command\n");
goto free_cmd;
}
+ /* According to datasheet anything else than 0x55 means "locked" */
+ if (cmd->data[RSP_DATA_IDX+3] == 0x55)
+ *config_is_locked = false;
+ else
+ *config_is_locked = true;
+
+ if (cmd->data[RSP_DATA_IDX+2] == 0x55)
+ *otp_data_is_locked = false;
+ else
+ *otp_data_is_locked = true;
+
+free_cmd:
+ kfree(cmd);
+ return ret;
+}
+
+static int device_sanity_check(struct i2c_client *client)
+{
+ struct atmel_ecc_cmd *cmd;
+ bool config_locked;
+ bool otp_data_locked;
+ int ret;
+
/*
* It is vital that the Configuration, Data and OTP zones be locked
* prior to release into the field of the system containing the device.
* Failure to lock these zones may permit modification of any secret
* keys and may lead to other security problems.
*/
- if (cmd->data[RSP_DATA_IDX+3] == 0x55) {
+ ret = atmel_ecc_get_lock_status(client, _locked,
+ _data_locked);
+ if (ret)
+ return ret;
+
+ if (!config_locked) {
dev_err(>dev, "configuration zone is unlocked\n");
ret = -ENOTSUPP;
}
- if (cmd->data[RSP_DATA_IDX+2] == 0x55) {
+ if (!otp_data_locked) {
dev_err(>dev, "data and OTP zones are unlocked\n");
ret = -ENOTSUPP;
}
--
2.17.0
[PATCH 1/9 v2] crypto: atmel-ecc: Make available for other platforms
This is a pure I2C driver, and this device appears on the 96boards Secure96 mezzanine card, so we want to enable the driver on other devices. Cut the Kconfig limitations to Atmel SoC only. Reviewed-by: Tudor Ambarus Signed-off-by: Linus Walleij --- ChangeLog v1->v2: - Collect Tudor's review tag. --- drivers/crypto/Kconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index 43cccf6aff61..b4dd64d2d4ca 100644 --- a/drivers/crypto/Kconfig +++ b/drivers/crypto/Kconfig @@ -519,7 +519,6 @@ config CRYPTO_DEV_ATMEL_SHA config CRYPTO_DEV_ATMEL_ECC tristate "Support for Microchip / Atmel ECC hw accelerator" - depends on ARCH_AT91 || COMPILE_TEST depends on I2C select CRYPTO_ECDH select CRC16 -- 2.17.0
[PATCH 4/9 v2] crypto: atmel-ecc: Provide config zone defines
The config zone has 0x16 words of 4 bytes each, so provide
some basic defines so that we can address these individually.
Rename the last word to "footer", this is where we currently
look to see if the configuration is locked.
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Diet the defines down to the ones used in this patch
set.
---
drivers/crypto/atmel-ecc.c | 2 +-
drivers/crypto/atmel-ecc.h | 9 +++--
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index baef0d07164d..98f9322ca303 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -145,7 +145,7 @@ static void atmel_ecc_init_read_cmd(struct atmel_ecc_cmd
*cmd)
* (UserExtra, Selector, LockValue, LockConfig).
*/
cmd->param1 = CONFIG_ZONE;
- cmd->param2 = DEVICE_LOCK_ADDR;
+ cmd->param2 = CONFIG_ZONE_FOOTER;
cmd->count = READ_COUNT;
atmel_ecc_checksum(cmd);
diff --git a/drivers/crypto/atmel-ecc.h b/drivers/crypto/atmel-ecc.h
index 25232c8abcc2..d941c4f3d28f 100644
--- a/drivers/crypto/atmel-ecc.h
+++ b/drivers/crypto/atmel-ecc.h
@@ -89,8 +89,13 @@ static const struct {
#define RSP_DATA_IDX 1 /* buffer index of data in response */
#define DATA_SLOT_22 /* used for ECDH private key */
-/* Definitions for the device lock state */
-#define DEVICE_LOCK_ADDR 0x15
+/* Definitions for the configuration zone words, these are 4 bytes each */
+#define CONFIG_ZONE_SERIAL_0_3 0x00
+#define CONFIG_ZONE_REVISION 0x01
+#define CONFIG_ZONE_SERIAL_4_7 0x02
+#define CONFIG_ZONE_SERIAL_8_I2CEN 0x03
+#define CONFIG_ZONE_I2C_OTP0x04
+#define CONFIG_ZONE_FOOTER 0x15
#define LOCK_VALUE_IDX (RSP_DATA_IDX + 2)
#define LOCK_CONFIG_IDX(RSP_DATA_IDX + 3)
--
2.17.0
[PATCH 2/9 v2] crypto: atmel-ecc: Just warn on missing clock frequency
The Atmel ECC driver contains a check for the I2C bus clock
frequency, so as to check that the I2C adapter in use
satisfies the device specs.
If the device is connected to a device tree node that does not
contain a clock frequency setting, such as an I2C mux or gate,
this blocks the probe. Make the probe continue with a warning
if no clock frequency can be found, assuming it is safe.
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Instead of silently ignoring the missing clock frequency,
issue a warning and continue.
---
drivers/crypto/atmel-ecc.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index e66f18a0ddd0..33773920e4bf 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -659,12 +659,9 @@ static int atmel_ecc_probe(struct i2c_client *client,
ret = of_property_read_u32(client->adapter->dev.of_node,
"clock-frequency", _clk_rate);
- if (ret) {
- dev_err(dev, "of: failed to read clock-frequency property\n");
- return ret;
- }
-
- if (bus_clk_rate > 100L) {
+ if (ret)
+ dev_warn(dev, "i2c host missing clock frequency information\n");
+ else if (bus_clk_rate > 100L) {
dev_err(dev, "%d exceeds maximum supported clock frequency
(1MHz)\n",
bus_clk_rate);
return -EINVAL;
--
2.17.0
[PATCH 8/9 v2] crypto: atmel-ecc: Detail what is unlocked
Instead of just providing a broad error message about the
chip being unlocked provide details on what is unlocked,
one line per thing that can be locked: data and OTP and
configuration are locked independently. Loose the
overzealous defines.
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Rebased
---
drivers/crypto/atmel-ecc.c | 8 ++--
drivers/crypto/atmel-ecc.h | 2 --
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 659cb3cf37a9..0dfea6eadc56 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -690,8 +690,12 @@ static int device_sanity_check(struct i2c_client *client)
* Failure to lock these zones may permit modification of any secret
* keys and may lead to other security problems.
*/
- if (cmd->data[LOCK_CONFIG_IDX] || cmd->data[LOCK_VALUE_IDX]) {
- dev_err(>dev, "Configuration or Data and OTP zones are
unlocked!\n");
+ if (cmd->data[RSP_DATA_IDX+3] == 0x55) {
+ dev_err(>dev, "configuration zone is unlocked\n");
+ ret = -ENOTSUPP;
+ }
+ if (cmd->data[RSP_DATA_IDX+2] == 0x55) {
+ dev_err(>dev, "data and OTP zones are unlocked\n");
ret = -ENOTSUPP;
}
diff --git a/drivers/crypto/atmel-ecc.h b/drivers/crypto/atmel-ecc.h
index 4458585ab588..42b724256f93 100644
--- a/drivers/crypto/atmel-ecc.h
+++ b/drivers/crypto/atmel-ecc.h
@@ -97,8 +97,6 @@ static const struct {
#define CONFIG_ZONE_SERIAL_8_I2CEN 0x03
#define CONFIG_ZONE_I2C_OTP0x04
#define CONFIG_ZONE_FOOTER 0x15
-#define LOCK_VALUE_IDX (RSP_DATA_IDX + 2)
-#define LOCK_CONFIG_IDX(RSP_DATA_IDX + 3)
/*
* Wake High delay to data communication (microseconds). SDA should be stable
--
2.17.0
[PATCH 6/9 v2] crypto: atmel-ecc: Marshal the command while sending
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
---
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 = >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(_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
[PATCH 7/9 v2] crypto: atmel-ecc: Print out serial number
This reads out the serial number of the crypto chip and prints it,
also toss this into the entropy pool as it is device-unique data.
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- kfree(cmd) was missed. Fix it with a goto construction.
- Coding style fixes.
---
drivers/crypto/atmel-ecc.c | 60 ++
1 file changed, 60 insertions(+)
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index f3322fae454e..659cb3cf37a9 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -610,6 +611,61 @@ static inline size_t atmel_ecc_wake_token_sz(u32
bus_clk_rate)
return DIV_ROUND_UP(no_of_bits, 8);
}
+static int atmel_ecc_get_serial(struct i2c_client *client)
+{
+ struct atmel_ecc_cmd *cmd;
+ int ret;
+ int i;
+ u8 serial[9];
+
+ cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
+ if (!cmd)
+ return -ENOMEM;
+
+ atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_0_3);
+ ret = atmel_ecc_send_receive(client, cmd);
+ if (ret) {
+ dev_err(>dev,
+ "failed to read serial byte 0-3\n");
+ goto out_free_cmd;
+ }
+ for (i = 0; i < 4; i++)
+ serial[i] = cmd->data[RSP_DATA_IDX + i];
+
+ atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_4_7);
+ ret = atmel_ecc_send_receive(client, cmd);
+ if (ret) {
+ dev_err(>dev,
+ "failed to read serial byte 4-7\n");
+ goto out_free_cmd;
+ }
+ for (i = 0; i < 4; i++)
+ serial[4 + i] = cmd->data[RSP_DATA_IDX + i];
+
+
+ atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_8_I2CEN);
+ ret = atmel_ecc_send_receive(client, cmd);
+ if (ret) {
+ kfree(cmd);
+ dev_err(>dev,
+ "failed to read serial byte 8\n");
+ goto out_free_cmd;
+ }
+ serial[8] = cmd->data[RSP_DATA_IDX];
+
+ /* This is device-unique data so it goes into the entropy pool */
+ add_device_randomness(serial, sizeof(serial));
+
+ dev_info(>dev,
+"serial number: %02x%02x%02x%02x%02x%02x%02x%02x%02x\n",
+serial[0], serial[1], serial[2], serial[3], serial[4],
+serial[5], serial[6], serial[7], serial[8]);
+
+out_free_cmd:
+ kfree(cmd);
+ return ret;
+}
+
static int device_sanity_check(struct i2c_client *client)
{
struct atmel_ecc_cmd *cmd;
@@ -692,6 +748,10 @@ static int atmel_ecc_probe(struct i2c_client *client,
if (ret)
return ret;
+ ret = atmel_ecc_get_serial(client);
+ if (ret)
+ return ret;
+
spin_lock(_data.i2c_list_lock);
list_add_tail(_priv->i2c_client_list_node,
_data.i2c_client_list);
--
2.17.0
[PATCH 3/9 v2] crypto: atmel-ecc: More helpful error messages
Report errors once when they happen on the I2C bus so we
get good information in cases such as when the wrong I2C
address is used.
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Strip some comments that are now obvious from the context
with the error messages.
- Do not print the excess ECC error message, atmel_ecc_status()
already prints error messages.
---
drivers/crypto/atmel-ecc.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 33773920e4bf..baef0d07164d 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -310,26 +310,31 @@ static int atmel_ecc_send_receive(struct i2c_client
*client,
mutex_lock(_priv->lock);
ret = atmel_ecc_wakeup(client);
- if (ret)
+ if (ret) {
+ dev_err(>dev, "wakeup failed\n");
goto err;
+ }
- /* send the command */
ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(>dev, "command send failed\n");
goto err;
+ }
/* delay the appropriate amount of time for command to execute */
msleep(cmd->msecs);
- /* receive the response */
ret = i2c_master_recv(client, cmd->data, cmd->rxsize);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(>dev, "getting response failed\n");
goto err;
+ }
- /* put the device into low-power mode */
ret = atmel_ecc_sleep(client);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(>dev, "putting to sleep failed\n");
goto err;
+ }
mutex_unlock(_priv->lock);
return atmel_ecc_status(>dev, cmd->data);
@@ -624,8 +629,11 @@ static int device_sanity_check(struct i2c_client *client)
atmel_ecc_init_read_cmd(cmd);
ret = atmel_ecc_send_receive(client, cmd);
- if (ret)
+ if (ret) {
+ dev_err(>dev,
+ "failed to send ECC init command\n");
goto free_cmd;
+ }
/*
* It is vital that the Configuration, Data and OTP zones be locked
--
2.17.0
Re: [PATCH 4/9] crypto: atmel-ecc: Provide config zone defines
On Tue, Jun 12, 2018 at 2:59 PM Tudor Ambarus wrote: > On 06/05/2018 04:49 PM, Linus Walleij wrote: > > The config zone has 0x16 words of 4 bytes each, so provide > > some basic defines so that we can address these individually. > > Are you going to use all these defines? I would add just the defines > that are needed, when they are needed, but I guess it's a matter of > preference. I can cut it down to the ones I actually use in the subsequent patches, but I'd like to keep it in a separate patch so as not to disturb the order of the patch series. I can do like you say and add it when adding users in future patches. Yours, Linus Walleij
Re: [PATCH 8/9] crypto: atmel-ecc: Detail what is unlocked
On Tue, Jun 12, 2018 at 3:25 PM Tudor Ambarus wrote: > On 06/05/2018 04:49 PM, Linus Walleij wrote: > > Instead of just providing a broad error message about the > > chip being unlocked provide details on what is unlocked, > > one line per thing that can be locked: data and OTP and > > configuration are locked independently. Loose the > > Failure to lock these zones may permit modification of secret keys, so > we don't permit using the device if any of these zones is unlocked. > Why do you think it's relevant to indicate which zone is unlocked? It is always nice for the user to know exactly what the problem is. Instead of "there is some problem with locking" they now know what locking the problem is about: the first, the second or both. Yours, Linus Walleij
Re: [PATCH 7/9] crypto: atmel-ecc: Print out serial number
On Tue, Jun 12, 2018 at 3:13 PM Tudor Ambarus
wrote:
> > +#include
>
> includes should be ordered alphabetically.
OK fixed.
> > +static int atmel_ecc_get_serial(struct i2c_client *client)
> > +{
> > + struct atmel_ecc_cmd *cmd;
> > + u8 serial[9];
>
> int i, ret; before serial to avoid stack padding.
>
> > + int ret;
> > + int i;
Is your point trying to help the compiler to lay things out on
the stack? Isn't that premature optimization? I was under the
impression that we should leave this stuff to the compiler.
But sure, it is a small change so I did it anyways :)
> > +
> > + cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
> > + if (!cmd)
> > + return -ENOMEM;
> > +
> > + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_0_3);
> > + ret = atmel_ecc_send_receive(client, cmd);
> > + if (ret) {
>
> free(cmd);
Whoops added a goto contruction to do proper free:ing of
cmd, thanks.
> > + dev_err(>dev,
> > + "failed to read serial byte 0-3\n");
> > + return ret;
> > + }
> > + for (i = 0; i < 4; i++)
> > + serial[0+i] = cmd->data[RSP_DATA_IDX + i];
>
> serial[i]?
OK.
> > + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_4_7);
>
> can't we read the serial_no once?
Sorry not following. Do you mean we need an accessor to
read several config words in succession? I think I read somewhere
that the device only supports reading one config word at the
time and the serial number is spread out over three config words...
> > + for (i = 0; i < 4; i++)
> > + serial[4+i] = cmd->data[RSP_DATA_IDX + i];
>
> spaces preferred around that '+'
OK.
> > + dev_info(>dev,
> > + "serial number: %02x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> > + serial[0], serial[1], serial[2], serial[3], serial[4],
> > + serial[5], serial[6], serial[7], serial[8]);
>
> Why do you need the serial number printed out?
Cuddly feeling I guess. If there is a problem with the device the
user might want to report the serial number to the
manufacturer?
Yours,
Linus Walleij
Re: [PATCH 6/9] crypto: atmel-ecc: Marshal the command while sending
On Tue, Jun 12, 2018 at 12:19 PM Tudor Ambarus 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
Re: [PATCH 3/9] crypto: atmel-ecc: More helpful error messages
On Tue, Jun 12, 2018 at 2:36 PM Tudor Ambarus
wrote:
> On 06/05/2018 04:49 PM, Linus Walleij wrote:
> > /* send the command */
>
> I guess that this comment will become superfluous if you're going to add
> an error message.
OK stripped obvious comments.
> > - return atmel_ecc_status(>dev, cmd->data);
> > + ret = atmel_ecc_status(>dev, cmd->data);
>
> atmel_ecc_status already prints errors when needed.
OK skipped this change.
> > ret = atmel_ecc_send_receive(client, cmd);
> > - if (ret)
> > + if (ret) {
> > + dev_err(>dev,
> > + "failed to send ECC init command\n");
>
> Here we will have two errors reported, the first being from the
> atmel_ecc_send_receive(). I would go with just an error reported. Do we
> really care what happened with the i2c transfer, or it's enough to
> report that the init failed?
The more help the better.
I think it is relevant to have both: you will read the log
and say "OK init failed because this I2C transfer is not
getting across as it should", that is helpful.
Yours,
Linus Walleij
Re: [PATCH 2/9] crypto: atmel-ecc: Silently ignore missing clock frequency
On Mon, Jun 11, 2018 at 11:46 AM Tudor Ambarus wrote: > On 06/05/2018 04:49 PM, Linus Walleij wrote: > > The Atmel ECC driver contains a check for the I2C bus clock > > frequency, so as to check that the I2C adapter in use > > satisfies the device specs. > > > > If the device is connected to a device tree node that does not > > contain a clock frequency setting, such as an I2C mux or gate, > > this blocks the probe. Make the probe continue silently if > > no clock frequency can be found, assuming all is safe. > > I don't think it's safe. We use bus_clk_rate to compute the ecc508's > wake token. If you can't wake the device, it will ignore all your > commands. I see. I wonder why it works so well for me then? Could we just print a warning and continue? The general advice for the kernel is not to bail out if hardware is not optimally configured, but continue with a warning that maybe not everything is all right. > My proposal was to introduce a bus_freq_hz member in the i2c_adapter > structure (https://patchwork.kernel.org/patch/10307637/), but I haven't > received feedback yet, Wolfram? > > I would say first to check the bus_freq_hz from adapter (if it will be > accepted) else to look into dt and, in the last case, if nowhere > provided, to block the probe. So blocking the probe because we are not 100% sure the hardware will work is against established practice: the established practice is to be liberal with letting probe() continue, but emit a warning. In my case that is good because it makes the hardware probe and work without any visible problems. I *can* try to traverse the device tree upwards from the mux node to find the parent I2C controller and its "clock-frequency" property. I felt that was hackish, but if you prefer the hack I can try to use that. Yours, Linus Walleij
