[PATCH] staging: dgnc: usleep_range is preferred over udelay

[PATCH] staging: dgnc: usleep_range is preferred over udelay

[Aybuke Ozdemir]

This patch fixes the issue:

CHECK: usleep_range is preferred over udelay; see
Documentation/timers/timers-howto.txt

Signed-off-by: Aybuke Ozdemir <aybuke.147@gmail.com>
---
 drivers/staging/dgnc/dgnc_cls.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index 72f0aaa..ce1471b 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -956,7 +956,7 @@ static void cls_flush_uart_read(struct channel_t *ch)
 	 * Presumably, this is a bug in this UART.
 	 */
 
-	udelay(10);
+	usleep_range(10, 100);
 }
 
 static void cls_copy_data_from_queue_to_uart(struct channel_t *ch)
@@ -1116,7 +1116,7 @@ static void cls_assert_modem_signals(struct channel_t *ch)
 	writeb(out, &ch->ch_cls_uart->mcr);
 
 	/* Give time for the UART to actually drop the signals */
-	udelay(10);
+	usleep_range(10, 100);
 }
 
 static void cls_send_start_character(struct channel_t *ch)
@@ -1170,7 +1170,7 @@ static void cls_uart_init(struct channel_t *ch)
 
 	writeb((UART_FCR_ENABLE_FIFO|UART_FCR_CLEAR_RCVR|UART_FCR_CLEAR_XMIT),
 	       &ch->ch_cls_uart->isr_fcr);
-	udelay(10);
+	usleep_range(10, 100);
 
 	ch->ch_flags |= (CH_FIFO_ENABLED | CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM);
 
-- 
2.1.4

Re: [Outreachy kernel] [PATCH] staging: dgnc: usleep_range is preferred over udelay

[Tejun Heo]

Hello,

On Thu, Feb 11, 2016 at 03:14:30PM +0200, Aybuke Ozdemir wrote:
> @@ -956,7 +956,7 @@ static void cls_flush_uart_read(struct channel_t *ch)
>  	 * Presumably, this is a bug in this UART.
>  	 */
>  
> -	udelay(10);
> +	usleep_range(10, 100);
>  }

So, one important difference between udelay() and usleep_range() is
that the former can be called from any context while the latter only
from a blockable context as it's asking to be scheduled away.  These
uart functions seem to be accessing hardware directly which usually
indicate that they need to be synchronized somehow.

grepping for flush_uart_ shows that these function are called under a
channel_t->ch_lock spinlock, so this patch will end up triggering a
sanity check in the scheduler and leave the kernel in a buggy state
when the usleep_range()'s are called.

Please don't forget to cc the original author or maintainer of the
code being modified.

Thanks.

-- 
tejun

Re: [Outreachy kernel] [PATCH] staging: dgnc: usleep_range is preferred over udelay

[Julia Lawall]

On Thu, 11 Feb 2016, Tejun Heo wrote:

> Hello,
>
> On Thu, Feb 11, 2016 at 03:14:30PM +0200, Aybuke Ozdemir wrote:
> > @@ -956,7 +956,7 @@ static void cls_flush_uart_read(struct channel_t *ch)
> >  	 * Presumably, this is a bug in this UART.
> >  	 */
> >
> > -	udelay(10);
> > +	usleep_range(10, 100);
> >  }
>
> So, one important difference between udelay() and usleep_range() is
> that the former can be called from any context while the latter only
> from a blockable context as it's asking to be scheduled away.  These
> uart functions seem to be accessing hardware directly which usually
> indicate that they need to be synchronized somehow.
>
> grepping for flush_uart_ shows that these function are called under a
> channel_t->ch_lock spinlock, so this patch will end up triggering a
> sanity check in the scheduler and leave the kernel in a buggy state
> when the usleep_range()'s are called.
>
> Please don't forget to cc the original author or maintainer of the
> code being modified.

Actually, no, they should only write to the list.

Thanks for the explanation otherwise.

julia

Re: [Outreachy kernel] [PATCH] staging: dgnc: usleep_range is preferred over udelay

[Tejun Heo]

Hello, Julia.

On Thu, Feb 11, 2016 at 04:53:49PM +0100, Julia Lawall wrote:
> > Please don't forget to cc the original author or maintainer of the
> > code being modified.
> 
> Actually, no, they should only write to the list.

OIC, gotta go back and read the introduction more carefully.  Sorry
about the confusion. :)

-- 
tejun

Re: [Outreachy kernel] [PATCH] staging: dgnc: usleep_range is preferred over udelay

[Greg KH]

On Thu, Feb 11, 2016 at 03:14:30PM +0200, Aybuke Ozdemir wrote:
> This patch fixes the issue:
> 
> CHECK: usleep_range is preferred over udelay; see
> Documentation/timers/timers-howto.txt
> 
> Signed-off-by: Aybuke Ozdemir <aybuke.147@gmail.com>
> ---
>  drivers/staging/dgnc/dgnc_cls.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
> index 72f0aaa..ce1471b 100644
> --- a/drivers/staging/dgnc/dgnc_cls.c
> +++ b/drivers/staging/dgnc/dgnc_cls.c
> @@ -956,7 +956,7 @@ static void cls_flush_uart_read(struct channel_t *ch)
>  	 * Presumably, this is a bug in this UART.
>  	 */
>  
> -	udelay(10);
> +	usleep_range(10, 100);

As Tejun pointed out, this isn't ok, you can just ignore that checkpatch
warning.  Maybe we don't want to sleep for 100, are you sure about that?
Without the hardware to test with, we don't want to touch stuff like
this, sorry.

greg k-h