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

[Guenter Roeck]

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@xxxxxxxxx wrote:
>>>>> From: Madhuparna Bhowmik <madhuparnabhowmik10@xxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxx>
>>>>
>>>> Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
>>>>
>>>> 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. 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.

Thanks,
Guenter