Re: [Outreachy kernel] [PATCH] staging: dgnc: replace udelay with usleep_range

[Aishwarya Pant <aishpant@gmail.com>]

On Sun, Feb 26, 2017 at 08:02:56PM +0100, Julia Lawall wrote:
> 
> 
> On Mon, 27 Feb 2017, Aishwarya Pant wrote:
> 
> > Adding first patch for checkpatch warning on dgnc_cls.c -
> 
> What do you mean by "Adding first patch"?
> 
> Actually, commit messages should be written in the imperative (as you have
> for the subject line), and should describe what you do and why.
> 
> > CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt
> 
> Also commit messages should be limited to around 70 characters or so per
> line.
> 
> How did you choose 20 as the upper bound?
> 
> julia
>

From what I've read of `udelay` and `usleep_range` from the
documentation, this is what I understand:

udelay(t) initiates a busy-wait loop. This means it's taking up CPU
execution cycles to do work. Useful for atomic transactions as we'd like
to prevent other processes from doing work. Because it depends on CPU
frequency, the delay is sometimes more than t microseconds.

To sleep for precisely t microseconds without depending on CPU
frequency, we utilise hrtimers (high resolution timers), which are
implemented as callbacks. No CPU cycles are spent, a notification
(interrupt) is received when delay period is over. 
It is possible to use usleep_range(t, t) for precision, but if we use
this pattern throughout too many interrupts will be triggered - a big
overhead. If the range is sufficiently large, one optimisation the
kernel can make is to coalesce wakeups for efficiency. We should use
usleep_range as usleep_range(t, t + delta). A safe choice for delta is t
, so we have - usleep_range(t, 2*t)
(Another recommendation is that t lie between 10 and 20 microseconds, so
the choice of delta seems safe).

In the end, usleep_range is precise within a defined window of
(LOWER_LIMIT, UPPER_LIMIT)

I have a couple of questions about usleep_range  -

-- What are the other wake-ups that we can coalesce (any example)? 

-- "The larger a range you supply, the greater a chance that you will
not trigger an interrupt." Our goal is to trigger an interrupt, so that
we can end delay. How is end delay handled then?

-- 
Cheers 
Aishwarya Pant

> >
> > Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> > ---
> >  drivers/staging/dgnc/dgnc_cls.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
> > index c20ffdd..2cf146e 100644
> > --- a/drivers/staging/dgnc/dgnc_cls.c
> > +++ b/drivers/staging/dgnc/dgnc_cls.c
> > @@ -16,7 +16,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/sched.h>	/* For jiffies, task states */
> >  #include <linux/interrupt.h>	/* For tasklet and interrupt structs/defines */
> > -#include <linux/delay.h>	/* For udelay */
> > +#include <linux/delay.h>	/* For usleep_range */
> >  #include <linux/io.h>		/* For read[bwl]/write[bwl] */
> >  #include <linux/serial.h>	/* For struct async_serial */
> >  #include <linux/serial_reg.h>	/* For the various UART offsets */
> > @@ -409,7 +409,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, 20);
> >  }
> >
> >  static void cls_copy_data_from_queue_to_uart(struct channel_t *ch)
> > @@ -631,7 +631,7 @@ static void cls_flush_uart_read(struct channel_t *ch)
> >  	 * Presumably, this is a bug in this UART.
> >  	 */
> >
> > -	udelay(10);
> > +	usleep_range(10, 20);
> >  }
> >
> >  /*
> > @@ -1096,7 +1096,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, 20);
> >
> >  	ch->ch_flags |= (CH_FIFO_ENABLED | CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM);
> >
> > --
> > 2.7.4
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170226183806.GA3498%40aishwarya.
> > For more options, visit https://groups.google.com/d/optout.
> >