2014-06-23 10:35:17

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On Thu, May 22, 2014 at 02:12:07PM -0700, Guenter Roeck wrote:
> On Thu, May 22, 2014 at 10:34:44PM +0200, Maxime Ripard wrote:
> > On Mon, May 19, 2014 at 05:04:22PM +0200, Maxime Ripard wrote:
> > > On Thu, May 15, 2014 at 11:11:23AM +0200, Maxime Ripard wrote:
> > > > On Wed, May 07, 2014 at 02:33:18PM -0700, Guenter Roeck wrote:
> > > > > On Tue, May 06, 2014 at 09:44:19PM -0500, Maxime Ripard wrote:
> > > > > > Most of the watchdog code is duplicated between the machine restart code and
> > > > > > the watchdog driver. Add the restart hook to the watchdog driver, to be able to
> > > > > > remove it from the machine code eventually.
> > > > > >
> > > > > > Signed-off-by: Maxime Ripard <[email protected]>
> > > > > > Acked-by: Arnd Bergmann <[email protected]>
> > > > >
> > > > > Reviewed-by: Guenter Roeck <[email protected]>
> > > >
> > > > Wim, do you have any comment on this one?
> > >
> > > Ping?
> > >
> > > It would be really great to see this in 3.16, and we get quite close
> > > to the end of ARM's merge window.
> >
> > Ping?
> >
> > Guenter, since you seem to be the only responsive, may I suggest that
> > you start merging patches and do a pull request to either Wim or Linus
> > directly during the merge window?
> >
> I had prepared a pull request for Wim last weekend or so, but then there
> were more patches piling in and I got distracted, so I didn't have time
> to actually send it. I'll try again this weekend ... the kids should be
> busy learning for their finals, and I'll have Friday and Monday off
> from work, so I should be able to find the time.
>
> As for sending patches to Linus directly, well, Wim is the watchdog maintainer.
> I manage to upset enough people, and would not want to add Wim to the list.
>
> The patches _are_ in my watchdog-next branch and get some coverage from
> both my auto-builders and from Fenguang's build robots, so while they are
> not in linux-next, they are not completely in the dark either.

So, this patch finally didn't make it into 3.16. Great. Now, we can't
even reboot the boards.

Given how it's just impossible to get something merged reliably
through the watchdog tree, I guess I should just start merging the
patches through mine?

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (2.30 kB)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-06-23 14:31:13

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On 06/23/2014 03:31 AM, Maxime Ripard wrote:
> On Thu, May 22, 2014 at 02:12:07PM -0700, Guenter Roeck wrote:
>> On Thu, May 22, 2014 at 10:34:44PM +0200, Maxime Ripard wrote:
>>> On Mon, May 19, 2014 at 05:04:22PM +0200, Maxime Ripard wrote:
>>>> On Thu, May 15, 2014 at 11:11:23AM +0200, Maxime Ripard wrote:
>>>>> On Wed, May 07, 2014 at 02:33:18PM -0700, Guenter Roeck wrote:
>>>>>> On Tue, May 06, 2014 at 09:44:19PM -0500, Maxime Ripard wrote:
>>>>>>> Most of the watchdog code is duplicated between the machine restart code and
>>>>>>> the watchdog driver. Add the restart hook to the watchdog driver, to be able to
>>>>>>> remove it from the machine code eventually.
>>>>>>>
>>>>>>> Signed-off-by: Maxime Ripard <[email protected]>
>>>>>>> Acked-by: Arnd Bergmann <[email protected]>
>>>>>>
>>>>>> Reviewed-by: Guenter Roeck <[email protected]>
>>>>>
>>>>> Wim, do you have any comment on this one?
>>>>
>>>> Ping?
>>>>
>>>> It would be really great to see this in 3.16, and we get quite close
>>>> to the end of ARM's merge window.
>>>
>>> Ping?
>>>
>>> Guenter, since you seem to be the only responsive, may I suggest that
>>> you start merging patches and do a pull request to either Wim or Linus
>>> directly during the merge window?
>>>
>> I had prepared a pull request for Wim last weekend or so, but then there
>> were more patches piling in and I got distracted, so I didn't have time
>> to actually send it. I'll try again this weekend ... the kids should be
>> busy learning for their finals, and I'll have Friday and Monday off
>> from work, so I should be able to find the time.
>>
>> As for sending patches to Linus directly, well, Wim is the watchdog maintainer.
>> I manage to upset enough people, and would not want to add Wim to the list.
>>
>> The patches _are_ in my watchdog-next branch and get some coverage from
>> both my auto-builders and from Fenguang's build robots, so while they are
>> not in linux-next, they are not completely in the dark either.
>
> So, this patch finally didn't make it into 3.16. Great. Now, we can't
> even reboot the boards.
>
> Given how it's just impossible to get something merged reliably
> through the watchdog tree, I guess I should just start merging the
> patches through mine?
>

