Re: IB700 watchdog

[Mike Larkin]

On Fri, Mar 18, 2016 at 08:18:30PM +0100, Mark Kettenis wrote:
> > Date: Thu, 17 Mar 2016 14:28:04 +0900
> > From: Masao Uebayashi 
> > 
> > This tiny driver is only meant for a backup watchdog configuration on
> > QEMU, whose ICH watchdog, supported by ichwdt(4), seems broken.
> > 
> > ib700wdt(4) supports only 0-30 second timeouts but in practice it should
> > be good enough.
> 

Wait. So we're saying "let's write an entirely new driver because qemu
has broken emulation somewhere else"? Why not just ask (nicely) the qemu
folks to fix the root cause, upstream?

I had a pretty good exchange with some of their devs in the past - can't
speak for all of them but the team I was working with before seemed very
receptive to accepting changes.

Writing a new device for this (nevermind the concerns kettenis and theo
are also mentioning) seems like the wrong approach.

-ml

> The problem is that isa-style probes like this are dangerous.  That is
> probably why you didn't enable this driver by default.  But if you
> don't enable the code, it will rot.
> 
> Is there a better way to detect this "device"?  For example in the
> acpi device tree?
> 
> > diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC
> > index fee3c2a..82e7de0 100644
> > --- a/sys/arch/amd64/conf/GENERIC
> > +++ b/sys/arch/amd64/conf/GENERIC
> > @@ -608,6 +608,7 @@ bktr0   at pci?
> >  radio* at bktr?
> >  
> >  #wdt0  at pci? # Ind Computer Source PCI-WDT50x driver
> > +#ib700wdt* at isa? port 0x441  # iBase 700 (IB700) Watchdog Timer
> >  
> >  # crypto support
> >  hifn*  at pci? # Hi/fn 7751 crypto card
> > diff --git a/sys/arch/i386/conf/GENERIC b/sys/arch/i386/conf/GENERIC
> > index 0984e5b..dbb7df6 100644
> > --- a/sys/arch/i386/conf/GENERIC
> > +++ b/sys/arch/i386/conf/GENERIC
> > @@ -108,6 +108,7 @@ geodesc* at pci?# Geode SC1100/SCx200 
> > IAOC
> >  wdt0   at pci? # Ind Computer Source PCI-WDT50x driver
> >  berkwdt0 at pci?   # Berkshire PCI-PC Watchdog driver
> >  pwdog0 at pci? # Quancom PWDOG1 watchdog timer
> > +#ib700wdt* at isa? port 0x441  # iBase 700 (IB700) Watchdog Timer
> >  
> >  # National Semiconductor LM7[89] and compatible hardware monitors
> >  lm0at isa? port 0x290
> > diff --git a/sys/dev/isa/files.isa b/sys/dev/isa/files.isa
> > index 863d11c..92a9ecc 100644
> > --- a/sys/dev/isa/files.isa
> > +++ b/sys/dev/isa/files.isa
> > @@ -380,3 +380,8 @@ filedev/isa/isagpio.c   isagpio
> >  #file  dev/isa/pcmcia_pcic.c   pcic | pcicmaster
> >  
> >  #file  dev/isa/pcmcia_isa.cpcmcia
> > +
> > +# iBase 700 (IB700) Watchdog Timer
> > +device ib700wdt
> > +attach ib700wdt at isa
> > +file   dev/isa/ib700wdt.c  ib700wdt
> > diff --git a/sys/dev/isa/ib700wdt.c b/sys/dev/isa/ib700wdt.c
> > new file mode 100644
> > index 000..cdad79f
> > --- /dev/null
> > +++ b/sys/dev/isa/ib700wdt.c
> > @@ -0,0 +1,142 @@
> > +/*
> > + * Copyright (c) 2016 Masao Uebayashi 
> > + *
> > + * Permission to use, copy, modify, and distribute this software for any
> > + * purpose with or without fee is hereby granted, provided that the above
> > + * copyright notice and this permission notice appear in all copies.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +
> > +/*
> > + * IB700 USER'S MANUAL
> > + * Version 1.0B
> > + * p28
> > + * Watchdog Timer Configuration
> > + */
> > +
> > +#defineIB700WDT_BASE   0x441
> > +#defineIB700WDT_SIZE   3
> > +#defineIB700WDT_DIS0   /* 0x441 */
> > +#defineIB700WDT_ENA2   /* 0x443 */
> > +#defineIB700WDT_SEC_MIN0
> > +#defineIB700WDT_SEC_MAX30
> > +
> > +static inline uint8_t
> > +ib700wdt_sec2val(int sec)
> > +{
> > +   /* 0/1sec = 0xf, 2/3sec = 0xe, ..., 28/29 = 0x1, 30sec = 0x0 */
> > +   return (0xf - (sec * 0xf / 30));
> > +}
> > +
> > +/*
> > + * Driver for iBase 700 Watchdog Timer
> > + * Mainly meant for QEMU's "ib700" watchdog device
> > + * Can't handle immediate reset (sec=0)
> > + */
> > +
> > +struct ib700wdt_softc {
> > +   struct device sc_dev;
> > +   bus_space_tag_t sc_iot;
> > +   bus_space_handle_t sc_ioh;
> > +   int sc_period;
> > +};
> > +
> > +int 

