linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
@ 2020-08-07 11:29 madhuparnabhowmik10
  2020-08-07 16:21 ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: madhuparnabhowmik10 @ 2020-08-07 11:29 UTC (permalink / raw)
  To: wim, linux
  Cc: linux-watchdog, linux-kernel, andrianov, ldv-project, Madhuparna Bhowmik

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>
---
 drivers/watchdog/rdc321x_wdt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/rdc321x_wdt.c b/drivers/watchdog/rdc321x_wdt.c
index 57187efeb86f..f0c94ea51c3e 100644
--- a/drivers/watchdog/rdc321x_wdt.c
+++ b/drivers/watchdog/rdc321x_wdt.c
@@ -231,6 +231,8 @@ static int rdc321x_wdt_probe(struct platform_device *pdev)
 
 	rdc321x_wdt_device.sb_pdev = pdata->sb_pdev;
 	rdc321x_wdt_device.base_reg = r->start;
+	rdc321x_wdt_device.queue = 0;
+	rdc321x_wdt_device.default_ticks = ticks;
 
 	err = misc_register(&rdc321x_wdt_misc);
 	if (err < 0) {
@@ -245,14 +247,11 @@ static int rdc321x_wdt_probe(struct platform_device *pdev)
 				rdc321x_wdt_device.base_reg, RDC_WDT_RST);
 
 	init_completion(&rdc321x_wdt_device.stop);
-	rdc321x_wdt_device.queue = 0;
 
 	clear_bit(0, &rdc321x_wdt_device.inuse);
 
 	timer_setup(&rdc321x_wdt_device.timer, rdc321x_wdt_trigger, 0);
 