You can not really blame Wim here.

In this case, I suspect the major reason for not accepting the patch
is that I tried to provide a clean method / API for "reset through watchdog
subsystem", which went nowhere, in my understanding because someone objected
that it would be the wrong thing to do [1] and it didn't get approval /
acceptance from the arm maintainers. If it is wrong to reset the board
from the watchdog subsystem in a clean way, it is for sure even more wrong
to do it as you proposed in your patch.

My conclusion therefore is that all board reset code should move back out
of the watchdog subsystem, and that we should not accept such code in the
future. This is not my personal preference, but I do believe that we should
do it in a clean way or not at all.

Guenter

---
[1] https://lkml.org/lkml/2014/5/15/838

2014-06-23 14:43:45

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On Monday 23 June 2014 07:30:56 Guenter Roeck wrote:
> On 06/23/2014 03:31 AM, Maxime Ripard wrote:
> > On Thu, May 22, 2014 at 02:12:07PM -0700, Guenter Roeck wrote:
> >> On Thu, May 22, 2014 at 10:34:44PM +0200, Maxime Ripard wrote:
> >>> On Mon, May 19, 2014 at 05:04:22PM +0200, Maxime Ripard wrote:
> >>
> >> The patches _are_ in my watchdog-next branch and get some coverage from
> >> both my auto-builders and from Fenguang's build robots, so while they are
> >> not in linux-next, they are not completely in the dark either.
> >
> > So, this patch finally didn't make it into 3.16. Great. Now, we can't
> > even reboot the boards.
> >
> > Given how it's just impossible to get something merged reliably
> > through the watchdog tree, I guess I should just start merging the
> > patches through mine?
> >
>
> You can not really blame Wim here.
>
> In this case, I suspect the major reason for not accepting the patch
> is that I tried to provide a clean method / API for "reset through watchdog
> subsystem", which went nowhere, in my understanding because someone objected
> that it would be the wrong thing to do [1] and it didn't get approval /
> acceptance from the arm maintainers. If it is wrong to reset the board
> from the watchdog subsystem in a clean way, it is for sure even more wrong
> to do it as you proposed in your patch.
>
> My conclusion therefore is that all board reset code should move back out
> of the watchdog subsystem, and that we should not accept such code in the
> future. This is not my personal preference, but I do believe that we should
> do it in a clean way or not at all.

Moved to where?

I certainly don't want it in the platform directories, and for arm64 we
intentionally don't have a place to put this stuff.

drivers/power/reset would be an option, but then we have to solve the
problem of loading two drivers for one device first.

Arnd

