Re: [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs

[Florian Fainelli <f.fainelli@gmail.com>]

On 8/7/2020 4:23 PM, Guenter Roeck wrote:
> Hi Florian,
> 
> On 8/7/20 1:09 PM, Florian Fainelli wrote:
>>
>> On 8/7/2020 12:08 PM, Guenter Roeck wrote:
>>> On 8/7/20 11:08 AM, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 8/7/2020 9:21 AM, Guenter Roeck wrote:
>>>>> On Fri, Aug 07, 2020 at 04:59:02PM +0530, madhuparnabhowmik10@gmail.com wrote:
>>>>>> From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>>>>>>
>>>>>> In rdc321x_wdt_probe(), rdc321x_wdt_device.queue is initialized
>>>>>> after misc_register(), hence if ioctl is called before its
>>>>>> initialization which can call rdc321x_wdt_start() function,
>>>>>> it will see an uninitialized value of rdc321x_wdt_device.queue,
>>>>>> hence initialize it before misc_register().
>>>>>> Also, rdc321x_wdt_device.default_ticks is accessed in reset()
>>>>>> function called from write callback, thus initialize it before
>>>>>> misc_register().
>>>>>>
>>>>>> Found by Linux Driver Verification project (linuxtesting.org).
>>>>>>
>>>>>> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>>>>>
>>>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>>>>
>>>>> Having said that ... this is yet another potentially obsolete driver.
>>>>> You are really wasting your (and, fwiw, my) time.
>>>>>
>>>>> Florian, any thoughts if support for this chip can/should be deprecated
>>>>> or even removed ?
>>>>
>>>> I am still using my rdc321x-based SoC, so no, this is not obsolete as
>>>> far as I am concerned, time permitting, modernizing the driver is on my
>>>> TODO after checking/fixing the Ethernet driver first.
>>>>
>>>
>>> Do you have a manual ? I'd give it a try if you can test it - conversion
>>> should be simple enough (I have a coccinelle script which partially
>>> automates it), but this chip seems to have a fast timeout, and the
>>> comments in the code ("set the timeout to 81.92 us") seem to be quite
>>> obviously wrong.
>>
>> Yes, there is a public manual for that SoC, search for RDC R8610 and the
>> first link you find should be a 276 page long manual for the SoC.
>>
> 
> I found two, one for R8610 and one for R8610-G.

The R8610-G datasheet is the one that I have had and used thus far.

> Unfortunately, none of those
> describes the use of bit(31) in the watchdog register, nor the meaning
> of bit(12) and bit(13). Bit(31) is described in the code as "Mask",
> and it is set by a couple of commands. I _suspect_ that bit(31) has to be
> set to change some of the register bits, for example the counter value.
> That is just a wild guess, but it would explain why the driver works
> in the first place.
> 
> It is also not clear if the bits in the counter register are accumulative
> or if only the highest bit counts. The datasheets suggest that only the
> highest bit counts, but then the value of RDC_CLS_TMR doesn't make much
> sense since it sets two bits.
> 
> Since you wrote the driver, I was hoping that you might have a datasheet
> which explains all this in more detail.

I do not, and this was over 12 years ago, and I honestly do not recall
all the details, when I get the board running a newish kernel, I will
poke around.
-- 
Florian