[PATCH] watchdog: imx: error out on negative timeouts

[PATCH] watchdog: imx: error out on negative timeouts

[Uwe Kleine-König]

I'm not sure where a negative timeout could come from but making the
code more robust for no additional runtime cost is good nevertheless.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/imxwd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
index 31c3d0d85353..66e9f6848f74 100644
--- a/drivers/watchdog/imxwd.c
+++ b/drivers/watchdog/imxwd.c
@@ -87,7 +87,7 @@ static int imx21_watchdog_set_timeout(struct imx_wd *priv, int timeout)
 
 	dev_dbg(priv->dev, "%s: %d\n", __func__, timeout);
 
-	if (!timeout || timeout > 128)
+	if (timeout <= 0 || timeout > 128)
 		return -EINVAL;
 
 	if (timeout > 0)
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] watchdog: imx: error out on negative timeouts

[Sascha Hauer]

On Tue, Jan 27, 2015 at 02:41:01PM +0100, Uwe Kleine-König wrote:
> I'm not sure where a negative timeout could come from but making the
> code more robust for no additional runtime cost is good nevertheless.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/watchdog/imxwd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] watchdog: imx: error out on negative timeouts

[Uwe Kleine-König]

Hello,

On Tue, Jan 27, 2015 at 02:41:01PM +0100, Uwe Kleine-König wrote:
> I'm not sure where a negative timeout could come from but making the
> code more robust for no additional runtime cost is good nevertheless.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/watchdog/imxwd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
> index 31c3d0d85353..66e9f6848f74 100644
> --- a/drivers/watchdog/imxwd.c
> +++ b/drivers/watchdog/imxwd.c
> @@ -87,7 +87,7 @@ static int imx21_watchdog_set_timeout(struct imx_wd *priv, int timeout)
>  
>  	dev_dbg(priv->dev, "%s: %d\n", __func__, timeout);
>  
> -	if (!timeout || timeout > 128)
> +	if (timeout <= 0 || timeout > 128)
>  		return -EINVAL;
This patch is broken because reset_cpu (in the same source file) calls
set_timeout with timeout=-1 to reset the cpu immediatly. The wd command
only parses non-negative values, so from there nothing strange should be
expected.

Returning -EINVAL on timeout=0 (which means "disable watchdog") is OK
because the imx21 watchdog cannot be stopped.

So in short: please drop this patch from next.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] watchdog: imx: error out on negative timeouts

[Sascha Hauer]

On Mon, Feb 02, 2015 at 11:49:57AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jan 27, 2015 at 02:41:01PM +0100, Uwe Kleine-König wrote:
> > I'm not sure where a negative timeout could come from but making the
> > code more robust for no additional runtime cost is good nevertheless.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/watchdog/imxwd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
> > index 31c3d0d85353..66e9f6848f74 100644
> > --- a/drivers/watchdog/imxwd.c
> > +++ b/drivers/watchdog/imxwd.c
> > @@ -87,7 +87,7 @@ static int imx21_watchdog_set_timeout(struct imx_wd *priv, int timeout)
> >  
> >  	dev_dbg(priv->dev, "%s: %d\n", __func__, timeout);
> >  
> > -	if (!timeout || timeout > 128)
> > +	if (timeout <= 0 || timeout > 128)
> >  		return -EINVAL;
> This patch is broken because reset_cpu (in the same source file) calls
> set_timeout with timeout=-1 to reset the cpu immediatly. The wd command
> only parses non-negative values, so from there nothing strange should be
> expected.
> 
> Returning -EINVAL on timeout=0 (which means "disable watchdog") is OK
> because the imx21 watchdog cannot be stopped.
> 
> So in short: please drop this patch from next.

Did that.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] watchdog: imx: error out on negative timeouts

[Masahiro Yamada]

