linux-kernel - Re: [PATCH 1/2] watchdog: Add Cadence WDT driver

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 1 Apr 2014 13:05:53 +0530
From:	Harini Katakam <harinikatakamlinux@...il.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Rob Landley <rob@...dley.net>, michals@...inx.com,
	linux-watchdog@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	Harini Katakam <harinik@...inx.com>
Subject: Re: [PATCH 1/2] watchdog: Add Cadence WDT driver

Hi,

On Tue, Apr 1, 2014 at 6:02 AM, Guenter Roeck <linux@...ck-us.net> wrote:
> On 03/28/2014 03:31 AM, Harini Katakam wrote:
>>
>> Add Cadence WDT driver. This is used by Xilinx Zynq.
>>
>> Signed-off-by: Harini Katakam <harinik@...inx.com>
>> ---
>>   drivers/watchdog/Kconfig       |    7 +
>>   drivers/watchdog/Makefile      |    1 +
>>   drivers/watchdog/cadence_wdt.c |  530
>> ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 538 insertions(+)
>>   create mode 100644 drivers/watchdog/cadence_wdt.c
>>

<snip>

>> +
>> +       spin_lock(&wdt->io_lock);
>> +       cdns_wdt_writereg(wdt->regs + CDNS_WDT_ZMR_OFFSET,
>> +                         CDNS_WDT_ZMR_ZKEY_VAL);
>> +
>> +       /* Shift the count value to correct bit positions */
>> +       count = (count << 2) & CDNS_WDT_CCR_CRV_MASK;
>> +
>> +       /* Write counter access key first to be able write to register */
>> +       data = count | CDNS_WDT_REGISTER_ACCESS_KEY | wdt->ctrl_clksel;
>> +       cdns_wdt_writereg(wdt->regs + CDNS_WDT_CCR_OFFSET, data);
>> +       data = CDNS_WDT_ZMR_WDEN_MASK | CDNS_WDT_ZMR_RSTLEN_16 |
>> +              CDNS_WDT_ZMR_ZKEY_VAL;
>> +
>> +       /* Reset on timeout if specified in device tree. */
>> +       if (wdt->rst) {
>> +               data |= CDNS_WDT_ZMR_RSTEN_MASK;
>> +               data &= ~CDNS_WDT_ZMR_IRQEN_MASK;
>> +       } else {
>> +               data &= ~CDNS_WDT_ZMR_RSTEN_MASK;
>> +               data |= CDNS_WDT_ZMR_IRQEN_MASK;
>> +       }
>> +       cdns_wdt_writereg(wdt->regs + CDNS_WDT_ZMR_OFFSET, data);
>> +       spin_unlock(&wdt->io_lock);
>> +       cdns_wdt_writereg(wdt->regs + CDNS_WDT_RESTART_OFFSET,
>> +                         CDNS_WDT_RESTART_KEY);
>> +
>
> Sometimes you write into this register with lock protection, here without.
>
> How comes (or, in other words, why does the write have to be spinlock
> protected above but not here) ?
>

CDNS_WDT_RESTART_OFFSET needs to be written here with lock protection.
I missed it because it is in wdt_start, but i realise it should be protected.
Will correct that.

<snip>

>> +       /* Initialize the members of cdns_wdt structure */
>> +       cdns_wdt_device->parent = &pdev->dev;
>> +       of_get_property(pdev->dev.of_node, "timeout-sec",
>> +                       &cdns_wdt_device->timeout);
>> +       if (wdt_timeout < CDNS_WDT_MAX_TIMEOUT &&
>> +           wdt_timeout > CDNS_WDT_MIN_TIMEOUT)
>
>
> Why are CDNS_WDT_MAX_TIMEOUT and CDNS_WDT_MIN_TIMEOUT not acceptable ?
> Why don't you use watchdog_init_timeout() anyway ?
>

I'll use watchdog_init_timeout.

>
>> +               cdns_wdt_device->timeout = wdt_timeout;
>> +       else
>> +               dev_info(&pdev->dev,
>> +                        "timeout limited to 1 - %d sec, using
>> default=%d\n",
>> +                        CDNS_WDT_MAX_TIMEOUT, CDNS_WDT_DEFAULT_TIMEOUT);
>> +
>> +       watchdog_set_nowayout(cdns_wdt_device, nowayout);
>> +       watchdog_set_drvdata(cdns_wdt_device, wdt);
>> +
>
>
> Set too late (see other e-mail)
>

Will correct order in probe as pointed in other mails.

Regards,
Harini
--
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/