linux-kernel - Re: [PATCH 2/3] watchdog: orion: don't enable rstout if an interrupt is configured

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: <4f82b24c3b244d3eba0f8de8cb25049f@svr-chch-ex1.atlnz.lc>
Date:   Wed, 11 Oct 2017 04:30:50 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Guenter Roeck <linux@...ck-us.net>,
        "wim@...ana.be" <wim@...ana.be>,
        "gregory.clement@...e-electrons.com" 
        <gregory.clement@...e-electrons.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] watchdog: orion: don't enable rstout if an interrupt
 is configured

On 11/10/17 16:42, Guenter Roeck wrote:
> On 10/10/2017 07:29 PM, Chris Packham wrote:
>> The orion_wdt_irq invokes panic() so we are going to reset the CPU
>> regardless.  By not setting this bit we get a chance to gather debug
>> from the panic output before the system is reset.
>>
>> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> 
> Unless I am missing something, this assumes that the interrupt is
> handled, ie that the system is not stuck with interrupts disabled.
> This makes the watchdog less reliable. This added verbosity comes
> at a significant cost. I'd like to get input from others if this
> is acceptable.
> 
> That would be different if there was a means to configure a pretimeout,
> ie a means to tell the system to generate an irq first, followed by a
> hard reset if the interrupt is not served. that does not seem to be
> the case here, though.
> 

As far as I can tell there is no pretimeout capability in the orion 
/armada WDT. I have got a work-in-progress patch that enables the RSTOUT 
in the interrupt handler which some-what mitigates your concern but 
still requires the interrupt to be handled at least once.

Another option would be to use one of the spare global timers to provide 
the interrupt-driven aspect.

Of course if the irq isn't specified then the existing behaviour is 
retained which would make the 3/3 patch of this series the debatable part.


> Guenter
> 
>> ---
>>    drivers/watchdog/orion_wdt.c | 25 +++++++++++++++++--------
>>    1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
>> index ea676d233e1e..ce88f339ef7f 100644
>> --- a/drivers/watchdog/orion_wdt.c
>> +++ b/drivers/watchdog/orion_wdt.c
>> @@ -71,6 +71,7 @@ struct orion_watchdog {
>>    	unsigned long clk_rate;
>>    	struct clk *clk;
>>    	const struct orion_watchdog_data *data;
>> +	int irq;
>>    };
>>    
>>    static int orion_wdt_clock_init(struct platform_device *pdev,
>> @@ -203,9 +204,11 @@ static int armada375_start(struct watchdog_device *wdt_dev)
>>    						dev->data->wdt_enable_bit);
>>    
>>    	/* Enable reset on watchdog */
>> -	reg = readl(dev->rstout);
>> -	reg |= dev->data->rstout_enable_bit;
>> -	writel(reg, dev->rstout);
>> +	if (!dev->irq) {
>> +		reg = readl(dev->rstout);
>> +		reg |= dev->data->rstout_enable_bit;
>> +		writel(reg, dev->rstout);
>> +	}
>>    
>>    	atomic_io_modify(dev->rstout_mask, dev->data->rstout_mask_bit, 0);
>>    	return 0;
>> @@ -228,9 +231,12 @@ static int armada370_start(struct watchdog_device *wdt_dev)
>>    						dev->data->wdt_enable_bit);
>>    
>>    	/* Enable reset on watchdog */
>> -	reg = readl(dev->rstout);
>> -	reg |= dev->data->rstout_enable_bit;
>> -	writel(reg, dev->rstout);
>> +	if (!dev->irq) {
>> +		reg = readl(dev->rstout);
>> +		reg |= dev->data->rstout_enable_bit;
>> +		writel(reg, dev->rstout);
>> +	}
>> +
>>    	return 0;
>>    }
>>    
>> @@ -247,8 +253,9 @@ static int orion_start(struct watchdog_device *wdt_dev)
>>    						dev->data->wdt_enable_bit);
>>    
>>    	/* Enable reset on watchdog */
>> -	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
>> -				      dev->data->rstout_enable_bit);
>> +	if (!dev->irq)
>> +		atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
>> +					      dev->data->rstout_enable_bit);
>>    
>>    	return 0;
>>    }
>> @@ -595,6 +602,8 @@ static int orion_wdt_probe(struct platform_device *pdev)
>>    			dev_err(&pdev->dev, "failed to request IRQ\n");
>>    			goto disable_clk;
>>    		}
>> +
>> +		dev->irq = irq;
>>    	}
>>    
>>    	watchdog_set_nowayout(&dev->wdt, nowayout);
>>
> 
>