linux-kernel - Re: [PATCH] watchdog: bcm7038

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:   Mon, 22 Feb 2021 22:48:09 +0100
From:   Álvaro Fernández Rojas <noltari@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] watchdog: bcm7038_wdt: add big endian support

Hi Guenter,

> El 22 feb 2021, a las 22:24, Guenter Roeck <linux@...ck-us.net> escribió:
> 
> On 2/22/21 12:03 PM, Álvaro Fernández Rojas wrote:
>> bcm7038_wdt can be used on bmips (bcm63xx) devices too.
>> 
> It might make sense to actually enable it for BCM63XX.

bcm63xx SoCs are supported in bcm63xx and bmips.
bcm63xx doesn’t have device tree support, but bmips does and this watchdog is already enabled for bmips.

> 
>> Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>
>> ---
>> drivers/watchdog/bcm7038_wdt.c | 30 ++++++++++++++++++++++++------
>> 1 file changed, 24 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
>> index 979caa18d3c8..62494da1ac57 100644
>> --- a/drivers/watchdog/bcm7038_wdt.c
>> +++ b/drivers/watchdog/bcm7038_wdt.c
>> @@ -34,6 +34,24 @@ struct bcm7038_watchdog {                                 
>> 
>> static bool nowayout = WATCHDOG_NOWAYOUT;
>> 
>> +static inline void bcm7038_wdt_write(unsigned long data, void __iomem *reg)
>> +{
>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>> +    __raw_writel(data, reg);
>> +#else
>> +    writel(data, reg);
>> +#endif
>> +}
>> +
>> +static inline unsigned long bcm7038_wdt_read(void __iomem *reg)
>> +{
>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>> +    return __raw_readl(reg);
>> +#else
>> +    return readl(reg);
>> +#endif
>> +}
>> +
> 
> This needs further explanation. Why not just use __raw_writel() and
> __raw_readl() unconditionally ? Also, is it known for sure that,
> say, bmips_be_defconfig otherwise uses the wrong endianness
> (vs. bmips_stb_defconfig which is a little endian configuration) ?

Because __raw_writel() doesn’t have memory barriers and writel() does.
Those configs use the correct endiannes, so I don’t know what you mean...

> 
> Thanks,
> Guenter
> 
>> static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
>> {
>>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>> @@ -41,15 +59,15 @@ static void bcm7038_wdt_set_timeout_reg(struct watchdog_device *wdog)
>> 
>>    timeout = wdt->rate * wdog->timeout;
>> 
>> -    writel(timeout, wdt->base + WDT_TIMEOUT_REG);
>> +    bcm7038_wdt_write(timeout, wdt->base + WDT_TIMEOUT_REG);
>> }
>> 
>> static int bcm7038_wdt_ping(struct watchdog_device *wdog)
>> {
>>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>> 
>> -    writel(WDT_START_1, wdt->base + WDT_CMD_REG);
>> -    writel(WDT_START_2, wdt->base + WDT_CMD_REG);
>> +    bcm7038_wdt_write(WDT_START_1, wdt->base + WDT_CMD_REG);
>> +    bcm7038_wdt_write(WDT_START_2, wdt->base + WDT_CMD_REG);
>> 
>>    return 0;
>> }
>> @@ -66,8 +84,8 @@ static int bcm7038_wdt_stop(struct watchdog_device *wdog)
>> {
>>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>> 
>> -    writel(WDT_STOP_1, wdt->base + WDT_CMD_REG);
>> -    writel(WDT_STOP_2, wdt->base + WDT_CMD_REG);
>> +    bcm7038_wdt_write(WDT_STOP_1, wdt->base + WDT_CMD_REG);
>> +    bcm7038_wdt_write(WDT_STOP_2, wdt->base + WDT_CMD_REG);
>> 
>>    return 0;
>> }
>> @@ -88,7 +106,7 @@ static unsigned int bcm7038_wdt_get_timeleft(struct watchdog_device *wdog)
>>    struct bcm7038_watchdog *wdt = watchdog_get_drvdata(wdog);
>>    u32 time_left;
>> 
>> -    time_left = readl(wdt->base + WDT_CMD_REG);
>> +    time_left = bcm7038_wdt_read(wdt->base + WDT_CMD_REG);
>> 
>>    return time_left / wdt->rate;
>> }
>> 
>