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

[Sascha Hauer <s.hauer@pengutronix.de>]

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