lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <514D1273.8070902@wwwdotorg.org>
Date:	Fri, 22 Mar 2013 20:24:51 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Lubomir Rintel <lkundrak@...sk>
CC:	linux-kernel@...r.kernel.org, Wim Van Sebroeck <wim@...ana.be>,
	linux-rpi-kernel@...ts.infradead.org,
	linux-watchdog@...r.kernel.org
Subject: Re: [PATCH] watchdog: Add Broadcom BCM2708 watchdog timer driver

On 03/22/2013 06:55 AM, Lubomir Rintel wrote:
> Signed-off-by: Lubomir Rintel <lkundrak@...sk>

A commit description would be useful.

>  arch/arm/configs/bcm2835_defconfig |    4 +
>  drivers/watchdog/Kconfig           |   11 +++
>  drivers/watchdog/Makefile          |    1 +
>  drivers/watchdog/bcm2835_wdt.c     |  158 ++++++++++++++++++++++++++++++++++++

The changes to bcm2835_defconfig should be a separate patch, since they
would be applied in the BCM2835 ARM sub-arch tree, whereas the driver
patch would be applied to the watchdog driver tree.

> diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig

> +CONFIG_BCM2835_WDT=y
> +
>  CONFIG_MMC=y

That blank line is a little odd; was this defconfig change created using
"make savedefconfig"?

> diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c

> +static int heartbeat = -1;
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +static void __iomem *wdt_regs;
> +static DEFINE_SPINLOCK(wdog_lock);

Can these be stored in a dynamically-allocated structure, stored in the
device's drvdata?

> +static struct platform_driver bcm2835_wdt_driver = {
...
> +};
> +
> +module_platform_driver(bcm2835_wdt_driver);

I believe it's typical not to leave a blank line before
module_platform_driver();

A couple of general comments:

1)

This driver touches the same registers that
arch/arm/mach-bcm2835/bcm2835.c uses to implement reboot and "power
off". Some co-ordination might be necessary.

The implementation of bcm2835_power_off() could easily be moved into
this driver, to avoid some of the need for co-ordination.

Moving bcm2835_restart() would be more tricky, since the ARM machine
descriptor needs a pointer to that function. I guess the kernel probably
ensures that none of the code in this watchdog driver is running by the
time bcm2835_restart() is called, although perhaps it'd be better to
have mach-bcm2835/bcm2835.c and this driver share a lock?

2)

I'm curious where you got the documentation to write this driver; this
HW module isn't described in BCM2835-ARM-Peripherals.pdf. I assume this
is based on the downstream kernel driver? If so, at least some credit in
the commit description might be appropriate. At least the relevant
commit downstream already has an appropriate Signed-off-by line:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