* [PATCH] staging: dgnc: replace udelay with usleep_range @ 2017-02-26 18:38 Aishwarya Pant 2017-02-26 19:02 ` [Outreachy kernel] " Julia Lawall 0 siblings, 1 reply; 4+ messages in thread From: Aishwarya Pant @ 2017-02-26 18:38 UTC (permalink / raw) To: Lidza Louina, Mark Hounschell, Greg Kroah-Hartman; +Cc: outreachy-kernel Adding first patch for checkpatch warning on dgnc_cls.c - CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: dgnc: replace udelay with usleep_range 2017-02-26 18:38 [PATCH] staging: dgnc: replace udelay with usleep_range Aishwarya Pant @ 2017-02-26 19:02 ` Julia Lawall 2017-02-27 11:46 ` Aishwarya Pant 0 siblings, 1 reply; 4+ messages in thread From: Julia Lawall @ 2017-02-26 19:02 UTC (permalink / raw) To: Aishwarya Pant Cc: Lidza Louina, Mark Hounschell, Greg Kroah-Hartman, outreachy-kernel 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 > > 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. > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: dgnc: replace udelay with usleep_range 2017-02-26 19:02 ` [Outreachy kernel] " Julia Lawall @ 2017-02-27 11:46 ` Aishwarya Pant 2017-02-27 13:55 ` Julia Lawall 0 siblings, 1 reply; 4+ messages in thread From: Aishwarya Pant @ 2017-02-27 11:46 UTC (permalink / raw) To: Julia Lawall Cc: Lidza Louina, Mark Hounschell, Greg Kroah-Hartman, outreachy-kernel 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. > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: dgnc: replace udelay with usleep_range 2017-02-27 11:46 ` Aishwarya Pant @ 2017-02-27 13:55 ` Julia Lawall 0 siblings, 0 replies; 4+ messages in thread From: Julia Lawall @ 2017-02-27 13:55 UTC (permalink / raw) To: Aishwarya Pant Cc: Lidza Louina, Mark Hounschell, Greg Kroah-Hartman, outreachy-kernel On Mon, 27 Feb 2017, Aishwarya Pant wrote: > 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? Thanks for the very nice explanation. You could add to your commit message that you chose the upper limit based on the suggestions in XXX. I don't know the answer to the other questions, but maybe someone else can help out. julia > > -- > 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. > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-02-27 13:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-26 18:38 [PATCH] staging: dgnc: replace udelay with usleep_range Aishwarya Pant 2017-02-26 19:02 ` [Outreachy kernel] " Julia Lawall 2017-02-27 11:46 ` Aishwarya Pant 2017-02-27 13:55 ` Julia Lawall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.