Re: IB700 watchdog

[Martin Pieuchot]

On 17/03/16(Thu) 14:28, Masao Uebayashi wrote:
> This tiny driver is only meant for a backup watchdog configuration on
> QEMU, whose ICH watchdog, supported by ichwdt(4), seems broken.

What's the problem?  Is it in our code or in QEMU?

Re: IB700 watchdog

[Mark Kettenis]

> Date: Thu, 17 Mar 2016 14:28:04 +0900
> From: Masao Uebayashi 
> 
> This tiny driver is only meant for a backup watchdog configuration on
> QEMU, whose ICH watchdog, supported by ichwdt(4), seems broken.
> 
> ib700wdt(4) supports only 0-30 second timeouts but in practice it should
> be good enough.

The problem is that isa-style probes like this are dangerous.  That is
probably why you didn't enable this driver by default.  But if you
don't enable the code, it will rot.

Is there a better way to detect this "device"?  For example in the
acpi device tree?

> diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC
> index fee3c2a..82e7de0 100644
> --- a/sys/arch/amd64/conf/GENERIC
> +++ b/sys/arch/amd64/conf/GENERIC
> @@ -608,6 +608,7 @@ bktr0 at pci?
>  radio*   at bktr?
>  
>  #wdt0at pci? # Ind Computer Source PCI-WDT50x driver
> +#ib700wdt* at isa? port 0x441# iBase 700 (IB700) Watchdog Timer
>  
>  # crypto support
>  hifn*at pci? # Hi/fn 7751 crypto card
> diff --git a/sys/arch/i386/conf/GENERIC b/sys/arch/i386/conf/GENERIC
> index 0984e5b..dbb7df6 100644
> --- a/sys/arch/i386/conf/GENERIC
> +++ b/sys/arch/i386/conf/GENERIC
> @@ -108,6 +108,7 @@ geodesc* at pci?  # Geode SC1100/SCx200 IAOC
>  wdt0 at pci? # Ind Computer Source PCI-WDT50x driver
>  berkwdt0 at pci? # Berkshire PCI-PC Watchdog driver
>  pwdog0   at pci? # Quancom PWDOG1 watchdog timer
> +#ib700wdt* at isa? port 0x441# iBase 700 (IB700) Watchdog Timer
>  
>  # National Semiconductor LM7[89] and compatible hardware monitors
>  lm0  at isa? port 0x290
> diff --git a/sys/dev/isa/files.isa b/sys/dev/isa/files.isa
> index 863d11c..92a9ecc 100644
> --- a/sys/dev/isa/files.isa
> +++ b/sys/dev/isa/files.isa
> @@ -380,3 +380,8 @@ file  dev/isa/isagpio.c   isagpio
>  #filedev/isa/pcmcia_pcic.c   pcic | pcicmaster
>  
>  #filedev/isa/pcmcia_isa.cpcmcia
> +
> +# iBase 700 (IB700) Watchdog Timer
> +device   ib700wdt
> +attach   ib700wdt at isa
> +file dev/isa/ib700wdt.c  ib700wdt
> diff --git a/sys/dev/isa/ib700wdt.c b/sys/dev/isa/ib700wdt.c
> new file mode 100644
> index 000..cdad79f
> --- /dev/null
> +++ b/sys/dev/isa/ib700wdt.c
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright (c) 2016 Masao Uebayashi 
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +/*
> + * IB700 USER'S MANUAL
> + * Version 1.0B
> + * p28
> + * Watchdog Timer Configuration
> + */
> +
> +#define  IB700WDT_BASE   0x441
> +#define  IB700WDT_SIZE   3
> +#define  IB700WDT_DIS0   /* 0x441 */
> +#define  IB700WDT_ENA2   /* 0x443 */
> +#define  IB700WDT_SEC_MIN0
> +#define  IB700WDT_SEC_MAX30
> +
> +static inline uint8_t
> +ib700wdt_sec2val(int sec)
> +{
> + /* 0/1sec = 0xf, 2/3sec = 0xe, ..., 28/29 = 0x1, 30sec = 0x0 */
> + return (0xf - (sec * 0xf / 30));
> +}
> +
> +/*
> + * Driver for iBase 700 Watchdog Timer
> + * Mainly meant for QEMU's "ib700" watchdog device
> + * Can't handle immediate reset (sec=0)
> + */
> +
> +struct ib700wdt_softc {
> + struct device sc_dev;
> + bus_space_tag_t sc_iot;
> + bus_space_handle_t sc_ioh;
> + int sc_period;
> +};
> +
> +int  ib700wdt_match(struct device *, void *, void *);
> +void ib700wdt_attach(struct device *, struct device *, void *);
> +int  ib700wdt_activate(struct device *, int);
> +
> +int  ib700wdt_cb(void *, int);
> +
> +struct cfattach ib700wdt_ca = {
> + sizeof(struct ib700wdt_softc),
> + ib700wdt_match, ib700wdt_attach, NULL, ib700wdt_activate
> +};
> +
> +struct cfdriver ib700wdt_cd = {
> + NULL, "ib700wdt", DV_DULL
> +};
> +
> +int
> +ib700wdt_match(struct device *parent, void *match, void *aux)
> +{
> + struct isa_attach_args *ia = aux;
> +
> + ia->ia_iosize = IB700WDT_SIZE;
> + ia->ia_msize = 0;
> +
> + return (1);
> +}
> +
> +void
> +ib700wdt_attach(struct device *parent, struct device *self, void *aux)
> +{
> + struct ib700wdt_softc *sc = 

Re: IB700 watchdog

[Theo de Raadt]

+#ib700wdt* at isa? port 0x441  # iBase 700 (IB700) Watchdog Timer

We don't do drivers with numbers in their names.

+int
+ib700wdt_match(struct device *parent, void *match, void *aux)
+{
+   struct isa_attach_args *ia = aux;
+
+   ia->ia_iosize = IB700WDT_SIZE;
+   ia->ia_msize = 0;
+
+   return (1);
+}

That does nothing.  OH LOOK!  My laptop has this chip.
So does my server!  Every machine has it.

+   if (period == 0) {
+   if (sc->sc_period != 0)
+   bus_space_write_1(sc->sc_iot, sc->sc_ioh, IB700WDT_DIS, 
0);
+   } else {

Now you write to a register, on all x86 machines.

No, that won't do at all.  Not even commented out, because the
pattern it teaches others is not right.