On Mon, 2 Feb 2015 12:04:32 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Mon, Feb 02, 2015 at 11:49:57AM +0100, Uwe Kleine-Konig wrote:
> > Hello,
> > 
> > On Tue, Jan 27, 2015 at 02:41:01PM +0100, Uwe Kleine-Konig wrote:
> > > I'm not sure where a negative timeout could come from but making the
> > > code more robust for no additional runtime cost is good nevertheless.
> > > 
> > > Signed-off-by: Uwe Kleine-Konig <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/watchdog/imxwd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
> > > index 31c3d0d85353..66e9f6848f74 100644
> > > --- a/drivers/watchdog/imxwd.c
> > > +++ b/drivers/watchdog/imxwd.c
> > > @@ -87,7 +87,7 @@ static int imx21_watchdog_set_timeout(struct imx_wd *priv, int timeout)
> > >  
> > >  	dev_dbg(priv->dev, "%s: %d\n", __func__, timeout);
> > >  
> > > -	if (!timeout || timeout > 128)
> > > +	if (timeout <= 0 || timeout > 128)
> > >  		return -EINVAL;
> > This patch is broken because reset_cpu (in the same source file) calls
> > set_timeout with timeout=-1 to reset the cpu immediatly. The wd command
> > only parses non-negative values, so from there nothing strange should be
> > expected.
> > 
> > Returning -EINVAL on timeout=0 (which means "disable watchdog") is OK
> > because the imx21 watchdog cannot be stopped.
> > 
> > So in short: please drop this patch from next.
> 
> Did that.
> 


Forgive my newbie questions.

I have been studied barebox for one month and a half,
I think I could roughly understand the next branch policy in this community,
but could you help me to make it a little bit clearer?


I guess the barebox/next is similar to the linux-next repository.

Similar points are:
 [1] barebox/next is not fast-forwarded
     (Developers should work with "git rebase --onto" as we do in linux-next
 [2] barebox/next represents the source tree that is *probably* merged into the master branch
     after the next release.
     (Perhaps, barebox might not have what we call Merge Window,
      but I notice topic branches are merged right after every-month release.)


On the other hand, I notice some differences

[3] All the topic branches are locally maintained by Sascha, so they are never pushed
    to the public repository.
[4] Some commits in topic branches might be dropped rather than being git-reverted
    if they turned out to be bad.
    (i.e. commit ID becomes a fixed value when it is merge into the master branch.)


If [4] is true, we should not describe the commit ID in the following commits
until it is merged into the master branch.   (Or we should be very careful when we do so.)

We often write something like
"Since commit xxxxxxxxxxxx, the foo function has not been working. Blah Blah ..."
in bug-fix patches.
But xxxxxxxxxxxx may change if the preceding commit is dropped or modified.


Is this statement correct?


Best Regards
Masahiro Yamada


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] watchdog: imx: error out on negative timeouts

[Sascha Hauer]