2014-06-23 15:16:31

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On 06/23/2014 07:42 AM, Arnd Bergmann wrote:
> On Monday 23 June 2014 07:30:56 Guenter Roeck wrote:
>> On 06/23/2014 03:31 AM, Maxime Ripard wrote:
>>> On Thu, May 22, 2014 at 02:12:07PM -0700, Guenter Roeck wrote:
>>>> On Thu, May 22, 2014 at 10:34:44PM +0200, Maxime Ripard wrote:
>>>>> On Mon, May 19, 2014 at 05:04:22PM +0200, Maxime Ripard wrote:
>>>>
>>>> The patches _are_ in my watchdog-next branch and get some coverage from
>>>> both my auto-builders and from Fenguang's build robots, so while they are
>>>> not in linux-next, they are not completely in the dark either.
>>>
>>> So, this patch finally didn't make it into 3.16. Great. Now, we can't
>>> even reboot the boards.
>>>
>>> Given how it's just impossible to get something merged reliably
>>> through the watchdog tree, I guess I should just start merging the
>>> patches through mine?
>>>
>>
>> You can not really blame Wim here.
>>
>> In this case, I suspect the major reason for not accepting the patch
>> is that I tried to provide a clean method / API for "reset through watchdog
>> subsystem", which went nowhere, in my understanding because someone objected
>> that it would be the wrong thing to do [1] and it didn't get approval /
>> acceptance from the arm maintainers. If it is wrong to reset the board
>> from the watchdog subsystem in a clean way, it is for sure even more wrong
>> to do it as you proposed in your patch.
>>
>> My conclusion therefore is that all board reset code should move back out
>> of the watchdog subsystem, and that we should not accept such code in the
>> future. This is not my personal preference, but I do believe that we should
>> do it in a clean way or not at all.
>
> Moved to where?
>
> I certainly don't want it in the platform directories, and for arm64 we
> intentionally don't have a place to put this stuff.
>

I have no idea, but setting the arm reset function pointer from a watchdog
driver doesn't seem like a good idea either. The arm code _does_ provide
and expect platform code to set the reset function, so having it in the arm
code would at least make more sense than expecting some unrelated driver
code to set it - especially since it is inherently racy [1].

> drivers/power/reset would be an option, but then we have to solve the
> problem of loading two drivers for one device first.
>
It sounds crazy and may be huge overkill, but one solution for this is
would be to use mfd to bridge the gap.

Guenter

---
[1] https://lkml.org/lkml/2014/5/15/838

2014-06-23 15:36:13

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On Monday 23 June 2014 08:16:18 Guenter Roeck wrote:
> > Moved to where?
> >
> > I certainly don't want it in the platform directories, and for arm64 we
> > intentionally don't have a place to put this stuff.
> >
>
> I have no idea, but setting the arm reset function pointer from a watchdog
> driver doesn't seem like a good idea either. The arm code _does_ provide
> and expect platform code to set the reset function, so having it in the arm
> code would at least make more sense than expecting some unrelated driver
> code to set it - especially since it is inherently racy [1].

I don't think the race is inherent. We could solve the multiple registration
problem and the unload problem using a notifier chain. If there are actually
cases where we expect to see multiple reboot functions to be present in the
system, we could go one step further and have a priority associated with
it, so we try the best function first and fall back to soft_restart()
if everything else fails.

Arnd

2014-06-23 15:40:17

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On 06/23/2014 08:35 AM, Arnd Bergmann wrote:
> On Monday 23 June 2014 08:16:18 Guenter Roeck wrote:
>>> Moved to where?
>>>
>>> I certainly don't want it in the platform directories, and for arm64 we
>>> intentionally don't have a place to put this stuff.
>>>
>>
>> I have no idea, but setting the arm reset function pointer from a watchdog
>> driver doesn't seem like a good idea either. The arm code _does_ provide
>> and expect platform code to set the reset function, so having it in the arm
>> code would at least make more sense than expecting some unrelated driver
>> code to set it - especially since it is inherently racy [1].
>
> I don't think the race is inherent. We could solve the multiple registration
> problem and the unload problem using a notifier chain. If there are actually
> cases where we expect to see multiple reboot functions to be present in the
> system, we could go one step further and have a priority associated with
> it, so we try the best function first and fall back to soft_restart()
> if everything else fails.
>

