linux-kernel - Re: [PATCH V1 1/1] watchdog: f71808e

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, 25 Mar 2019 14:10:15 +0800
From:   "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>, wim@...ux-watchdog.orgw
Cc:     linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V1 1/1] watchdog: f71808e_wdt: fix F81866 bit operation

Guenter Roeck 於 2019/3/22 下午 09:06 寫道:
> On 3/21/19 8:36 PM, Ji-Ze Hong (Peter Hong) wrote:
>> Fix error bit operation in watchdog_start()
>>
> 
> Hmm ... does that mean it never worked ? Did you test it this time ?

Sorry for lacking test procedure. I had only test the functional (reset)
, not to test the register value.

The F81866 PIN70 (WDTRST#/GPIO15) is default set to WDTRST# function and
the old code only change register 27h bit(4) - PORT_4E_EN.

If the mainboard entry port is 4Eh, the old code is equal to nothing
done, but when the mainboard entry port is 2Eh, this code will make the
change from entry port 2Eh to 4Eh.

https://html.alldatasheet.com/html-pdf/459086/FINTEK/F81866AD/26531/119/F81866AD.html

> A secondary concern is that the watchdog doesn't _have_ to trigger WDTRST,
> but may also trigger PWOK. But that may be a separate issue.

Out watchdog is only support WDTRST#.

> Please add:
> 
> Fixes: 14b24a88a3660 ("watchdog: f71808e_wdt: Add F81866 support")

OK, I'll add to v2

>> diff --git a/drivers/watchdog/f71808e_wdt.c 
>> b/drivers/watchdog/f71808e_wdt.c
>> index 9a1c761258ce..9129485732c7 100644
>> --- a/drivers/watchdog/f71808e_wdt.c
>> +++ b/drivers/watchdog/f71808e_wdt.c
>> @@ -387,18 +387,17 @@ static int watchdog_start(void)
>>       case f81866:
>>           /* Set pin 70 to WDTRST# */
>> -        superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
>> -                  BIT(3) | BIT(0));
>> -        superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL,
>> -                BIT(2));
>> +        superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 3);
>> +        superio_clear_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 0);
>> +        superio_set_bit(watchdog.sioaddr, SIO_F81866_REG_PORT_SEL, 2);
> 
> Better use superio_inb()/superio_outb(). The above is (much) more 
> expensive,
> and  we have no real idea what the impact of changing one bit at a time 
> may be.

Could I add a superio_mask_write(reg, mask, data) with v2 patch like
following fintek driver ?
https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/8250/8250_fintek.c#L113

Thanks
-- 
With Best Regards,
Peter Hong