| Message ID | 22d22fe6eea294c5132e47b8901e094d60b0e99d.1371535242.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 18, 2013 at 08:50:25PM +0530, Viresh Kumar wrote: > This driver was broken since ever. > > - Interrupt request doesn't use the right API: The TWD watchdog uses a per-cpu > interrupt (usually interrupt #30), and the GIC configuration should flag it as > such. With this setup, request_irq() should fail, and the right API is > request_percpu_irq(), together with enable_percpu_irq()/disable_percpu_irq(). > > - Nothing ensures the userspace ioctl() will end-up kicking the watchdog on the > right CPU. > > Was last discussed here: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-April/095960.html > > Lets mark it broken until somebody with this hardware gets up and fixes it. > I must be missing something. What is the point of the remaining patches in this case ? Guenter > Suggested-by: Marc Zyngier <marc.zyngier@arm.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/watchdog/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 9d03af1..c7dabe9 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -223,7 +223,7 @@ config DW_WATCHDOG > > config MPCORE_WATCHDOG > tristate "MPcore watchdog" > - depends on HAVE_ARM_TWD > + depends on HAVE_ARM_TWD && BROKEN > help > Watchdog timer embedded into the MPcore system. > > -- > 1.7.12.rc2.18.g61b472e > > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 18/06/13 16:42, Guenter Roeck wrote: > On Tue, Jun 18, 2013 at 08:50:25PM +0530, Viresh Kumar wrote: >> This driver was broken since ever. >> >> - Interrupt request doesn't use the right API: The TWD watchdog uses a per-cpu >> interrupt (usually interrupt #30), and the GIC configuration should flag it as >> such. With this setup, request_irq() should fail, and the right API is >> request_percpu_irq(), together with enable_percpu_irq()/disable_percpu_irq(). >> >> - Nothing ensures the userspace ioctl() will end-up kicking the watchdog on the >> right CPU. >> >> Was last discussed here: >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-April/095960.html >> >> Lets mark it broken until somebody with this hardware gets up and fixes it. >> > I must be missing something. What is the point of the remaining patches in this > case ? Indeed. This looks like pointless churn to me, unless someone actually picks up the driver and fixes it for good. If nobody cares enough about it, then maybe it should be moved into staging and eventually retired... M.
On Tue, Jun 18, 2013 at 9:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 18/06/13 16:42, Guenter Roeck wrote: >> On Tue, Jun 18, 2013 at 08:50:25PM +0530, Viresh Kumar wrote: >>> This driver was broken since ever. >>> >>> - Interrupt request doesn't use the right API: The TWD watchdog uses a per-cpu >>> interrupt (usually interrupt #30), and the GIC configuration should flag it as >>> such. With this setup, request_irq() should fail, and the right API is >>> request_percpu_irq(), together with enable_percpu_irq()/disable_percpu_irq(). >>> >>> - Nothing ensures the userspace ioctl() will end-up kicking the watchdog on the >>> right CPU. >>> >>> Was last discussed here: >>> >>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-April/095960.html >>> >>> Lets mark it broken until somebody with this hardware gets up and fixes it. >>> >> I must be missing something. What is the point of the remaining patches in this >> case ? > > Indeed. This looks like pointless churn to me, unless someone actually > picks up the driver and fixes it for good. > > If nobody cares enough about it, then maybe it should be moved into > staging and eventually retired... That was a year ago, and nobody has done anything to the driver. Just remove it -- if someone wants to do the work later on it's easy to revert the commit and start over. Keeping code in the kernel but marking it BROKEN is only useful if we think someone will fix it soon. It seems very unlikely in this case. -Olof
Wow!! So many replies, let me reply to everyone in this chain. On 18 June 2013 22:05, Olof Johansson <olof@lixom.net> wrote: > On Tue, Jun 18, 2013 at 9:11 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On 18/06/13 16:42, Guenter Roeck wrote: >>> On Tue, Jun 18, 2013 at 08:50:25PM +0530, Viresh Kumar wrote: >>>> Lets mark it broken until somebody with this hardware gets up and fixes it. >>>> >>> I must be missing something. What is the point of the remaining patches in this >>> case ? In case somebody wakes up and tries to fix this driver, he doesn't have to write stuff which I already wrote. That's it. This stuff was pending in my tree for more than a year now and I wanted to get rid of it (without deleting it) :) >> Indeed. This looks like pointless churn to me, unless someone actually >> picks up the driver and fixes it for good. >> >> If nobody cares enough about it, then maybe it should be moved into >> staging and eventually retired... > > > That was a year ago, and nobody has done anything to the driver. Just > remove it -- if someone wants to do the work later on it's easy to > revert the commit and start over. > > Keeping code in the kernel but marking it BROKEN is only useful if we > think someone will fix it soon. It seems very unlikely in this case. I believed that this is the driver which will be used by all cortex family, i.e. all ARM SMP platforms, isn't it? I am sure atleast the A9 family had this. If no, then which ones are the real users of this driver/hardware? If yes, Why isn't anybody using this? I will send a patch that will delete this driver and will provide link to my patches, in case somebody wants it back in future. -- Viresh
On Wed, 19 Jun 2013 08:40:25 +0530, Viresh Kumar <viresh.kumar@linaro.org> wrote: > Wow!! So many replies, let me reply to everyone in this chain. > > On 18 June 2013 22:05, Olof Johansson <olof@lixom.net> wrote: >> On Tue, Jun 18, 2013 at 9:11 AM, Marc Zyngier <marc.zyngier@arm.com> >> wrote: >>> On 18/06/13 16:42, Guenter Roeck wrote: >>>> On Tue, Jun 18, 2013 at 08:50:25PM +0530, Viresh Kumar wrote: > >>>>> Lets mark it broken until somebody with this hardware gets up and >>>>> fixes it. >>>>> >>>> I must be missing something. What is the point of the remaining >>>> patches in this >>>> case ? > > In case somebody wakes up and tries to fix this driver, he doesn't have to > write stuff which I already wrote. That's it. This stuff was pending in my > tree > for more than a year now and I wanted to get rid of it (without deleting > it) :) > >>> Indeed. This looks like pointless churn to me, unless someone actually >>> picks up the driver and fixes it for good. >>> >>> If nobody cares enough about it, then maybe it should be moved into >>> staging and eventually retired... >> >> >> That was a year ago, and nobody has done anything to the driver. Just >> remove it -- if someone wants to do the work later on it's easy to >> revert the commit and start over. >> >> Keeping code in the kernel but marking it BROKEN is only useful if we >> think someone will fix it soon. It seems very unlikely in this case. > > I believed that this is the driver which will be used by all cortex > family, i.e. > all ARM SMP platforms, isn't it? I am sure atleast the A9 family had this. ARM11, A5 and A9 in their MP configurations only. > If no, then which ones are the real users of this driver/hardware? > If yes, Why isn't anybody using this? Because, as Russell mentioned, the piece of IP doesn't fit our watchdog model at all (per-CPU watchdog???), and most SoCs/boards have a separate watchdog anyway. > I will send a patch that will delete this driver and will provide link to > my > patches, in case somebody wants it back in future. Thanks. M.
On 19 June 2013 13:26, Marc Zyngier <marc.zyngier@arm.com> wrote: > ARM11, A5 and A9 in their MP configurations only. > >> If no, then which ones are the real users of this driver/hardware? >> If yes, Why isn't anybody using this? > > Because, as Russell mentioned, the piece of IP doesn't fit our watchdog > model at all (per-CPU watchdog???), and most SoCs/boards have a separate > watchdog anyway. I believe we should use mpcore watchdogs. Atleast on the platforms I have worked (SPEAr), they don't have an external watchdog. But yes, this would require somebody, with a A5/9/ARM11 board, no separate watchdog and a customer requirement, to work on it :) I will get rid of it. Thanks.
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 9d03af1..c7dabe9 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -223,7 +223,7 @@ config DW_WATCHDOG config MPCORE_WATCHDOG tristate "MPcore watchdog" - depends on HAVE_ARM_TWD + depends on HAVE_ARM_TWD && BROKEN help Watchdog timer embedded into the MPcore system.
This driver was broken since ever. - Interrupt request doesn't use the right API: The TWD watchdog uses a per-cpu interrupt (usually interrupt #30), and the GIC configuration should flag it as such. With this setup, request_irq() should fail, and the right API is request_percpu_irq(), together with enable_percpu_irq()/disable_percpu_irq(). - Nothing ensures the userspace ioctl() will end-up kicking the watchdog on the right CPU. Was last discussed here: http://lists.infradead.org/pipermail/linux-arm-kernel/2012-April/095960.html Lets mark it broken until somebody with this hardware gets up and fixes it. Suggested-by: Marc Zyngier <marc.zyngier@arm.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/watchdog/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)