You mean something like a restart notifier, which would be called from
architecture code (and replace the arm reset function pointer) ?
Yes, that sounds like a possible solution.

Guenter

2014-06-23 15:50:07

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On Mon, Jun 23, 2014 at 07:30:56AM -0700, Guenter Roeck wrote:
> >>The patches _are_ in my watchdog-next branch and get some coverage from
> >>both my auto-builders and from Fenguang's build robots, so while they are
> >>not in linux-next, they are not completely in the dark either.
> >
> >So, this patch finally didn't make it into 3.16. Great. Now, we can't
> >even reboot the boards.
> >
> >Given how it's just impossible to get something merged reliably
> >through the watchdog tree, I guess I should just start merging the
> >patches through mine?
> >
>
> You can not really blame Wim here.
>
> In this case, I suspect the major reason for not accepting the patch
> is that I tried to provide a clean method / API for "reset through watchdog
> subsystem", which went nowhere, in my understanding because someone objected
> that it would be the wrong thing to do [1] and it didn't get approval /
> acceptance from the arm maintainers. If it is wrong to reset the board
> from the watchdog subsystem in a clean way, it is for sure even more wrong
> to do it as you proposed in your patch.
>
> My conclusion therefore is that all board reset code should move back out
> of the watchdog subsystem, and that we should not accept such code in the
> future. This is not my personal preference, but I do believe that we should
> do it in a clean way or not at all.

Well, considering that this patch isn't depending on your reboot API
set, and that Wim never either commented on this patch, your reboot
API patchset or your pull request to say that he was not willing to
merge this, there's still a huge failure to communicate.

I'm fine with any technical reason, let's debate on that. But the
point is there has been no debate at all, only silence from his side.

I have been told some patches would be merged and I merged through my
tree some patches that were depending on this one based on that
assumption.

And now, we have a regression.

Anyway... I guess I should just revert some commits now.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (2.07 kB)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-06-23 21:13:38

by Wim Van Sebroeck

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

Hi Maxime,

> > > Guenter, since you seem to be the only responsive, may I suggest that
> > > you start merging patches and do a pull request to either Wim or Linus
> > > directly during the merge window?
> > >
> > I had prepared a pull request for Wim last weekend or so, but then there
> > were more patches piling in and I got distracted, so I didn't have time
> > to actually send it. I'll try again this weekend ... the kids should be
> > busy learning for their finals, and I'll have Friday and Monday off
> > from work, so I should be able to find the time.
> >
> > As for sending patches to Linus directly, well, Wim is the watchdog maintainer.
> > I manage to upset enough people, and would not want to add Wim to the list.
> >
> > The patches _are_ in my watchdog-next branch and get some coverage from
> > both my auto-builders and from Fenguang's build robots, so while they are
> > not in linux-next, they are not completely in the dark either.
>
> So, this patch finally didn't make it into 3.16. Great. Now, we can't
> even reboot the boards.

1) For me the discussion was not ended and needs further thinking. (And I just read some good ideas about it).
2) You never mentioned you needed this in for 3.16 and that things would break because of it.

So based on these 2 points why would I have to have put this in allready?

> Given how it's just impossible to get something merged reliably
> through the watchdog tree, I guess I should just start merging the
> patches through mine?

I agree that I have the problem of having only 24 hours in a day and that I lack time to communicate and that I am not good at communicating either, but I checked all sunxi related e-mails and you never mentioned the constraint to have it in for 3.16...
But I do understand your frustration.

Kind regards,
Wim.

2014-06-23 21:37:57

by Wim Van Sebroeck

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

Hi All,

