staging: dgnc: dgnc_cls.c: usleep_range is preferred over udelay

PATCH Superseded Anchal Jain 3 participants 7 messages Sep 13, 2016 Staging

Share & tools

Anchal Jain

author

2016-09-13 07:52 UTC #0 lore raw

PATCH 1/1 staging: dgnc: dgnc_cls.c: usleep_range is preferred over udelay

According to Documentation/timers/timers-howto.txt" udelay() is only called once from a place where sleeping is allowed. We can replace it with a call to usleep_range()
with a reasonable upper limit.

Signed-off-by Anchal Jain <[email protected]>

---
 drivers/staging/dgnc/dgnc_cls.c      |  6 +++---
 drivers/staging/wilc1000/linux_mon.c | 11 +++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

drivers/staging/dgnc/dgnc_cls.c +3 -3

index 4e1e0dc6..1b07255 100644
@@ -410,7 +410,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)

@@ -632,7 +632,7 @@ static void cls_flush_uart_read(struct channel_t *ch)
 	 * Presumably, this is a bug in this UART.
 	 */
 
-	udelay(10);
+	usleep_delay(10, 20);
 }
 
 /*

@@ -1104,7 +1104,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_delay(10, 20);
 
 	ch->ch_flags |= (CH_FIFO_ENABLED | CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM);

drivers/staging/wilc1000/linux_mon.c +11 -0

index 242f82f..22f7684 100644
@@ -59,9 +59,15 @@ void WILC_WFI_monitor_rx(u8 *buff, u32 size)
 
 	/* Get WILC header */
 	memcpy(&header, (buff - HOST_HDR_OFFSET), HOST_HDR_OFFSET);
+<<<<<<< HEAD
 	/*
 	 * The packet offset field contain info about what type of management
 	 * the frame we are dealing with and ack status
+=======
+
+	/* The packet offset field conain info about what type of management
+	 * frame we are dealing with and ack status
+>>>>>>> staging: wilc1000: Fix lines over 80 characters
 	 */
 	pkt_offset = GET_PKT_OFFSET(header);

@@ -128,8 +134,13 @@ struct tx_complete_mon_data {
 static void mgmt_tx_complete(void *priv, int status)
 {
 	struct tx_complete_mon_data *pv_data = priv;
+<<<<<<< HEAD
 	/*
 	 * in case of fully hosting mode, the freeing will be done
+=======
+
+	/* in case of fully hosting mode, the freeing will be done
+>>>>>>> staging: wilc1000: Fix lines over 80 characters
 	 * in response to the cfg packet
 	 */
 	kfree(pv_data->buff);

Signature · 1 line

1.9.1

Julia Lawall

2016-09-13 07:58 UTC #1 lore raw

Quoted 47 lines · Anchal Jain

Signed-off-by Anchal Jain <[email protected]>

---
 drivers/staging/dgnc/dgnc_cls.c      |  6 +++---
 drivers/staging/wilc1000/linux_mon.c | 11 +++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

drivers/staging/dgnc/dgnc_cls.c +3 -3

index 4e1e0dc6..1b07255 100644
@@ -410,7 +410,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)

@@ -632,7 +632,7 @@ static void cls_flush_uart_read(struct channel_t *ch)
 	 * Presumably, this is a bug in this UART.
 	 */
 
-	udelay(10);
+	usleep_delay(10, 20);
 }
 
 /*

@@ -1104,7 +1104,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_delay(10, 20);
 
 	ch->ch_flags |= (CH_FIFO_ENABLED | CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM);

drivers/staging/wilc1000/linux_mon.c +0 -0

index 242f82f..22f7684 100644

drivers/staging/wilc1000/linux_mon.c void WILC_WFI_monitor_rx(u8 *buff, u32 size) · line 62

Quoted 4 lines

drivers/staging/wilc1000/linux_mon.c +1 -0

@@ -59,9 +59,15 @@ void WILC_WFI_monitor_rx(u8 *buff, u32 size)
 
 	/* Get WILC header */
 	memcpy(&header, (buff - HOST_HDR_OFFSET), HOST_HDR_OFFSET);
+<<<<<<< HEAD

This looks like git noise, and should not be in the patch.

julia

Quoted 35 lines

drivers/staging/wilc1000/linux_mon.c +10 -1
 	/*
 	 * The packet offset field contain info about what type of management
 	 * the frame we are dealing with and ack status
+=======
+
+	/* The packet offset field conain info about what type of management
+	 * frame we are dealing with and ack status
+>>>>>>> staging: wilc1000: Fix lines over 80 characters
 	 */
 	pkt_offset = GET_PKT_OFFSET(header);
 
