Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling

[Guenter Roeck]

On 11/18/24 15:00, Andrew Jeffery wrote:
> On Mon, 2024-11-18 at 12:50 -0800, Guenter Roeck wrote:
> > On 11/18/24 04:46, Chin-Ting Kuo wrote:
> > > Hi Guenter,
> > >
> > > Thanks for the reply.
> > >
> > > > -----Original Message-----
> > > > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter
> > > > Roeck
> > > > Sent: Friday, November 8, 2024 10:08 PM
> > > > Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus
> > > > handling
> > > >
> > > > On 11/7/24 21:42, Chin-Ting Kuo wrote:
> > > >
> > > > > 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?
> > > >
> > > > This is a UAPI change. That is a major change. It needs to be
> > > > discussed
> > > > separately, on its own, and can not be sneaked in like this.
> > >
> > > Agree. However, how to trigger this discussion? Can I just send a
> > > new
> > > patch separately with only this UAPI modification? This is the
> > > first time
> > > I change such common source code.
> >
> > Yes. That needs to include arguments explaining why this specific new
> > flag
> > even adds value. I for my part don't immediately see that value.
>
> So maybe I was derailed with my WDIOF_REBOOT suggestion by the proposal
> to repurpose WDIOF_EXTERN1 to indicate a regular reboot. I still don't
> think repurposing WDIOF_EXTERN1 is the right direction. But, perhaps
> the thing to do for a regular reboot is to not set any reason flags at
> all? It just depends on whether we're wanting to separate a cold boot
> from a reboot (as they _may_ behave differently on Aspeed hardware), as
> on a cold boot we wouldn't set any reason flags either.


Thew point here is that this is just a warm reboot which happens to use
the watchdog as vehicle trigger a reset. A UAPI change along that line
would tell the user just that, and I don't see the value in it.

FWIW, the only really valuable flag is WDIOF_CARDRESET. All others are
at best misleading since the watchdog isn't typically involved in
making policy decisions such as WDIOF_OVERHEAT or WDIOF_FANFAULT and thus
can not report such reasons to userspace.

Unfortunately we can at best deprecate all those existing flags. At the same
time I really don't want to introduce new ones unless they provide real value.
While it might be valuable to know if a reboot was a cold or a warm reboot,
the watchdog subsystem is not involved in this either on almost all
platforms, meaning userspace can not really rely on it. Yes, Aspeed
hardware may behave differently, but typically all those differences
would need to be handled in the kernel when (re-)initializing the hardware,
not in userspace.

So, again, what exactly would userspace do with the information that this
was a watchdog triggered warm reboot ? Why would it need that information ?

Thanks,
Guenter