> On Mon, Jun 23, 2014 at 07:30:56AM -0700, Guenter Roeck wrote:
> > >>The patches _are_ in my watchdog-next branch and get some coverage from
> > >>both my auto-builders and from Fenguang's build robots, so while they are
> > >>not in linux-next, they are not completely in the dark either.
> > >
> > >So, this patch finally didn't make it into 3.16. Great. Now, we can't
> > >even reboot the boards.
> > >
> > >Given how it's just impossible to get something merged reliably
> > >through the watchdog tree, I guess I should just start merging the
> > >patches through mine?
> > >
> >
> > You can not really blame Wim here.
> >
> > In this case, I suspect the major reason for not accepting the patch
> > is that I tried to provide a clean method / API for "reset through watchdog
> > subsystem", which went nowhere, in my understanding because someone objected
> > that it would be the wrong thing to do [1] and it didn't get approval /
> > acceptance from the arm maintainers. If it is wrong to reset the board
> > from the watchdog subsystem in a clean way, it is for sure even more wrong
> > to do it as you proposed in your patch.
> >
> > My conclusion therefore is that all board reset code should move back out
> > of the watchdog subsystem, and that we should not accept such code in the
> > future. This is not my personal preference, but I do believe that we should
> > do it in a clean way or not at all.
>
> Well, considering that this patch isn't depending on your reboot API
> set, and that Wim never either commented on this patch, your reboot
> API patchset or your pull request to say that he was not willing to
> merge this, there's still a huge failure to communicate.
>
> I'm fine with any technical reason, let's debate on that. But the
> point is there has been no debate at all, only silence from his side.
>
> I have been told some patches would be merged and I merged through my
> tree some patches that were depending on this one based on that
> assumption.
>
> And now, we have a regression.
>
> Anyway... I guess I should just revert some commits now.
>

To continue the discussion: I would like to add an excerpt from drivers/watchdog/alim7101_wdt.c
/*
* Notifier for system down
*/

static int wdt_notify_sys(struct notifier_block *this,
unsigned long code, void *unused)
{
if (code == SYS_DOWN || code == SYS_HALT)
wdt_turnoff();

if (code == SYS_RESTART) {
/*
* Cobalt devices have no way of rebooting themselves other
* than getting the watchdog to pull reset, so we restart the
* watchdog on reboot with no heartbeat
*/
wdt_change(WDT_ENABLE);
pr_info("Watchdog timer is now enabled with no heartbeat - should reboot in ~1 second\n");
}
return NOTIFY_DONE;
}

For some systems the watchdog is the only way to reboot... So where we should put it, is not trivial neither...

Kind regards,
Wim.

2014-06-23 21:48:01

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On 06/23/2014 02:30 PM, Wim Van Sebroeck wrote:
> Hi All,
>
>> On Mon, Jun 23, 2014 at 07:30:56AM -0700, Guenter Roeck wrote:
>>>>> The patches _are_ in my watchdog-next branch and get some coverage from
>>>>> both my auto-builders and from Fenguang's build robots, so while they are
>>>>> not in linux-next, they are not completely in the dark either.
>>>>
>>>> So, this patch finally didn't make it into 3.16. Great. Now, we can't
>>>> even reboot the boards.
>>>>
>>>> Given how it's just impossible to get something merged reliably
>>>> through the watchdog tree, I guess I should just start merging the
>>>> patches through mine?
>>>>
>>>
>>> You can not really blame Wim here.
>>>
>>> In this case, I suspect the major reason for not accepting the patch
>>> is that I tried to provide a clean method / API for "reset through watchdog
>>> subsystem", which went nowhere, in my understanding because someone objected
>>> that it would be the wrong thing to do [1] and it didn't get approval /
>>> acceptance from the arm maintainers. If it is wrong to reset the board
>>> from the watchdog subsystem in a clean way, it is for sure even more wrong
>>> to do it as you proposed in your patch.
>>>
>>> My conclusion therefore is that all board reset code should move back out
>>> of the watchdog subsystem, and that we should not accept such code in the
>>> future. This is not my personal preference, but I do believe that we should
>>> do it in a clean way or not at all.
>>
>> Well, considering that this patch isn't depending on your reboot API
>> set, and that Wim never either commented on this patch, your reboot
>> API patchset or your pull request to say that he was not willing to
>> merge this, there's still a huge failure to communicate.
>>
>> I'm fine with any technical reason, let's debate on that. But the
>> point is there has been no debate at all, only silence from his side.
>>
>> I have been told some patches would be merged and I merged through my
>> tree some patches that were depending on this one based on that
>> assumption.
>>
>> And now, we have a regression.
>>
>> Anyway... I guess I should just revert some commits now.
>>
>
> To continue the discussion: I would like to add an excerpt from drivers/watchdog/alim7101_wdt.c
> /*
> * Notifier for system down
> */
>
> static int wdt_notify_sys(struct notifier_block *this,
> unsigned long code, void *unused)
> {
> if (code == SYS_DOWN || code == SYS_HALT)
> wdt_turnoff();
>
> if (code == SYS_RESTART) {
> /*
> * Cobalt devices have no way of rebooting themselves other
> * than getting the watchdog to pull reset, so we restart the
> * watchdog on reboot with no heartbeat
> */
> wdt_change(WDT_ENABLE);
> pr_info("Watchdog timer is now enabled with no heartbeat - should reboot in ~1 second\n");
> }
> return NOTIFY_DONE;
> }
>
> For some systems the watchdog is the only way to reboot... So where we should put it, is not trivial neither...
>

