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

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

Commit Message

Linus Walleij June 5, 2018, 1:49 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>

---
 drivers/crypto/atmel-ecc.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

-- 
2.17.0

Comments

Tudor Ambarus June 12, 2018, 12:36 p.m. | #1
Hi, Linus,

On 06/05/2018 04:49 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>

> ---

>   drivers/crypto/atmel-ecc.c | 27 +++++++++++++++++++++------

>   1 file changed, 21 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c

> index 145ab3a39a56..214b0572bf8b 100644

> --- a/drivers/crypto/atmel-ecc.c

> +++ b/drivers/crypto/atmel-ecc.c

> @@ -310,29 +310,41 @@ 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 */


I guess that this comment will become superfluous if you're going to add
an error message.

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

> +	ret = atmel_ecc_status(&client->dev, cmd->data);


atmel_ecc_status already prints errors when needed.

> +	if (ret < 0)

> +		dev_err(&client->dev, "ECC status parse failed\n");

> +

> +	return ret;

>   err:

>   	mutex_unlock(&i2c_priv->lock);

>   	return ret;

> @@ -624,8 +636,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");


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?

Thanks,
ta

>   		goto free_cmd;

> +	}

>   

>   	/*

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

>
Linus Walleij June 28, 2018, 8:54 a.m. | #2
On Tue, Jun 12, 2018 at 2:36 PM Tudor Ambarus
<tudor.ambarus@microchip.com> 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(&client->dev, cmd->data);

> > +     ret = atmel_ecc_status(&client->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(&client->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

Patch

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index 145ab3a39a56..214b0572bf8b 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -310,29 +310,41 @@  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);
+	ret = atmel_ecc_status(&client->dev, cmd->data);
+	if (ret < 0)
+		dev_err(&client->dev, "ECC status parse failed\n");
+
+	return ret;
 err:
 	mutex_unlock(&i2c_priv->lock);
 	return ret;
@@ -624,8 +636,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