@@ -128,8 +134,13 @@ struct tx_complete_mon_data {
 static void mgmt_tx_complete(void *priv, int status)
 {
 	struct tx_complete_mon_data *pv_data = priv;
+<<<<<<< HEAD
 	/*
 	 * in case of fully hosting mode, the freeing will be done
+=======
+
+	/* in case of fully hosting mode, the freeing will be done
+>>>>>>> staging: wilc1000: Fix lines over 80 characters
 	 * in response to the cfg packet
 	 */
 	kfree(pv_data->buff);
--
1.9.1
--
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 [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160913075211.GA6959%40life-desktop.
For more options, visit https://groups.google.com/d/optout.

Arnd Bergmann

2016-09-13 07:58 UTC #2 lore raw

In reply to Anchal Jain

Quoted 16 lines · CEST Anchal Jain

According to Documentation/timers/timers-howto.txt" udelay() is only called once from a place where sleeping is allowed. We can replace it with a call to usleep_range()
with a reasonable upper limit.
Signed-off-by Anchal Jain <[email protected]>
---
 drivers/staging/dgnc/dgnc_cls.c      |  6 +++---
 drivers/staging/wilc1000/linux_mon.c | 11 +++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)
drivers/staging/dgnc/dgnc_cls.c +0 -0
index 4e1e0dc6..1b07255 100644

drivers/staging/dgnc/dgnc_cls.c static void cls_assert_modem_signals(struct channel_t *ch) · line 415

Quoted 7 lines

drivers/staging/dgnc/dgnc_cls.c +1 -1

@@ -410,7 +410,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);
 }

This gets called from cls_copy_data_from_queue_to_uart() under a spinlock.

drivers/staging/wilc1000/linux_mon.c void WILC_WFI_monitor_rx(u8 *buff, u32 size) · line 66

Quoted 13 lines

drivers/staging/wilc1000/linux_mon.c +2 -0

index 242f82f..22f7684 100644
@@ -59,9 +59,15 @@ void WILC_WFI_monitor_rx(u8 *buff, u32 size)
 
 	/* Get WILC header */
 	memcpy(&header, (buff - HOST_HDR_OFFSET), HOST_HDR_OFFSET);
+<<<<<<< HEAD
 	/*
 	 * The packet offset field contain info about what type of management
 	 * the frame we are dealing with and ack status
+=======

Clearly doesn't belong into the patch.

Arnd

Anchal Jain

author

2016-09-13 08:24 UTC #3 lore raw

Quoted 1 line

This gets called from cls_copy_data_from_queue_to_uart() under a spinlock.

According to the documentation "Documentation/timers/timers-howto.txt". In this documentation no explanation of "spinlock". What is that meaning?

Quoted 49 lines · Arnd Bergmann

Quoted 5 lines · CEST Anchal Jain
According to Documentation/timers/timers-howto.txt" udelay() is only called once from a place where sleeping is allowed. We can replace it with a call to usleep_range()
with a reasonable upper limit.
Review
Quoted 7 lines
Signed-off-by Anchal Jain <[email protected]>
---
 drivers/staging/dgnc/dgnc_cls.c      |  6 +++---
 drivers/staging/wilc1000/linux_mon.c | 11 +++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/dgnc/dgnc_cls.c
drivers/staging/dgnc/dgnc_cls.c static void cls_assert_modem_signals(struct
Quoted 4 lines
index 4e1e0dc6..1b07255 100644
drivers/staging/dgnc/dgnc_cls.c +0 -0
@@ -410,7 +410,7 @@ static void cls_assert_modem_signals(struct
channel_t *ch)
drivers/staging/dgnc/dgnc_cls.c static void cls_assert_modem_signals(struct channel_t *ch) · line 415
Quoted 7 lines
drivers/staging/dgnc/dgnc_cls.c +1 -1
      writeb(out, &ch->ch_cls_uart->mcr);
 
      /* Give time for the UART to actually drop the signals */
-     udelay(10);
+     usleep_range(10, 20);
 }
 
This gets called from cls_copy_data_from_queue_to_uart() under a spinlock.
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/staging/wilc1000/linux_mon.c
drivers/staging/wilc1000/linux_mon.c void WILC_WFI_monitor_rx(u8 *buff, u32 size) · line 64
Quoted 10 lines
index 242f82f..22f7684 100644
drivers/staging/wilc1000/linux_mon.c +1 -0
@@ -59,9 +59,15 @@ void WILC_WFI_monitor_rx(u8 *buff, u32 size)
 
      /* Get WILC header */
      memcpy(&header, (buff - HOST_HDR_OFFSET), HOST_HDR_OFFSET);