-	rdc321x_wdt_device.default_ticks = ticks;
-
 	dev_info(&pdev->dev, "watchdog init success\n");
 
 	return 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-07 11:29 [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs madhuparnabhowmik10
@ 2020-08-07 16:21 ` Guenter Roeck
  2020-08-07 18:08   ` Florian Fainelli
  2020-08-07 18:30   ` [ldv-project] " Evgeny Novikov
  0 siblings, 2 replies; 11+ messages in thread
From: Guenter Roeck @ 2020-08-07 16:21 UTC (permalink / raw)
  To: madhuparnabhowmik10
  Cc: wim, linux-watchdog, linux-kernel, andrianov, ldv-project, f.fainelli

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 ?

Guenter

> ---
>  drivers/watchdog/rdc321x_wdt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/rdc321x_wdt.c b/drivers/watchdog/rdc321x_wdt.c
> index 57187efeb86f..f0c94ea51c3e 100644
> --- a/drivers/watchdog/rdc321x_wdt.c
> +++ b/drivers/watchdog/rdc321x_wdt.c
> @@ -231,6 +231,8 @@ static int rdc321x_wdt_probe(struct platform_device *pdev)
>  
>  	rdc321x_wdt_device.sb_pdev = pdata->sb_pdev;
>  	rdc321x_wdt_device.base_reg = r->start;
> +	rdc321x_wdt_device.queue = 0;
> +	rdc321x_wdt_device.default_ticks = ticks;
>  
>  	err = misc_register(&rdc321x_wdt_misc);
>  	if (err < 0) {
> @@ -245,14 +247,11 @@ static int rdc321x_wdt_probe(struct platform_device *pdev)
>  				rdc321x_wdt_device.base_reg, RDC_WDT_RST);
>  
>  	init_completion(&rdc321x_wdt_device.stop);
> -	rdc321x_wdt_device.queue = 0;
>  
>  	clear_bit(0, &rdc321x_wdt_device.inuse);
>  
>  	timer_setup(&rdc321x_wdt_device.timer, rdc321x_wdt_trigger, 0);
>  
> -	rdc321x_wdt_device.default_ticks = ticks;
> -
>  	dev_info(&pdev->dev, "watchdog init success\n");
>  
>  	return 0;
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-07 16:21 ` Guenter Roeck
@ 2020-08-07 18:08   ` Florian Fainelli
  2020-08-07 18:19     ` Guenter Roeck
  2020-08-07 19:08     ` Guenter Roeck
  2020-08-07 18:30   ` [ldv-project] " Evgeny Novikov
  1 sibling, 2 replies; 11+ messages in thread
From: Florian Fainelli @ 2020-08-07 18:08 UTC (permalink / raw)
  To: Guenter Roeck, madhuparnabhowmik10
  Cc: wim, linux-watchdog, linux-kernel, andrianov, ldv-project



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.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-07 18:08   ` Florian Fainelli
@ 2020-08-07 18:19     ` Guenter Roeck
  2020-08-07 19:08     ` Guenter Roeck
  1 sibling, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2020-08-07 18:19 UTC (permalink / raw)
  To: Florian Fainelli, madhuparnabhowmik10
  Cc: wim, linux-watchdog, linux-kernel, andrianov, ldv-project

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.
> 

Ok, fair enough.

> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [ldv-project] [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-07 16:21 ` Guenter Roeck
  2020-08-07 18:08   ` Florian Fainelli
@ 2020-08-07 18:30   ` Evgeny Novikov
  2020-08-07 19:00     ` Guenter Roeck
  1 sibling, 1 reply; 11+ messages in thread
From: Evgeny Novikov @ 2020-08-07 18:30 UTC (permalink / raw)
  To: Guenter Roeck, madhuparnabhowmik10
  Cc: ldv-project, f.fainelli, linux-watchdog, linux-kernel, wim

07.08.2020, 19:21, "Guenter Roeck" <linux@roeck-us.net>:
> 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.

Static analysis tools are not aware about obsolete drivers.
It would be great if there will be some formal way to filter them out.
Maybe some file will enumerate all obsolete drivers, or there will be
something within their source code, or something else.

-- 
Best regards,
Evgeny Novikov

>
> Florian, any thoughts if support for this chip can/should be deprecated
> or even removed ?
>
> Guenter
>
>>  ---
>>   drivers/watchdog/rdc321x_wdt.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>>  diff --git a/drivers/watchdog/rdc321x_wdt.c b/drivers/watchdog/rdc321x_wdt.c
>>  index 57187efeb86f..f0c94ea51c3e 100644
>>  --- a/drivers/watchdog/rdc321x_wdt.c
>>  +++ b/drivers/watchdog/rdc321x_wdt.c
>>  @@ -231,6 +231,8 @@ static int rdc321x_wdt_probe(struct platform_device *pdev)
>>
>>           rdc321x_wdt_device.sb_pdev = pdata->sb_pdev;
>>           rdc321x_wdt_device.base_reg = r->start;
>>  + rdc321x_wdt_device.queue = 0;
>>  + rdc321x_wdt_device.default_ticks = ticks;
>>
>>           err = misc_register(&rdc321x_wdt_misc);
>>           if (err < 0) {
>>  @@ -245,14 +247,11 @@ static int rdc321x_wdt_probe(struct platform_device *pdev)
>>                                   rdc321x_wdt_device.base_reg, RDC_WDT_RST);
>>
>>           init_completion(&rdc321x_wdt_device.stop);
>>  - rdc321x_wdt_device.queue = 0;
>>
>>           clear_bit(0, &rdc321x_wdt_device.inuse);
>>
>>           timer_setup(&rdc321x_wdt_device.timer, rdc321x_wdt_trigger, 0);
>>
>>  - rdc321x_wdt_device.default_ticks = ticks;
>>  -
>>           dev_info(&pdev->dev, "watchdog init success\n");
>>
>>           return 0;
>>  --
>>  2.17.1
>
> _______________________________________________
> ldv-project mailing list
> ldv-project@linuxtesting.org
> http://linuxtesting.org/cgi-bin/mailman/listinfo/ldv-project

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [ldv-project] [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-07 18:30   ` [ldv-project] " Evgeny Novikov
@ 2020-08-07 19:00     ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2020-08-07 19:00 UTC (permalink / raw)
  To: Evgeny Novikov, madhuparnabhowmik10
  Cc: ldv-project, f.fainelli, linux-watchdog, linux-kernel, wim

On 8/7/20 11:30 AM, Evgeny Novikov wrote:
> 07.08.2020, 19:21, "Guenter Roeck" <linux@roeck-us.net>:
>> 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.
> 
> Static analysis tools are not aware about obsolete drivers.
> It would be great if there will be some formal way to filter them out.
> Maybe some file will enumerate all obsolete drivers, or there will be
> something within their source code, or something else.
> 

In general, all watchdog drivers not implementing the watchdog API
are effectively obsolete and should not be touched. If they are still
in use (meaning someone has a means to test them), they should be
converted to use the watchdog API instead.

Guenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-07 18:08   ` Florian Fainelli
  2020-08-07 18:19     ` Guenter Roeck
@ 2020-08-07 19:08     ` Guenter Roeck
  2020-08-07 20:09       ` Florian Fainelli
  1 sibling, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2020-08-07 19:08 UTC (permalink / raw)
  To: Florian Fainelli, madhuparnabhowmik10
  Cc: wim, linux-watchdog, linux-kernel, andrianov, ldv-project

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.

Guenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-07 19:08     ` Guenter Roeck
@ 2020-08-07 20:09       ` Florian Fainelli
  2020-08-07 23:23         ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Florian Fainelli @ 2020-08-07 20:09 UTC (permalink / raw)
  To: Guenter Roeck, madhuparnabhowmik10
  Cc: wim, linux-watchdog, linux-kernel, andrianov, ldv-project



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 probably won't be able to test anything until the middle of next week
though.
--
Florian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-07 20:09       ` Florian Fainelli
@ 2020-08-07 23:23         ` Guenter Roeck
  2020-08-08  0:41           ` Florian Fainelli
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2020-08-07 23:23 UTC (permalink / raw)
  To: Florian Fainelli, madhuparnabhowmik10
  Cc: wim, linux-watchdog, linux-kernel, andrianov, ldv-project

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. 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-07 23:23         ` Guenter Roeck
@ 2020-08-08  0:41           ` Florian Fainelli
  2020-08-08  4:58             ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Florian Fainelli @ 2020-08-08  0:41 UTC (permalink / raw)
  To: Guenter Roeck, madhuparnabhowmik10
  Cc: wim, linux-watchdog, linux-kernel, andrianov, ldv-project



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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs
  2020-08-08  0:41           ` Florian Fainelli
@ 2020-08-08  4:58             ` Guenter Roeck
  0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2020-08-08  4:58 UTC (permalink / raw)
  To: Florian Fainelli, madhuparnabhowmik10
  Cc: wim, linux-watchdog, linux-kernel, andrianov, ldv-project

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

Mine is draft version 0.2. Do you have a newer version, by any chance ?

>> 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.
> 
Surprise :-)

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-08-08  4:59 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-07 11:29 [PATCH] drivers: watchdog: rdc321x_wdt: Fix race condition bugs madhuparnabhowmik10
2020-08-07 16:21 ` Guenter Roeck
2020-08-07 18:08   ` Florian Fainelli
2020-08-07 18:19     ` Guenter Roeck
2020-08-07 19:08     ` Guenter Roeck
2020-08-07 20:09       ` Florian Fainelli
2020-08-07 23:23         ` Guenter Roeck
2020-08-08  0:41           ` Florian Fainelli
2020-08-08  4:58             ` Guenter Roeck
2020-08-07 18:30   ` [ldv-project] " Evgeny Novikov
2020-08-07 19:00     ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).