Agreed. The above definitely doesn't look like a good solution to me.

Guenter

2014-06-24 09:28:49

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On Monday 23 June 2014 14:47:48 Guenter Roeck wrote:
> > To continue the discussion: I would like to add an excerpt from drivers/watchdog/alim7101_wdt.c
> > /*
> > * Notifier for system down
> > */
> >
> > static int wdt_notify_sys(struct notifier_block *this,
> > unsigned long code, void *unused)
> > {
> > if (code == SYS_DOWN || code == SYS_HALT)
> > wdt_turnoff();
> >
> > if (code == SYS_RESTART) {
> > /*
> > * Cobalt devices have no way of rebooting themselves other
> > * than getting the watchdog to pull reset, so we restart the
> > * watchdog on reboot with no heartbeat
> > */
> > wdt_change(WDT_ENABLE);
> > pr_info("Watchdog timer is now enabled with no heartbeat - should reboot in ~1 second\n");
> > }
> > return NOTIFY_DONE;
> > }
> >
> > For some systems the watchdog is the only way to reboot... So where we should put it, is not trivial neither...
> >
>
> Agreed. The above definitely doesn't look like a good solution to me.
>

Right, at the very least, it should be a separate notifier: the existing
reboot_notifier is meant for things that need to happen /before/ reboot,
so adding something in there to actually trigger the reboot is by definition
racy against the other notifiers that may or may not get called after
this one.

Arnd

2014-06-24 14:50:11

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On Mon, Jun 23, 2014 at 11:13:24PM +0200, Wim Van Sebroeck wrote:
> Hi Maxime,
>
> > > > Guenter, since you seem to be the only responsive, may I suggest that
> > > > you start merging patches and do a pull request to either Wim or Linus
> > > > directly during the merge window?
> > > >
> > > I had prepared a pull request for Wim last weekend or so, but then there
> > > were more patches piling in and I got distracted, so I didn't have time
> > > to actually send it. I'll try again this weekend ... the kids should be
> > > busy learning for their finals, and I'll have Friday and Monday off
> > > from work, so I should be able to find the time.
> > >
> > > As for sending patches to Linus directly, well, Wim is the watchdog maintainer.
> > > I manage to upset enough people, and would not want to add Wim to the list.
> > >
> > > The patches _are_ in my watchdog-next branch and get some coverage from
> > > both my auto-builders and from Fenguang's build robots, so while they are
> > > not in linux-next, they are not completely in the dark either.
> >
> > So, this patch finally didn't make it into 3.16. Great. Now, we can't
> > even reboot the boards.
>
> 1) For me the discussion was not ended and needs further
> thinking. (And I just read some good ideas about it).

