| 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
| ||
|
Message-ID:
<TYZPR06MB52034ABFD03BE4730BD9E097B2552@TYZPR06MB5203.apcprd06.prod.outlook.com>
Date: Thu, 31 Oct 2024 06:24:59 +0000
From: Chin-Ting Kuo <chin-ting_kuo@...eedtech.com>
To: Andrew Jeffery <andrew@...econstruct.com.au>, "patrick@...cx.xyz"
<patrick@...cx.xyz>, "robh@...nel.org" <robh@...nel.org>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
<conor+dt@...nel.org>, "joel@....id.au" <joel@....id.au>,
"wim@...ux-watchdog.org" <wim@...ux-watchdog.org>, "linux@...ck-us.net"
<linux@...ck-us.net>, "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>
CC: "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 v3 1/2] watchdog: aspeed: Update bootstatus handling
Hi Andrew,
> -----Original Message-----
> From: Andrew Jeffery <andrew@...econstruct.com.au>
> Sent: Thursday, October 31, 2024 7:54 AM
> Subject: Re: [PATCH v3 1/2] watchdog: aspeed: Update bootstatus handling
>
> Hello,
>
> On Wed, 2024-10-30 at 18:47 +0800, Chin-Ting Kuo wrote:
> > Update the bootstatus according to the latest design guide from the
> > OpenBMC shown as below.
> > https://github.com/openbmc/docs/blob/master/designs/bmc-reboot-cause-u
> > pdate.md#proposed-design
> >
> > In short,
> > - WDIOF_EXTERN1 => system is reset by Software
> > - WDIOF_CARDRESET => system is reset by WDT
> > - Others => other reset events, e.g., power on reset.
>
> ...
>
> >
> > On AST2400 platform, only a bit, SCU3C[1], represents that the system
> > is reset by WDT1 or WDT2.
> >
> > On AST2500 platform, SCU3C[4:2] are WDT reset flags.
> > SCU3C[4]: system is reset by WDT3.
> > SCU3C[3]: system is reset by WDT2.
> > SCU3C[2]: system is reset by WDT1.
> >
> > On AST2600 platform, SCU074[31:16] are WDT reset flags.
> > SCU074[31:28]: system is reset by WDT4
> > SCU074[31]: system is reset by WDT4 software reset.
> > SCU074[27:24]: system is reset by WDT3
> > SCU074[27]: system is reset by WDT3 software reset.
> > SCU074[23:20]: system is reset by WDT2
> > SCU074[23]: system is reset by WDT2 software reset.
> > SCU074[19:16]: system is reset by WDT1
> > SCU074[19]: system is reset by WDT1 software reset.
>
> This information emerges in the code (though it's a bit of a maze back through
> the derivations). I'd rather you discuss how the observable behaviour of the
> driver changes (with respect to booting from the alternate boot region) and
> what choices you've made when the hardware doesn't tell us whether this is a
> (graceful) software reset.
>
Okay, the commit message will be updated in the next patch version.
For booting from alternate region, it is also triggered by HW WDT SoC reset which is classified as card reset.
This information will also be updated in the next patch version.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@...eedtech.com>
> > ---
> > drivers/watchdog/aspeed_wdt.c | 114
> +++++++++++++++++++++++++++++++-
> > --
> > 1 file changed, 106 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/watchdog/aspeed_wdt.c
> > b/drivers/watchdog/aspeed_wdt.c index b4773a6aaf8c..add76be3ee42
> > 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -11,10 +11,12 @@
> > #include <linux/io.h>
> > #include <linux/kernel.h>
> > #include <linux/kstrtox.h>
> > +#include <linux/mfd/syscon.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/of_irq.h>
> > #include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > #include <linux/watchdog.h>
> >
> > static bool nowayout = WATCHDOG_NOWAYOUT; @@ -22,15 +24,44 @@
> > module_param(nowayout, bool, 0);
> > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once
> started
> > (default="
> > __MODULE_STRING(WATC
> HDOG_NOWAYOUT)
> > ")");
> >
> > +
> > +/* AST SCU Register for System Reset Event Log Register Set
> > + * ast2600 is scu074 ast2400/2500 is scu03c */ #define
> > +AST2400_SCU_SYS_RESET_STATUS 0x3c #define
> > +AST2400_SCU_SYS_RESET_WDT_MASK 0x1 #define
> > +AST2400_SCU_SYS_RESET_WDT_MASK_SHIFT 1
> > +
> > +#define AST2500_SCU_SYS_RESET_WDT_MASK 0x1
> #define
> > +AST2500_SCU_SYS_RESET_WDT_MASK_SHIFT 2
> > +
> > +#define AST2600_SCU_SYS_RESET_STATUS 0x74 #define
> > +AST2600_SCU_SYS_RESET_WDT_MASK 0xf #define
> > +AST2600_SCU_SYS_RESET_WDT_SW_MASK 0x8 #define
> > +AST2600_SCU_SYS_RESET_WDT_MASK_SHIFT 16
>
> I think for the most part these macros are only used to assign into the config
> data provided through the match struct? I kept having to scroll back-and-forth
> to track the values. I feel it might be better in this instance to _not_ define
> macros for them and assign the literals straight into the struct members down
> below.
>
Okay, theses definitions will be removed in the next patch version.
> > +
> > +#define WDT_REG_OFFSET_MASK 0x00000fff
> > +
> > +struct aspeed_wdt_scu {
> > + const char *compatible;
> > + u32 reset_status_reg;
> > + u32 wdt_reset_mask;
> > + u32 wdt_sw_reset_mask;
> > + u32 wdt_reset_mask_shift;
> > +};
> > +
> > struct aspeed_wdt_config {
> > u32 ext_pulse_width_mask;
> > u32 irq_shift;
> > u32 irq_mask;
> > + u32 reg_size;
>
> This is already specified in the devicetree, though admittedly aspeed- g4.dtsi
> sets the size to 0x1c, which is a little unhelpful for your calculations. I guess we
> can leave it for now.
>
Thanks for the reminder. It is also be updated the next patch.
> > + struct aspeed_wdt_scu scu;
> > };
> >
> > struct aspeed_wdt {
> > struct watchdog_device wdd;
> > void __iomem *base;
> > + int idx;
> > u32 ctrl;
> > const struct aspeed_wdt_config *cfg;
> > };
> > @@ -39,18 +70,42 @@ static const struct aspeed_wdt_config
> > ast2400_config = {
> > .ext_pulse_width_mask = 0xff,
> > .irq_shift = 0,
> > .irq_mask = 0,
> > + .reg_size = 0x20,
> > + .scu = {
> > + .compatible = "aspeed,ast2400-scu",
> > + .reset_status_reg =
> AST2400_SCU_SYS_RESET_STATUS,
> > + .wdt_reset_mask =
> AST2400_SCU_SYS_RESET_WDT_MASK,
> > + .wdt_sw_reset_mask = 0,
> > + .wdt_reset_mask_shift =
> > AST2400_SCU_SYS_RESET_WDT_MASK_SHIFT,
> > + },
> > };
> >
> > static const struct aspeed_wdt_config ast2500_config = {
> > .ext_pulse_width_mask = 0xfffff,
> > .irq_shift = 12,
> > .irq_mask = GENMASK(31, 12),
> > + .reg_size = 0x20,
> > + .scu = {
> > + .compatible = "aspeed,ast2500-scu",
> > + .reset_status_reg =
> AST2400_SCU_SYS_RESET_STATUS,
> > + .wdt_reset_mask =
> AST2500_SCU_SYS_RESET_WDT_MASK,
> > + .wdt_sw_reset_mask = 0,
> > + .wdt_reset_mask_shift =
> > AST2500_SCU_SYS_RESET_WDT_MASK_SHIFT,
> > + },
> > };
> >
> > static const struct aspeed_wdt_config ast2600_config = {
> > .ext_pulse_width_mask = 0xfffff,
> > .irq_shift = 0,
> > .irq_mask = GENMASK(31, 10),
> > + .reg_size = 0x40,
> > + .scu = {
> > + .compatible = "aspeed,ast2600-scu",
> > + .reset_status_reg =
> AST2600_SCU_SYS_RESET_STATUS,
> > + .wdt_reset_mask =
> AST2600_SCU_SYS_RESET_WDT_MASK,
> > + .wdt_sw_reset_mask =
> > AST2600_SCU_SYS_RESET_WDT_SW_MASK,
> > + .wdt_reset_mask_shift =
> > AST2600_SCU_SYS_RESET_WDT_MASK_SHIFT,
> > + },
> > };
> >
> > static const struct of_device_id aspeed_wdt_of_table[] = { @@ -213,6
> > +268,51 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd,
> > return 0;
> > }
> >
> > +static int aspeed_wdt_get_bootstatus(struct device *dev,
> > + struct aspeed_wdt
> *wdt)
>
> We're not really providing the boot status as a direct result of the function (i.e.
> in the return value or through an out-value pointer). I I feel this might be
> better named `aspeed_wdt_update_bootstatus()`.
>
Okay.
> > +{
> > + struct device_node *np = dev->of_node;
> > + struct aspeed_wdt_scu scu = wdt->cfg->scu;
>
> Is there a reason to take a copy? Can we make this a const pointer?
>
> > + struct regmap *scu_base;
> > + u32 reset_mask_width;
> > + u32 reset_mask_shift;
> > + u32 status;
> > + int ret;
> > +
> > + wdt->idx = ((u32)wdt->base & WDT_REG_OFFSET_MASK) /
>
> I think `(intptr_t)wdt->base` better conveys the intent here.
>
Okay.
> > + wdt->cfg->reg_size;
> > +
> > + /* On ast2400, only a bit is used to represent WDT reset */
> > + if (of_device_is_compatible(np, "aspeed,ast2400-wdt"))
> > + wdt->idx = 0;
> > +
> > + scu_base =
> > syscon_regmap_lookup_by_compatible(scu.compatible);
> > + if (IS_ERR(scu_base))
> > + return PTR_ERR(scu_base);
> > +
> > + ret = regmap_read(scu_base, scu.reset_status_reg, &status);
> > + if (ret)
> > + return ret;
> > +
> > + reset_mask_width = hweight32(scu.wdt_reset_mask);
> > + reset_mask_shift = scu.wdt_reset_mask_shift +
> > + reset_mask_width * wdt->idx;
> > +
> > + if (status & (scu.wdt_sw_reset_mask << reset_mask_shift))
> > + wdt->wdd.bootstatus = WDIOF_EXTERN1;
> > + else if (status & (scu.wdt_reset_mask << reset_mask_shift))
> > + wdt->wdd.bootstatus = WDIOF_CARDRESET;
> > + else
> > + wdt->wdd.bootstatus = 0;
>
> ...
>
> > +
> > + ret = regmap_write(scu_base, scu.reset_status_reg,
> > + scu.wdt_reset_mask <<
> reset_mask_shift);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> return regmap_write(...) ?
>
Okay.
> Andrew
Chin-Ting
Powered by blists - more mailing lists