[PATCH] drivers:watchdog:aspeed_wdt: using msleep instead of mdelay

[PATCH] drivers:watchdog:aspeed_wdt: using msleep instead of mdelay

[Karim Eshapa]

that's useful for the scheduler, power management unless
the driver needs to delay in atomic context
look at documentation/timers/timers-howto

Signed-off-by: Karim Eshapa <karim.eshapa@gmail.com>
---
 drivers/watchdog/aspeed_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 1c65258..17f06d1 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -115,7 +115,7 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd,
 
 	aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000);
 
-	mdelay(1000);
+	msleep(1000);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH] drivers:watchdog:aspeed_wdt: using msleep instead of mdelay

[Guenter Roeck]

On 04/16/2017 09:33 AM, Karim Eshapa wrote:
> that's useful for the scheduler, power management unless
> the driver needs to delay in atomic context
> look at documentation/timers/timers-howto
>
> Signed-off-by: Karim Eshapa <karim.eshapa@gmail.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 1c65258..17f06d1 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -115,7 +115,7 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd,
>
>  	aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000);
>
> -	mdelay(1000);
> +	msleep(1000);
>
>  	return 0;
>  }
>
Possibly, but how can you guarantee that the restart function is called with interrupts
enabled ? Also, why would it be necessary or even useful for the scheduler to do anything
while the system is in the process of restarting ?

Guenter

Re:drivers:watchdog:aspeed_wdt: using msleep instead of mdelay

[Karim Eshapa]

On Sun, 16 Apr 2017 12:53:28 -0700,Guenter Roeck wrote:
> On 04/16/2017 09:33 AM, Karim Eshapa wrote:
>>
>> that's useful for the scheduler, power management unless
>> the driver needs to delay in atomic context
>> look at documentation/timers/timers-howto
>
> Possibly, but how can you guarantee that the restart function is called with
> interrupts enabled ? Also, why would it be necessary or even useful for the 
> scheduler to do anything while the system is in the process of restarting ?

>From signaling or interruption point of view msleep() is uninterruptible. 
your process will sleep and won't be waked up until finish the time.

>From the cpu load and power point of view, mdelay() makes your code
stucked doing nothing until the delay finishes so, it's still headache
to the schedular from time slot perspective.
Although it's restating but it's still a long process that takes time. 

In addittion to mdelay() isn't preferable in case of large delays +10 as it uses udelay()
 
But the question now what about ptotecting your HW while being accessed
through manipulating the registers. and what about memory reordering may be generated
by the compiler or the machine itself! while accessing a sequence of registers.

>> Signed-off-by: Karim Eshapa <karim.eshapa@gmail.com>
>> ---
>> drivers/watchdog/aspeed_wdt.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)

>>  diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
>> index 1c65258..17f06d1 100644
>> --- a/drivers/watchdog/aspeed_wdt.c
>>  +++ b/drivers/watchdog/aspeed_wdt.c
>> @@ -115,7 +115,7 @@ static int aspeed_wdt_restart(struct watchdog_device *wdd>> , aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000);
>> -       mdelay(1000);
>> +       msleep(1000);
>>
>>         return 0;
>>   }

Thanks,
Karim

Re: drivers:watchdog:aspeed_wdt: using msleep instead of mdelay

[Guenter Roeck]

On Mon, Apr 17, 2017 at 07:05:30PM +0200, Karim Eshapa wrote:
> On Sun, 16 Apr 2017 12:53:28 -0700,Guenter Roeck wrote:
> > On 04/16/2017 09:33 AM, Karim Eshapa wrote:
> >>
> >> that's useful for the scheduler, power management unless
> >> the driver needs to delay in atomic context
> >> look at documentation/timers/timers-howto
> >
> > Possibly, but how can you guarantee that the restart function is called with
> > interrupts enabled ? Also, why would it be necessary or even useful for the 
> > scheduler to do anything while the system is in the process of restarting ?
> 
> From signaling or interruption point of view msleep() is uninterruptible. 
> your process will sleep and won't be waked up until finish the time.
> 
> From the cpu load and power point of view, mdelay() makes your code
> stucked doing nothing until the delay finishes so, it's still headache
> to the schedular from time slot perspective.
> Although it's restating but it's still a long process that takes time. 
> 
> In addittion to mdelay() isn't preferable in case of large delays +10 as it uses udelay()
>  
> But the question now what about ptotecting your HW while being accessed
> through manipulating the registers. and what about memory reordering may be generated
> by the compiler or the machine itself! while accessing a sequence of registers.
> 
We are in the process of _resetting the system_ in this function.
If the function works, it won't return from the call to mdelay().
If anything, I would argue that we don't want to use anything but mdelay()
in this situation.

Sorry, I don't see the point you are trying to make.

Guenter