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

2018-06-28 Thread Linus Walleij
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 3/9] crypto: atmel-ecc: More helpful error messages

2018-06-12 Thread Tudor Ambarus

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 
---
  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(_priv->lock);
  
  	ret = atmel_ecc_wakeup(client);

-   if (ret)
+   if (ret) {
+   dev_err(>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(>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);
+   ret = atmel_ecc_status(>dev, cmd->data);


atmel_ecc_status already prints errors when needed.


+   if (ret < 0)
+   dev_err(>dev, "ECC status parse failed\n");
+
+   return ret;
  err:
mutex_unlock(_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(>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



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

2018-06-05 Thread Linus Walleij
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 
---
 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(_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);
+   ret = atmel_ecc_status(>dev, cmd->data);
+   if (ret < 0)
+   dev_err(>dev, "ECC status parse failed\n");
+
+   return ret;
 err:
mutex_unlock(_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(>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