linux-kernel - RE: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

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:
 <TYZPR06MB52039B02B6D3053355F30489B25D2@TYZPR06MB5203.apcprd06.prod.outlook.com>
Date: Fri, 8 Nov 2024 05:42:21 +0000
From: Chin-Ting Kuo <chin-ting_kuo@...eedtech.com>
To: Andrew Jeffery <andrew@...econstruct.com.au>, Patrick Williams
	<patrick@...cx.xyz>, "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
	"linux@...ck-us.net" <linux@...ck-us.net>
CC: "joel@....id.au" <joel@....id.au>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
	<linux-aspeed@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-watchdog@...r.kernel.org"
	<linux-watchdog@...r.kernel.org>, "Peter.Yin@...ntatw.com"
	<Peter.Yin@...ntatw.com>, "Patrick_NC_Lin@...ynn.com"
	<Patrick_NC_Lin@...ynn.com>, "Bonnie_Lo@...ynn.com" <Bonnie_Lo@...ynn.com>,
	"DELPHINE_CHIU@...ynn.com" <DELPHINE_CHIU@...ynn.com>, BMC-SW
	<BMC-SW@...eedtech.com>, "chnguyen@...erecomputing.com"
	<chnguyen@...erecomputing.com>
Subject: RE: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

Hi Andrew,

> -----Original Message-----
> From: Andrew Jeffery <andrew@...econstruct.com.au>
> Sent: Friday, November 8, 2024 7:50 AM
> Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling
> 
> Hi Chin-Ting,
> 
> On Thu, 2024-11-07 at 05:35 +0000, Chin-Ting Kuo wrote:
> > Hi Andrew,
> >
> > Thanks for the check.
> >
> > > -----Original Message-----
> > > From: Andrew Jeffery <andrew@...econstruct.com.au>
> > > Sent: Monday, November 4, 2024 8:02 AM
> > > Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus
> > > handling
> > >
> > > On Fri, 2024-11-01 at 14:21 -0400, Patrick Williams wrote:
> > > > On Fri, Nov 01, 2024 at 08:11:59PM +0800, Chin-Ting Kuo wrote:
> > > > > The boot status mapping rule follows the latest design guide
> > > > > from the OpenBMC shown as below.
> > > > > https://github.com/openbmc/docs/blob/master/designs/bmc-reboot-c
> > > > > ause
> > > > > -update.md#proposed-design
> > > > > - WDIOF_EXTERN1   => system is reset by Software
> > > > > - WDIOF_CARDRESET => system is reset by WDT SoC reset
> > > > > - Others          => other reset events, e.g., power on reset.
> > > >
> > > > I'm quite surprised that the above is relevant for a kernel driver
> > > > at all.  Isn't "EXTERN1" a name of a real watchdog signal from
> > > > your hardware (my recollection is that there are 2 external
> > > > watchdogs).
> > >
> > > I think you may be referring to WDTRST1 (and WDTRST2) here.
> > >
> >
> > WDTRST1, wdt_ext, is a pulse signal generated when WDT timeout occurs.
> > However, depending on the HW board design, wdt_ext doesn’t always
> > affect the system reset. Thus, EXTERN1 boot status can be ignored in
> > ASPEED WDT driver and just implement "CARDRESET" and "others" types
> > since EXTERN1 is not always affected/controlled by WDT controller
> > directly. Or, an additional property in dts can be added to
> > distinguish whether the current EXTRST# reset event is triggered by
> > wdt_ext signal.
> 
> Yep, I understand how it works. I was responding to Patrick's query to clear up
> some confusion around the watchdog signal names.
> 

Okay.

> > >
> > > >
> > > > Having said that, it was known that there would need to be changes
> > > > to the driver because some of these conditions were not adequately
> > > > exposed at all.  I'm just still surprised that we're needing to
> > > > reference that document as part of these changes.
> > >
> > > I think the main question is whether an internal, graceful
> > > (userspace-
> > > requested) reset is a reasonable use of WDIOF_EXTERN[12]. My feeling
> > > no. I wonder whether defining a new flag (WDIOF_REBOOT?
> > > WDIOF_GRACEFUL?) in the UAPI would be acceptable?
> > >
> >
> > Agree, but this is out of the scope of this patch series and can be
> > discussed and
> > implemented in the other future patches.
> 
> I disagree, because then you're changing the userspace-visible
> behaviour of the driver yet again. I don't prefer the proposed patch as
> the way forward because I think it is abusing the meaning of
> WDIOF_EXTERN1. I think the concept needs input from the watchdog
> maintainers.
> 

Previously, I meant that only implement "CARDRESET " and "others"
reset type to complete the current driver for "CARDRESET ".
The implementation of SW restart mechanism can be postponed to the
next new patch series.

But now, I think it will be better to add a patch for creating a new
reset reason, e.g., WDIOF_REBOOT or WDIOF_RESTART, in watchdog.h
of uapi. Can I include this change, creating a new reset reason, in
this patch series? Or, should I create an extra new patch series for
this purpose?

> Andrew

Chin-Ting