On 07/10/08 09:44 +0200, Andi Kleen wrote:
> Jordan Crouse <jordan.crouse@xxxxxxx> writes:
>
> > Attached is a patch to support watchdog timers as described
> > by the WDRT (Watch Dog Resource Table). This has been tested on
> > the AMD SB600 chipset, but it should work for any chipset / BIOS
> > that implements the WDRT table.
>
> Nice. I remember thinking about writing such a driver a long time
> ago.
>
> > I'm not in love with the name - but I couldn't think of anything
> > better, so I'm open to suggestions. I have a limited number of
> > systems that implement this interface, so I would be very happy
> > to hear from other users, especially ones not using AMD chipsets.
>
> acpi-watchdog ?
Sure. The only thing I worry about with "acpi" in the name is that
somebody think this might be a watchdog implemented by ACPI, which
is a half truth. You also need the hardware interface to come along
for the ride.
> > +struct wdrt_table {
> > + struct acpi_table_header header;
> > + struct acpi_generic_address ctrl_addr;
> > + struct acpi_generic_address count_addr;
> > + u16 pci_devid;
> > + u16 pci_venid;
> > + u8 pci_bus;
> > + u8 pci_device;
> > + u8 pci_function;
> > + u8 pci_segment;
>
> You don't seem to use the PCI device information currently, but
> they would be needed on IOMMU enabled systems to map the correct
> device, wouldn't they?
I think so, but I have to admit a certain amount of ignorance about
how the IOMMU operates. All the ACPI implementations I've seen that
include the WDRT are not IOMMU aware (yet).
> > +static int acpiwdt_set_heartbeat(unsigned int interval)
> > +{
> > + u32 val;
> > +
> > + /* Make sure the interval is sane */
> > +
> > + if (((interval * 1000) / wdt_units) > wdt_max_count)
> > + return -EINVAL;
>
> It's probably not a big issue in this context, but the check
> can overflow and miss wrong intervals.
Okay - I'll adjust it.
> > +
> > + /* Enable the timer */
> > +
> > + val = readl(wdt_ctrl_reg);
> > + writel(val | WDT_CTRL_RUN, wdt_ctrl_reg);
> > +
> > + timeout = interval;
> > +
> > + return 0;
> > +}
> > +
> > +static int acpiwdt_open(struct inode *inode, struct file *file)
> > +{
> > + if (test_and_set_bit(WDT_FLAGS_OPEN, &wdt_flags))
> > + return -EBUSY;
> > + if (!test_and_clear_bit(WDT_FLAGS_ORPHAN, &wdt_flags))
> > + __module_get(THIS_MODULE);
>
> This shouldn't be needed because the VFS does that. Also doing it in
> the module itself is always racy because the module can be unloaded
> before it increases its count. That is why the VFS does it.
>
> I suppose you copied that from somehere, that somewhere is also wrong
> and should be ideally fixed too.
Yep - I shared this from the the geodewdt driver, which got it from
one of the other drivers in the directory.
> In general these watchdog drivers seem to have all a lot of copied
> code and at some point it would be nice to write a generic driver
> where the backend code is just plugged in.
This is my second WDT driver in the last year, and I have to admit,
that idea occurred to me as well.
Jordan
--
Jordan Crouse
Systems Software Development Engineer
Advanced Micro Devices, Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
