[3/9,v2] crypto: atmel-ecc: More helpful error messages

Message ID 20180628130729.17589-3-linus.walleij@linaro.org
State New
Headers show
Series
  • [1/9,v2] crypto: atmel-ecc: Make available for other platforms
Related show

Commit Message

Linus Walleij June 28, 2018, 1:07 p.m.
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 <linus.walleij@linaro.org>

---
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(-)

-- 
2.17.0

Comments

Tudor Ambarus July 2, 2018, 10:01 a.m. | #1
Hi, Linus,

On 06/28/2018 04:07 PM, Linus Walleij wrote:
> 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 <linus.walleij@linaro.org>

> ---

> 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(&i2c_priv->lock);

>  

>  	ret = atmel_ecc_wakeup(client);

> -	if (ret)

> +	if (ret) {

> +		dev_err(&client->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(&client->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(&client->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(&client->dev, "putting to sleep failed\n");

>  		goto err;

> +	}

>  

>  	mutex_unlock(&i2c_priv->lock);

>  	return atmel_ecc_status(&client->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(&client->dev,


If you want to have two errors reported, one related to i2c and the other to
crypto, I propose to be consistent and add an error message for the other call
to atmel_ecc_send_receive() in atmel_ecdh_set_secret().


> +			"failed to send ECC init command\n");


This line fits well in the previous. I would remove "ECC" from it, looks a bit
redundant:
atmel-ecc 2-0058: failed to send ECC init command

Thanks!
ta

>  		goto free_cmd;

> +	}

>  

>  	/*

>  	 * It is vital that the Configuration, Data and OTP zones be locked

>

Patch

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(&i2c_priv->lock);
 
 	ret = atmel_ecc_wakeup(client);
-	if (ret)
+	if (ret) {
+		dev_err(&client->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(&client->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(&client->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(&client->dev, "putting to sleep failed\n");
 		goto err;
+	}
 
 	mutex_unlock(&i2c_priv->lock);
 	return atmel_ecc_status(&client->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(&client->dev,
+			"failed to send ECC init command\n");
 		goto free_cmd;
+	}
 
 	/*
 	 * It is vital that the Configuration, Data and OTP zones be locked