+<<<<<<< HEAD
      /*
       * The packet offset field contain info about what type of
management
drivers/staging/wilc1000/linux_mon.c void WILC_WFI_monitor_rx(u8 *buff, u32 size) · line 66
Quoted 2 lines
drivers/staging/wilc1000/linux_mon.c +1 -0
       * the frame we are dealing with and ack status
+=======
Clearly doesn't belong into the patch.
Arnd

Julia Lawall

2016-09-13 09:47 UTC #4 lore raw

Quoted 4 lines · Anchal Jain

Quoted 1 line
This gets called from cls_copy_data_from_queue_to_uart() under a spinlock.
According to the documentation "Documentation/timers/timers-howto.txt". In this documentation no explanation of "spinlock". What is that meaning?

A spinlock is a certain kind of lock. It could be taken with a function like spin_lock or spin_lock_irqsave. Probably you can find more information with google.

julia

Quoted 69 lines · Arnd Bergmann

      On Tuesday, September 13, 2016 1:22:26 PM CEST Anchal Jain
      wrote:
      > According to Documentation/timers/timers-howto.txt"
      > udelay() is only called once from a place where sleeping is
      allowed.
      > We can replace it with a call to usleep_range()
      > with a reasonable upper limit.
      >
      > Signed-off-by: Anchal Jain <[email protected]>
      > ---
      >  drivers/staging/dgnc/dgnc_cls.c      |  6 +++---
      >  drivers/staging/wilc1000/linux_mon.c | 11 +++++++++++
      >  2 files changed, 14 insertions(+), 3 deletions(-)
      >
      > diff --git a/drivers/staging/dgnc/dgnc_cls.c
      b/drivers/staging/dgnc/dgnc_cls.c
      > index 4e1e0dc6..1b07255 100644
      > --- a/drivers/staging/dgnc/dgnc_cls.c
      > +++ b/drivers/staging/dgnc/dgnc_cls.c
      > @@ -410,7 +410,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);
      >  }
      >

      This gets called from cls_copy_data_from_queue_to_uart() under a
      spinlock.

      > diff --git a/drivers/staging/wilc1000/linux_mon.c
      b/drivers/staging/wilc1000/linux_mon.c
      > index 242f82f..22f7684 100644
      > --- a/drivers/staging/wilc1000/linux_mon.c
      > +++ b/drivers/staging/wilc1000/linux_mon.c
      > @@ -59,9 +59,15 @@ void WILC_WFI_monitor_rx(u8 *buff, u32
      size)
      >
      >       /* Get WILC header */
      >       memcpy(&header, (buff - HOST_HDR_OFFSET),
      HOST_HDR_OFFSET);
      > +<<<<<<< HEAD
      >       /*
      >        * The packet offset field contain info about what type
      of management
      >        * the frame we are dealing with and ack status
      > +=======

      Clearly doesn't belong into the patch.

              Arnd

--
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 [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/CAAbeOSv0%2BHCx28Ug1eqEn FsEjT3sMuP6aMUR%2BJ2G_gKFuYYcaw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Arnd Bergmann

2016-09-13 10:36 UTC #5 lore raw

In reply to Anchal Jain

Quoted 4 lines · CEST Anchal Jain

Quoted 1 line
This gets called from cls_copy_data_from_queue_to_uart() under a spinlock.
According to the documentation "Documentation/timers/timers-howto.txt". In this documentation no explanation of "spinlock". What is that meaning?

Holding a spinlock is one of the things that imply "atomic context", which is mentioned there. Other examples would be

- being called from an interrupt handler
- disabling interrupts
- disabling preemption

Arnd

Anchal Jain

author

2016-09-13 10:45 UTC #6 lore raw

Thank you, Now I understand. I will send the next version of the patch soon.

Quoted 16 lines · Arnd Bergmann

Quoted 1 line · CEST Anchal Jain
Quoted 1 line
This gets called from cls_copy_data_from_queue_to_uart() under a
spinlock.
Quoted 3 lines
According to the documentation "Documentation/timers/timers-howto.txt". In this documentation no explanation of "spinlock". What is that meaning?
Holding a spinlock is one of the things that imply "atomic context", which is mentioned there. Other examples would be
- being called from an interrupt handler
- disabling interrupts
- disabling preemption
Arnd