linux-kernel - Re: [PATCH 3/3] watchdog/aspeed: add support for dual boot

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]

Date:   Thu, 22 Aug 2019 12:15:20 +0300
From:   Ivan Mikhaylov <i.mikhaylov@...ro.com>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Joel Stanley <joel@....id.au>,
        Andrew Jeffery <andrew@...id.au>,
        <linux-watchdog@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-aspeed@...ts.ozlabs.org>, <linux-kernel@...r.kernel.org>,
        "Alexander Amelkin" <a.amelkin@...ro.com>
Subject: Re: [PATCH 3/3] watchdog/aspeed: add support for dual boot

On Wed, 2019-08-21 at 09:32 -0700, Guenter Roeck wrote:
> 
> > +	writel(WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION,
> > +			wdt->base + WDT_CLEAR_TIMEOUT_STATUS);
> > +	wdt->wdd.bootstatus |= WDIOF_EXTERN1;
> 
> The variable reflects the _boot status_. It should not change after booting.

Okay, then perhaps may we set 'status' handler for watchdog device and check 
'status' file? Right now 'bootstatus' and 'status' are same because there is no
handler for 'status'.

> > +
> > +	return size;
> > +}
> > +static DEVICE_ATTR_WO(access_cs0);
> > +
> > +static struct attribute *bswitch_attrs[] = {
> > +	&dev_attr_access_cs0.attr,
> > +	NULL
> > +};
> > +ATTRIBUTE_GROUPS(bswitch);
> > +
> >  static const struct watchdog_ops aspeed_wdt_ops = {
> >  	.start		= aspeed_wdt_start,
> >  	.stop		= aspeed_wdt_stop,
> > @@ -223,6 +248,9 @@ static int aspeed_wdt_probe(struct platform_device
> > *pdev)
> >  
> >  	wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> >  
> > +	if (of_property_read_bool(np, "aspeed,alt-boot"))
> > +		wdt->wdd.groups = bswitch_groups;
> > +
> Why does this have to be separate to the existing evaluation of
> aspeed,alt-boot, and why does the existing code not work ?
> 
> Also, is it guaranteed that this does not interfer with existing
> support for alt-boot ?

It doesn't, it just provides for ast2400 switch to cs0 at side 1(cs1). Problem
is that only one flash chip(side 1/cs1) is accessible on alternate boot, there
is citation from the documentation in commit body. So if by some reason side 0
is corrupted, need to switch into alternate boot to cs1, do the load from it,
drop that bit to make side 0 accessible and do the flash of first side. On
ast2500/2600 this problem is solved already, in alternate boot there both flash
chips are present. It's additional requirement for alternate boot on ast2400, to
make the possibility to access at all side 0 flash chip after we boot to the
alternate side.