On Mon, Feb 02, 2015 at 08:50:38PM +0900, Masahiro Yamada wrote:
> 
> On Mon, 2 Feb 2015 12:04:32 +0100
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Mon, Feb 02, 2015 at 11:49:57AM +0100, Uwe Kleine-Konig wrote:
> > > Hello,
> > > 
> > > On Tue, Jan 27, 2015 at 02:41:01PM +0100, Uwe Kleine-Konig wrote:
> > > > I'm not sure where a negative timeout could come from but making the
> > > > code more robust for no additional runtime cost is good nevertheless.
> > > > 
> > > > Signed-off-by: Uwe Kleine-Konig <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  drivers/watchdog/imxwd.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
> > > > index 31c3d0d85353..66e9f6848f74 100644
> > > > --- a/drivers/watchdog/imxwd.c
> > > > +++ b/drivers/watchdog/imxwd.c
> > > > @@ -87,7 +87,7 @@ static int imx21_watchdog_set_timeout(struct imx_wd *priv, int timeout)
> > > >  
> > > >  	dev_dbg(priv->dev, "%s: %d\n", __func__, timeout);
> > > >  
> > > > -	if (!timeout || timeout > 128)
> > > > +	if (timeout <= 0 || timeout > 128)
> > > >  		return -EINVAL;
> > > This patch is broken because reset_cpu (in the same source file) calls
> > > set_timeout with timeout=-1 to reset the cpu immediatly. The wd command
> > > only parses non-negative values, so from there nothing strange should be
> > > expected.
> > > 
> > > Returning -EINVAL on timeout=0 (which means "disable watchdog") is OK
> > > because the imx21 watchdog cannot be stopped.
> > > 
> > > So in short: please drop this patch from next.
> > 
> > Did that.
> > 
> 
> 
> Forgive my newbie questions.
> 
> I have been studied barebox for one month and a half,
> I think I could roughly understand the next branch policy in this community,
> but could you help me to make it a little bit clearer?
> 
> 
> I guess the barebox/next is similar to the linux-next repository.
> 
> Similar points are:
>  [1] barebox/next is not fast-forwarded
>      (Developers should work with "git rebase --onto" as we do in linux-next

If possible please base your patches on the master branch. I'll pick a
suitable for-next/* branch merge the for-next/* branches together and
handle the merge conflicts. If you have direct dependencies on some
patches in -next you can base your patches on -next.

>  [2] barebox/next represents the source tree that is *probably* merged into the master branch
>      after the next release.
>      (Perhaps, barebox might not have what we call Merge Window,
>       but I notice topic branches are merged right after every-month release.)

yes.

> 
> 
> On the other hand, I notice some differences
> 
> [3] All the topic branches are locally maintained by Sascha, so they are never pushed
>     to the public repository.

Right.

> [4] Some commits in topic branches might be dropped rather than being git-reverted
>     if they turned out to be bad.

Right. Also you can always send a patch committed with "--fixup=" to the
list, then I can just squash the changes into the original commit.

I routinely build every commit in -next with every defconfig in the
tree. It regularly happens that patches do not build in every defconfig
because of missing ifdefs or dependencies. Most of the times I fix that
up quietly.

>     (i.e. commit ID becomes a fixed value when it is merge into the master branch.)

Yes.

> 
> 
> If [4] is true, we should not describe the commit ID in the following commits
> until it is merged into the master branch.   (Or we should be very careful when we do so.)

Yes.

> 
> We often write something like
> "Since commit xxxxxxxxxxxx, the foo function has not been working. Blah Blah ..."
> in bug-fix patches.
> But xxxxxxxxxxxx may change if the preceding commit is dropped or modified.

As long xxxxxxxxxxxx hasn't hit master you can just ask me to fix the
offending commit directly, preferably using --fixup= to git-commit.
Once it hits master the commit IDs are stable, so you can refer to them
in bug fix patches.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] watchdog: imx: error out on negative timeouts

[Masahiro Yamada]

On Mon, 2 Feb 2015 13:16:32 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Mon, Feb 02, 2015 at 08:50:38PM +0900, Masahiro Yamada wrote:
> > 
> > On Mon, 2 Feb 2015 12:04:32 +0100
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > 
> > > On Mon, Feb 02, 2015 at 11:49:57AM +0100, Uwe Kleine-Konig wrote:
> > > > Hello,
> > > > 
> > > > On Tue, Jan 27, 2015 at 02:41:01PM +0100, Uwe Kleine-Konig wrote:
> > > > > I'm not sure where a negative timeout could come from but making the
> > > > > code more robust for no additional runtime cost is good nevertheless.
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-Konig <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > >  drivers/watchdog/imxwd.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
> > > > > index 31c3d0d85353..66e9f6848f74 100644
> > > > > --- a/drivers/watchdog/imxwd.c
> > > > > +++ b/drivers/watchdog/imxwd.c
> > > > > @@ -87,7 +87,7 @@ static int imx21_watchdog_set_timeout(struct imx_wd *priv, int timeout)
> > > > >  
> > > > >  	dev_dbg(priv->dev, "%s: %d\n", __func__, timeout);
> > > > >  
> > > > > -	if (!timeout || timeout > 128)
> > > > > +	if (timeout <= 0 || timeout > 128)
> > > > >  		return -EINVAL;
> > > > This patch is broken because reset_cpu (in the same source file) calls
> > > > set_timeout with timeout=-1 to reset the cpu immediatly. The wd command
> > > > only parses non-negative values, so from there nothing strange should be
> > > > expected.
> > > > 
> > > > Returning -EINVAL on timeout=0 (which means "disable watchdog") is OK
> > > > because the imx21 watchdog cannot be stopped.
> > > > 
> > > > So in short: please drop this patch from next.
> > > 
> > > Did that.
> > > 
> > 
> > 
> > Forgive my newbie questions.
> > 
> > I have been studied barebox for one month and a half,
> > I think I could roughly understand the next branch policy in this community,
> > but could you help me to make it a little bit clearer?
> > 
> > 
> > I guess the barebox/next is similar to the linux-next repository.
> > 
> > Similar points are:
> >  [1] barebox/next is not fast-forwarded
> >      (Developers should work with "git rebase --onto" as we do in linux-next
> 
> If possible please base your patches on the master branch. I'll pick a
> suitable for-next/* branch merge the for-next/* branches together and
> handle the merge conflicts. If you have direct dependencies on some
> patches in -next you can base your patches on -next.
> 
> >  [2] barebox/next represents the source tree that is *probably* merged into the master branch
> >      after the next release.
> >      (Perhaps, barebox might not have what we call Merge Window,
> >       but I notice topic branches are merged right after every-month release.)
> 
> yes.
> 
> > 
> > 
> > On the other hand, I notice some differences
> > 
> > [3] All the topic branches are locally maintained by Sascha, so they are never pushed
> >     to the public repository.
> 
> Right.
> 
> > [4] Some commits in topic branches might be dropped rather than being git-reverted
> >     if they turned out to be bad.
> 
> Right. Also you can always send a patch committed with "--fixup=" to the
> list, then I can just squash the changes into the original commit.
> 
> I routinely build every commit in -next with every defconfig in the
> tree. It regularly happens that patches do not build in every defconfig
> because of missing ifdefs or dependencies. Most of the times I fix that
> up quietly.
> 
> >     (i.e. commit ID becomes a fixed value when it is merge into the master branch.)
> 
> Yes.
> 
> > 
> > 
> > If [4] is true, we should not describe the commit ID in the following commits
> > until it is merged into the master branch.   (Or we should be very careful when we do so.)
> 
> Yes.
> 
> > 
> > We often write something like
> > "Since commit xxxxxxxxxxxx, the foo function has not been working. Blah Blah ..."
> > in bug-fix patches.
> > But xxxxxxxxxxxx may change if the preceding commit is dropped or modified.
> 
> As long xxxxxxxxxxxx hasn't hit master you can just ask me to fix the
> offending commit directly, preferably using --fixup= to git-commit.
> Once it hits master the commit IDs are stable, so you can refer to them
> in bug fix patches.


Sascha,
Thanks for clarification.
I will keep them in my mind.


Please let me ask one more question.

 Is there any pach tracking tool used for barebox, like PatchWork?

I guess the answer is no.

The barebox ML does not have much volume and your review is very quick,
so I do not think we need such a tool.


Best Regards
Masahiro Yamada


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] watchdog: imx: error out on negative timeouts

[Sascha Hauer]

On Tue, Feb 03, 2015 at 03:14:41PM +0900, Masahiro Yamada wrote:
> Please let me ask one more question.
> 
>  Is there any pach tracking tool used for barebox, like PatchWork?

No, there is no such tool at the moment.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox