| 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
| ||
|
Date: Tue, 02 Jun 2015 10:07:33 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Fu Wei <fu.wei@...aro.org>, Timur Tabi <timur@...eaurora.org>
CC: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
Linaro ACPI Mailman List <linaro-acpi@...ts.linaro.org>,
linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
Wei Fu <tekkamanninja@...il.com>,
G Gregory <graeme.gregory@...aro.org>,
Al Stone <al.stone@...aro.org>,
Hanjun Guo <hanjun.guo@...aro.org>,
Ashwin Chaugule <ashwin.chaugule@...aro.org>,
Arnd Bergmann <arnd@...db.de>, vgandhi@...eaurora.org,
wim@...ana.be, Jon Masters <jcm@...hat.com>,
Leo Duran <leo.duran@....com>, Jon Corbet <corbet@....net>,
Mark Rutland <mark.rutland@....com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>, rjw@...ysocki.net
Subject: Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On 06/02/2015 09:55 AM, Fu Wei wrote:
> Hi Timur,
>
> Thanks , feedback inline
>
> On 2 June 2015 at 23:32, Timur Tabi <timur@...eaurora.org> wrote:
>> On 06/01/2015 11:05 PM, fu.wei@...aro.org wrote:
>>
>>> +/*
>>> + * help functions for accessing 32bit registers of SBSA Generic Watchdog
>>> + */
>>> +static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,
>>> + struct watchdog_device *wdd)
>>> +{
>>> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>>> +
>>> + writel_relaxed(val, gwdt->control_base + reg);
>>> +}
>>> +
>>> +static void sbsa_gwdt_rf_write(unsigned int reg, u32 val,
>>> + struct watchdog_device *wdd)
>>> +{
>>> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>>> +
>>> + writel_relaxed(val, gwdt->refresh_base + reg);
>>> +}
>>> +
>>> +static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device
>>> *wdd)
>>> +{
>>> + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
>>> +
>>> + return readl_relaxed(gwdt->control_base + reg);
>>> +}
>>
>>
>> I still think you should get rid of these functions and just call
>> readl_relaxed() and writel_relaxed() every time, but I won't complain again
>> if you keep them.
>
> yes, that make sense, and will reduce the size of code, and I think
> the code's readability will be OK too.
> will try in my next patch,
>
>>
>>> +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
>>> +{
>>> + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
>>> + struct watchdog_device *wdd = &gwdt->wdd;
>>> +
>>> + if (wdd->pretimeout)
>>> + /* The pretimeout is valid, go panic */
>>> + panic("SBSA Watchdog pre-timeout");
>>> + else
>>> + /* We don't use pretimeout, trigger WS1 now*/
>>> + sbsa_gwdt_set_wcv(wdd, 0);
>>
>>
>> I don't like this.
>
> If so, what is your idea ,if pretimeout == 0?
>
> the reason of using WCV as (timout - pretimeout): it can provide the
> longer timeout period,
> (1)If we use WOR, it can only provide 10s @ 400MHz(max).
> as Guenter said earlier, the default timer out for most watchdog will
> be 30s, so I think 10s limit will be a little short
> (2)we can always program WCV just like ping.
> (3)if a timeout arrives, WOR will be use, so use it as pretimeout, but
> we still can make this pretimeout longer by programming WCV(I don't
> think it's necessary)
>
>
>> The triggering of the hardware reset should never depend
>> on an interrupt being handled properly.
>
> if this fail, system reset in 1S, because WOR == (1s)
>
So ?
>> You should always program WCV
>> correctly in advance. This is especially true since pre-timeout will
>> probably rarely be used.
>
> always programming WCV is doable. But I absolutely can not agree "
> pre-timeout will probably rarely be used"
> If so, SBSA watchdog is just a normal watchdog, This use case just
> makes this HW useless.
> If so, go to use SP805.
> you still don't see the importance of this warning and pretimeout to a
> real server.
>
If pretimeout isn't used, why not just set WCV = timeout, WOR = 0 ?
Guenter
> If the software of a real server goes wrong, then you just directly restart it ,
> never try to sync/protect the current data, never try to figure out
> what is wrong with it.
> I don't think that is a good server software.
>
> At least, I don't thinks " pre-timeout will probably rarely be used"
> is a good idea for a server.
> in another word, in a server ,pre-timeout should always be used.
>
>> So what happens if the CPU is totally hung and
>
> Again, system reset in 1S, because WOR == (1s).
>
>> this interrupt handler is never called? When will the timeout occur?
>
> if this interrupt hardler is never called, what I can see is "some
> one is feeding the dog".
> OK, in case, WS0 is triggered, but this interrupt hardler isn't
> called, then software really goes wrong. Then , Again, Again system
> reset in 1S, because WOR == (1s).
>
>>
>>> +static int sbsa_gwdt_probe(struct platform_device *pdev)
>>> +{
>>> + u64 first_period_max = U64_MAX;
>>> + struct device *dev = &pdev->dev;
>>> + struct watchdog_device *wdd;
>>> + struct sbsa_gwdt *gwdt;
>>> + struct resource *res;
>>> + void *rf_base, *cf_base;
>>> + int ret, irq;
>>> + u32 status;
>>> +
>>> + gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
>>> + if (!gwdt)
>>> + return -ENOMEM;
>>> + platform_set_drvdata(pdev, gwdt);
>>
>>
>> You should probably do this *after* calling platform_get_irq_byname().
>
> it just dose (pdev->) dev->driver_data = gwdt
> If we got gwdt, we can do that.
>
> But maybe I miss something(or a rule of usage), so please let know
> why this has to be called *after* calling platform_get_irq_byname().
>
>>
>>> +
>>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> "refresh");
>>> + rf_base = devm_ioremap_resource(dev, res);
>>> + if (IS_ERR(rf_base))
>>> + return PTR_ERR(rf_base);
>>> +
>>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> "control");
>>> + cf_base = devm_ioremap_resource(dev, res);
>>> + if (IS_ERR(cf_base))
>>> + return PTR_ERR(cf_base);
>>> +
>>> + irq = platform_get_irq_byname(pdev, "ws0");
>>> + if (irq < 0) {
>>> + dev_err(dev, "unable to get ws0 interrupt.\n");
>>> + return irq;
>>> + }
>>> +
>>> + /*
>>> + * Get the frequency of system counter from the cp15 interface of
>>> ARM
>>> + * Generic timer. We don't need to check it, because if it returns
>>> "0",
>>> + * system would panic in very early stage.
>>> + */
>>> + gwdt->clk = arch_timer_get_cntfrq();
>>> + gwdt->refresh_base = rf_base;
>>> + gwdt->control_base = cf_base;
>>> +
>>> + wdd = &gwdt->wdd;
>>> + wdd->parent = dev;
>>> + wdd->info = &sbsa_gwdt_info;
>>> + wdd->ops = &sbsa_gwdt_ops;
>>> + watchdog_set_drvdata(wdd, gwdt);
>>> + watchdog_set_nowayout(wdd, nowayout);
>>> +
>>> + wdd->min_pretimeout = 0;
>>> + wdd->max_pretimeout = U32_MAX / gwdt->clk;
>>> + wdd->min_timeout = 1;
>>> + do_div(first_period_max, gwdt->clk);
>>> + wdd->max_timeout = first_period_max;
>>> +
>>> + wdd->pretimeout = DEFAULT_PRETIMEOUT;
>>> + wdd->timeout = DEFAULT_TIMEOUT;
>>> + watchdog_init_timeouts(wdd, pretimeout, timeout, dev);
>>> +
>>> + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
>>> + if (status & SBSA_GWDT_WCS_WS1) {
>>> + dev_warn(dev, "System reset by WDT(WCV: %llx)\n",
>>> + sbsa_gwdt_get_wcv(wdd));
>>
>>
>> "System was previously reset via watchdog" is much clearer.
>
> OK
>
>>
>>> + wdd->bootstatus |= WDIOF_CARDRESET;
>>> + }
>>> + /* Check if watchdog is already enabled */
>>> + if (status & SBSA_GWDT_WCS_EN) {
>>> + dev_warn(dev, "already enabled!\n");
>>
>>
>> "watchdog is already enabled".
>
> I think I don't need to print "watchdog", dev_warn(dev, has help us on this.
> If you do so , the message will be "watchdog: watchdog0: watchdog is
> already enabled"
>
> > Never use exclamation marks in kernel
>> messages.
>
> that make sense , will delete it.
>
>>
>>> + sbsa_gwdt_keepalive(wdd);
>>> + }
>>> +
>>> + /* update pretimeout to WOR */
>>> + sbsa_gwdt_set_pretimeout(wdd, wdd->pretimeout);
>>> +
>>> + ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
>>> + pdev->name, gwdt);
>>> + if (ret) {
>>> + dev_err(dev, "unable to request IRQ %d\n", irq);
>>> + return ret;
>>> + }
>>> +
>>> + ret = watchdog_register_device(wdd);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @ %u
>>> Hz\n",
>>> + wdd->timeout, wdd->pretimeout, gwdt->clk);
>>
>>
>> if (wdd->pretimeout)
>> "watchdog initialized to %us timeout and %us pre-timeout at %u
>> Hz\n", wdd->timeout, wdd->pretimeout, gwdt->clk
>> else
>> "watchdog initialized to %us timeout at %u Hz\n", wdd->timeout,
>> gwdt->clk
>>
>> I think it's unlikely that users will use pre-timeout, so your code should
>> treat pre-timeout as a special case, not the normal usage.
>
> I don't think so, that why you didn't use pretimeout in your driver.
> Because you don't see the meaning of "pretimeout" to a server.
>
>>
>> --
>> Qualcomm Innovation Center, Inc.
>> The Qualcomm Innovation Center, Inc. is a member of the
>> Code Aurora Forum, a Linux Foundation Collaborative Project.
>
>
>
--
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