i2c: tegra: remove warning dump if timeout happen in transfer

Submitted by Laxman Dewangan on Feb. 14, 2013, 12:43 p.m.

Details

Message ID 1360845813-11334-1-git-send-email-ldewangan@nvidia.com
State Accepted
Headers show

Commit Message

Laxman Dewangan Feb. 14, 2013, 12:43 p.m.
If timeout error occurs in the i2c transfer then it was dumping warning
of call stack.

Remove the warning dump as there is may be possibility that some slave
devices are busy and not responding the i2c communication.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
The patch is generated based on discussion happen between Stephena and
Wolfram on the patch:
 i2c: add bcm2835 driver

resending patch as Wolfram's email id has been changed.

 drivers/i2c/busses/i2c-tegra.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Shubhrajyoti Datta Feb. 15, 2013, 1:01 p.m.
On Thu, Feb 14, 2013 at 6:13 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> If timeout error occurs in the i2c transfer then it was dumping warning
> of call stack.
>
> Remove the warning dump as there is may be possibility that some slave
> devices are busy and not responding the i2c communication.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
> The patch is generated based on discussion happen between Stephena and
> Wolfram on the patch:
>  i2c: add bcm2835 driver
>
> resending patch as Wolfram's email id has been changed.
>
>  drivers/i2c/busses/i2c-tegra.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index ae2e027..36704e3 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -587,7 +587,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>         ret = wait_for_completion_timeout(&i2c_dev->msg_complete, TEGRA_I2C_TIMEOUT);
>         tegra_i2c_mask_irq(i2c_dev, int_mask);
>
> -       if (WARN_ON(ret == 0)) {
> +       if (ret == 0) {

I think WARN_ON has a unlikely.

If you could do a profiling and have the unlikely.

BTW thats not an objection to the patch though.

>                 dev_err(i2c_dev->dev, "i2c transfer timed out\n");
>
>                 tegra_i2c_init(i2c_dev);
> --
> 1.7.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Feb. 15, 2013, 1:04 p.m.
On Fri, Feb 15, 2013 at 06:31:07PM +0530, Shubhrajyoti Datta wrote:
> On Thu, Feb 14, 2013 at 6:13 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> > If timeout error occurs in the i2c transfer then it was dumping warning
> > of call stack.
> >
> > Remove the warning dump as there is may be possibility that some slave
> > devices are busy and not responding the i2c communication.
> >
> > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> > ---
> > The patch is generated based on discussion happen between Stephena and
> > Wolfram on the patch:
> >  i2c: add bcm2835 driver
> >
> > resending patch as Wolfram's email id has been changed.
> >
> >  drivers/i2c/busses/i2c-tegra.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> > index ae2e027..36704e3 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -587,7 +587,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> >         ret = wait_for_completion_timeout(&i2c_dev->msg_complete, TEGRA_I2C_TIMEOUT);
> >         tegra_i2c_mask_irq(i2c_dev, int_mask);
> >
> > -       if (WARN_ON(ret == 0)) {
> > +       if (ret == 0) {
> 
> I think WARN_ON has a unlikely.
> 
> If you could do a profiling and have the unlikely.
> 
> BTW thats not an objection to the patch though.

The thing is: Timeouts can be expected on an I2C bus. Devices can be
busy, that's fine. So, no need for a WARN. I'm even thinking of asking
to remove the dev_err here, or at least change it into dev_warn, but
haven't made up my mind yet.

Regards,

   Wolfram

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Feb. 15, 2013, 7:18 p.m.
On Thu, Feb 14, 2013 at 06:13:33PM +0530, Laxman Dewangan wrote:
> If timeout error occurs in the i2c transfer then it was dumping warning
> of call stack.
> 
> Remove the warning dump as there is may be possibility that some slave
> devices are busy and not responding the i2c communication.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

I'd like to have the patch extended. Currently, i2c-tegra is the only
I2C user of BUG and BUG_ON. Could you maybe extend the patch to handle
those situations more gracefully while we are at it? From a glimpse,
these situations don't need a complete halt of the kernel?

Thanks,

   Wolfram

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 15, 2013, 7:40 p.m.
On 02/15/2013 12:18 PM, Wolfram Sang wrote:
> On Thu, Feb 14, 2013 at 06:13:33PM +0530, Laxman Dewangan wrote:
>> If timeout error occurs in the i2c transfer then it was dumping warning
>> of call stack.
>>
>> Remove the warning dump as there is may be possibility that some slave
>> devices are busy and not responding the i2c communication.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> 
> I'd like to have the patch extended. Currently, i2c-tegra is the only
> I2C user of BUG and BUG_ON. Could you maybe extend the patch to handle
> those situations more gracefully while we are at it? From a glimpse,
> these situations don't need a complete halt of the kernel?

While that's probably useful, surely that's a separate patch?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Feb. 15, 2013, 8:10 p.m.
On Fri, Feb 15, 2013 at 12:40:47PM -0700, Stephen Warren wrote:
> On 02/15/2013 12:18 PM, Wolfram Sang wrote:
> > On Thu, Feb 14, 2013 at 06:13:33PM +0530, Laxman Dewangan wrote:
> >> If timeout error occurs in the i2c transfer then it was dumping warning
> >> of call stack.
> >>
> >> Remove the warning dump as there is may be possibility that some slave
> >> devices are busy and not responding the i2c communication.
> >>
> >> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> > 
> > I'd like to have the patch extended. Currently, i2c-tegra is the only
> > I2C user of BUG and BUG_ON. Could you maybe extend the patch to handle
> > those situations more gracefully while we are at it? From a glimpse,
> > these situations don't need a complete halt of the kernel?
> 
> While that's probably useful, surely that's a separate patch?

Yes, can be argued. Patch applied to for-next, thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index ae2e027..36704e3 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -587,7 +587,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	ret = wait_for_completion_timeout(&i2c_dev->msg_complete, TEGRA_I2C_TIMEOUT);
 	tegra_i2c_mask_irq(i2c_dev, int_mask);
 
-	if (WARN_ON(ret == 0)) {
+	if (ret == 0) {
 		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
 
 		tegra_i2c_init(i2c_dev);