It would have been great for you to mention it then.

> 2) You never mentioned you needed this in for 3.16 and that things
> would break because of it.

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257690.html
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/259109.html

I thought it was pretty clear.

>
> So based on these 2 points why would I have to have put this in allready?
>
> > Given how it's just impossible to get something merged reliably
> > through the watchdog tree, I guess I should just start merging the
> > patches through mine?
>
> I agree that I have the problem of having only 24 hours in a day and
> that I lack time to communicate and that I am not good at
> communicating either, but I checked all sunxi related e-mails and
> you never mentioned the constraint to have it in for 3.16... But I
> do understand your frustration.

I totally understand the lack of time. A good way to ease your burden
and solve this situation is usually to take a co-maintainer. And given
that Guenter already reviews patches, maintains some branch, and is
developping some part of the framework, he seems up to the task.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


Attachments:
(No filename) (2.51 kB)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-06-24 16:01:33

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 1/6] wdt: sunxi: Move restart code to the watchdog driver

On 06/24/2014 07:49 AM, Maxime Ripard wrote:
> On Mon, Jun 23, 2014 at 11:13:24PM +0200, Wim Van Sebroeck wrote:
>> Hi Maxime,
>>
>>>>> Guenter, since you seem to be the only responsive, may I suggest that
>>>>> you start merging patches and do a pull request to either Wim or Linus
>>>>> directly during the merge window?
>>>>>
>>>> I had prepared a pull request for Wim last weekend or so, but then there
>>>> were more patches piling in and I got distracted, so I didn't have time
>>>> to actually send it. I'll try again this weekend ... the kids should be
>>>> busy learning for their finals, and I'll have Friday and Monday off
>>>> from work, so I should be able to find the time.
>>>>
>>>> As for sending patches to Linus directly, well, Wim is the watchdog maintainer.
>>>> I manage to upset enough people, and would not want to add Wim to the list.
>>>>
>>>> The patches _are_ in my watchdog-next branch and get some coverage from
>>>> both my auto-builders and from Fenguang's build robots, so while they are
>>>> not in linux-next, they are not completely in the dark either.
>>>
>>> So, this patch finally didn't make it into 3.16. Great. Now, we can't
>>> even reboot the boards.
>>
>> 1) For me the discussion was not ended and needs further
>> thinking. (And I just read some good ideas about it).
>
> It would have been great for you to mention it then.
>
>> 2) You never mentioned you needed this in for 3.16 and that things
>> would break because of it.
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257690.html
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/259109.html
>
> I thought it was pretty clear.
>
>>
>> So based on these 2 points why would I have to have put this in allready?
>>
>>> Given how it's just impossible to get something merged reliably
>>> through the watchdog tree, I guess I should just start merging the
>>> patches through mine?
>>
>> I agree that I have the problem of having only 24 hours in a day and
>> that I lack time to communicate and that I am not good at
>> communicating either, but I checked all sunxi related e-mails and
>> you never mentioned the constraint to have it in for 3.16... But I
>> do understand your frustration.
>
> I totally understand the lack of time. A good way to ease your burden
> and solve this situation is usually to take a co-maintainer. And given
> that Guenter already reviews patches, maintains some branch, and is
> developping some part of the framework, he seems up to the task.
>

Let's focus on the problem at hand. I prepared a set of patches to add a
kernel restart notifier, quite similar to the existing reboot notifier.
Only question is where it should reside. So far it is in parallel
to the reboot notifier, ie in kernel/notifier.c and kernel/reboot.c.
Before I send it out for review, I'd like to get a notion if this is
the right approach, or if it is going to create heat from other sides.
Thoughts, anyone ?

On the plus side, this might have the potential of replacing arm_pm_restart,
which I think would be a good thing.

Thanks,
Guenter