Re: [PATCH] staging: dgap: use schedule_timeout_interruptible() instead of dgap_ms_sleep()

[Joe Perches]

On Tue, 2014-09-16 at 09:31 +0300, Dan Carpenter wrote:
> There is no point in calling signal_pending() if you don't care about
> the return value.

So maybe make it __must_check?
---
 include/linux/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7d799ea..03273c0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2692,7 +2692,7 @@ static inline int restart_syscall(void)
return -ERESTARTNOINTR;
 }
 
-static inline int signal_pending(struct task_struct *p)
+static inline int __must_check signal_pending(struct task_struct *p)
 {
return unlikely(test_tsk_thread_flag(p,TIF_SIGPENDING));
 }


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: dgap: use schedule_timeout_interruptible() instead of dgap_ms_sleep()

[DaeSeok Youn]

Hi,

2014-09-16 15:31 GMT+09:00 Dan Carpenter :
>
> On Tue, Sep 16, 2014 at 12:33:33PM +0900, Daeseok Youn wrote:
> > @@ -2297,12 +2273,12 @@ static void dgap_tty_close(struct tty_struct *tty, 
> > struct file *file)
> >* Go to sleep to ensure RTS/DTR
> >* have been dropped for modems to see it.
> >*/
> > - if (ch->ch_close_delay) {
> > - spin_unlock_irqrestore(>ch_lock,
> > -lock_flags);
> > - dgap_ms_sleep(ch->ch_close_delay);
> > - spin_lock_irqsave(>ch_lock, lock_flags);
> > - }
> > + spin_unlock_irqrestore(>ch_lock,
> > + lock_flags);
> > + /* .25 second delay for dropping RTS/DTR */
> > + schedule_timeout_interruptible(msecs_to_jiffies(250));
> > + signal_pending(current);
>
> There is no point in calling signal_pending() if you don't care about
> the return value.
Yes, you're right.
Originally it didn't have any handling of the return value from
signal_pending().
So, I will re-send this patch after removing signal_pending().

Thanks.

regards,
Daeseok Youn
>
> > + spin_lock_irqsave(>ch_lock, lock_flags);
> >   }
> >
>
> regards,
> dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: dgap: use schedule_timeout_interruptible() instead of dgap_ms_sleep()

[Dan Carpenter]

On Tue, Sep 16, 2014 at 12:33:33PM +0900, Daeseok Youn wrote:
> @@ -2297,12 +2273,12 @@ static void dgap_tty_close(struct tty_struct *tty, 
> struct file *file)
>* Go to sleep to ensure RTS/DTR
>* have been dropped for modems to see it.
>*/
> - if (ch->ch_close_delay) {
> - spin_unlock_irqrestore(>ch_lock,
> -lock_flags);
> - dgap_ms_sleep(ch->ch_close_delay);
> - spin_lock_irqsave(>ch_lock, lock_flags);
> - }
> + spin_unlock_irqrestore(>ch_lock,
> + lock_flags);
> + /* .25 second delay for dropping RTS/DTR */
> + schedule_timeout_interruptible(msecs_to_jiffies(250));
> + signal_pending(current);

There is no point in calling signal_pending() if you don't care about
the return value.

> + spin_lock_irqsave(>ch_lock, lock_flags);
>   }
>  

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: dgap: use schedule_timeout_interruptible() instead of dgap_ms_sleep()

[Dan Carpenter]

On Tue, Sep 16, 2014 at 12:33:33PM +0900, Daeseok Youn wrote:
 @@ -2297,12 +2273,12 @@ static void dgap_tty_close(struct tty_struct *tty, 
 struct file *file)
* Go to sleep to ensure RTS/DTR
* have been dropped for modems to see it.
*/
 - if (ch-ch_close_delay) {
 - spin_unlock_irqrestore(ch-ch_lock,
 -lock_flags);
 - dgap_ms_sleep(ch-ch_close_delay);
 - spin_lock_irqsave(ch-ch_lock, lock_flags);
 - }
 + spin_unlock_irqrestore(ch-ch_lock,
 + lock_flags);
 + /* .25 second delay for dropping RTS/DTR */
 + schedule_timeout_interruptible(msecs_to_jiffies(250));
 + signal_pending(current);

There is no point in calling signal_pending() if you don't care about
the return value.

 + spin_lock_irqsave(ch-ch_lock, lock_flags);
   }
  

regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: dgap: use schedule_timeout_interruptible() instead of dgap_ms_sleep()

[DaeSeok Youn]

Hi,

2014-09-16 15:31 GMT+09:00 Dan Carpenter dan.carpen...@oracle.com:

 On Tue, Sep 16, 2014 at 12:33:33PM +0900, Daeseok Youn wrote:
  @@ -2297,12 +2273,12 @@ static void dgap_tty_close(struct tty_struct *tty, 
  struct file *file)
 * Go to sleep to ensure RTS/DTR
 * have been dropped for modems to see it.
 */
  - if (ch-ch_close_delay) {
  - spin_unlock_irqrestore(ch-ch_lock,
  -lock_flags);
  - dgap_ms_sleep(ch-ch_close_delay);
  - spin_lock_irqsave(ch-ch_lock, lock_flags);
  - }
  + spin_unlock_irqrestore(ch-ch_lock,
  + lock_flags);
  + /* .25 second delay for dropping RTS/DTR */
  + schedule_timeout_interruptible(msecs_to_jiffies(250));
  + signal_pending(current);

 There is no point in calling signal_pending() if you don't care about
 the return value.
Yes, you're right.
Originally it didn't have any handling of the return value from
signal_pending().
So, I will re-send this patch after removing signal_pending().

Thanks.

regards,
Daeseok Youn

  + spin_lock_irqsave(ch-ch_lock, lock_flags);
}
 

 regards,
 dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: dgap: use schedule_timeout_interruptible() instead of dgap_ms_sleep()

[Joe Perches]

On Tue, 2014-09-16 at 09:31 +0300, Dan Carpenter wrote:
 There is no point in calling signal_pending() if you don't care about
 the return value.

So maybe make it __must_check?
---
 include/linux/sched.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7d799ea..03273c0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2692,7 +2692,7 @@ static inline int restart_syscall(void)
return -ERESTARTNOINTR;
 }
 
-static inline int signal_pending(struct task_struct *p)
+static inline int __must_check signal_pending(struct task_struct *p)
 {
return unlikely(test_tsk_thread_flag(p,TIF_SIGPENDING));
 }


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: dgap: use schedule_timeout_interruptible() instead of dgap_ms_sleep()

[Daeseok Youn]

Using schedule_timeout_interruptible() is exactly same as
setting a status of current process and calling  schedule_timeout().

Removes dgap_ms_sleep(), because this function is used
only when closing tty channel on dgap_tty_close().
And also removes ch_close_delay that is always set to 250
on dgap_tty_init().

Signed-off-by: Daeseok Youn 
---
 drivers/staging/dgap/dgap.c |   36 ++--
 drivers/staging/dgap/dgap.h |3 ---
 2 files changed, 6 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 67da1d5..8aff0de 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -180,7 +180,6 @@ static char *dgap_create_config_string(struct board_t *bd, 
char *string);
 static uint dgap_config_get_useintr(struct board_t *bd);
 static uint dgap_config_get_altpin(struct board_t *bd);
 
-static int dgap_ms_sleep(ulong ms);
 static void dgap_do_bios_load(struct board_t *brd, const u8 *ubios, int len);
 static void dgap_do_fep_load(struct board_t *brd, const u8 *ufep, int len);
 #ifdef DIGI_CONCENTRATORS_SUPPORTED
@@ -1200,26 +1199,6 @@ static void dgap_init_globals(void)
 
 /
  *
- * Utility functions
- *
- /
-
-/*
- * dgap_ms_sleep()
- *
- * Put the driver to sleep for x ms's
- *
- * Returns 0 if timed out, !0 (showing signal) if interrupted by a signal.
- */
-static int dgap_ms_sleep(ulong ms)
-{
-   current->state = TASK_INTERRUPTIBLE;
-   schedule_timeout((ms * HZ) / 1000);
-   return signal_pending(current);
-}
-
-/
- *
  * TTY Initialization/Cleanup Functions
  *
  /
@@ -1462,9 +1441,6 @@ static int dgap_tty_init(struct board_t *brd)
ch->ch_tstart = 0;
ch->ch_rstart = 0;
 
-   /* .25 second delay */
-   ch->ch_close_delay = 250;
-
/*
 * Set queue water marks, interrupt mask,
 * and general tty parameters.
@@ -2297,12 +2273,12 @@ static void dgap_tty_close(struct tty_struct *tty, 
struct file *file)
 * Go to sleep to ensure RTS/DTR
 * have been dropped for modems to see it.
 */
-   if (ch->ch_close_delay) {
-   spin_unlock_irqrestore(>ch_lock,
-  lock_flags);
-   dgap_ms_sleep(ch->ch_close_delay);
-   spin_lock_irqsave(>ch_lock, lock_flags);
-   }
+   spin_unlock_irqrestore(>ch_lock,
+   lock_flags);
+   /* .25 second delay for dropping RTS/DTR */
+   schedule_timeout_interruptible(msecs_to_jiffies(250));
+   signal_pending(current);
+   spin_lock_irqsave(>ch_lock, lock_flags);
}
 
ch->pscan_state = 0;
diff --git a/drivers/staging/dgap/dgap.h b/drivers/staging/dgap/dgap.h
index a0307b9..ba05c65 100644
--- a/drivers/staging/dgap/dgap.h
+++ b/drivers/staging/dgap/dgap.h
@@ -982,9 +982,6 @@ struct channel_t {
u32 ch_open_count;  /* open count   */
u32 ch_flags;   /* Channel flags*/
 
-   u32 ch_close_delay; /* How long we should drop  */
-   /* RTS/DTR for  */
-
u32 ch_cpstime; /* Time for CPS calculations*/
 
tcflag_t ch_c_iflag;/* channel iflags   */
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: dgap: use schedule_timeout_interruptible() instead of dgap_ms_sleep()

[Daeseok Youn]

Using schedule_timeout_interruptible() is exactly same as
setting a status of current process and calling  schedule_timeout().

Removes dgap_ms_sleep(), because this function is used
only when closing tty channel on dgap_tty_close().
And also removes ch_close_delay that is always set to 250
on dgap_tty_init().

Signed-off-by: Daeseok Youn daeseok.y...@gmail.com
---
 drivers/staging/dgap/dgap.c |   36 ++--
 drivers/staging/dgap/dgap.h |3 ---
 2 files changed, 6 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 67da1d5..8aff0de 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -180,7 +180,6 @@ static char *dgap_create_config_string(struct board_t *bd, 
char *string);
 static uint dgap_config_get_useintr(struct board_t *bd);
 static uint dgap_config_get_altpin(struct board_t *bd);
 
-static int dgap_ms_sleep(ulong ms);
 static void dgap_do_bios_load(struct board_t *brd, const u8 *ubios, int len);
 static void dgap_do_fep_load(struct board_t *brd, const u8 *ufep, int len);
 #ifdef DIGI_CONCENTRATORS_SUPPORTED
@@ -1200,26 +1199,6 @@ static void dgap_init_globals(void)
 
 /
  *
- * Utility functions
- *
- /
-
-/*
- * dgap_ms_sleep()
- *
- * Put the driver to sleep for x ms's
- *
- * Returns 0 if timed out, !0 (showing signal) if interrupted by a signal.
- */
-static int dgap_ms_sleep(ulong ms)
-{
-   current-state = TASK_INTERRUPTIBLE;
-   schedule_timeout((ms * HZ) / 1000);
-   return signal_pending(current);
-}
-
-/
- *
  * TTY Initialization/Cleanup Functions
  *
  /
@@ -1462,9 +1441,6 @@ static int dgap_tty_init(struct board_t *brd)
ch-ch_tstart = 0;
ch-ch_rstart = 0;
 
-   /* .25 second delay */
-   ch-ch_close_delay = 250;
-
/*
 * Set queue water marks, interrupt mask,
 * and general tty parameters.
@@ -2297,12 +2273,12 @@ static void dgap_tty_close(struct tty_struct *tty, 
struct file *file)
 * Go to sleep to ensure RTS/DTR
 * have been dropped for modems to see it.
 */
-   if (ch-ch_close_delay) {
-   spin_unlock_irqrestore(ch-ch_lock,
-  lock_flags);
-   dgap_ms_sleep(ch-ch_close_delay);
-   spin_lock_irqsave(ch-ch_lock, lock_flags);
-   }
+   spin_unlock_irqrestore(ch-ch_lock,
+   lock_flags);
+   /* .25 second delay for dropping RTS/DTR */
+   schedule_timeout_interruptible(msecs_to_jiffies(250));
+   signal_pending(current);
+   spin_lock_irqsave(ch-ch_lock, lock_flags);
}
 
ch-pscan_state = 0;
diff --git a/drivers/staging/dgap/dgap.h b/drivers/staging/dgap/dgap.h
index a0307b9..ba05c65 100644
--- a/drivers/staging/dgap/dgap.h
+++ b/drivers/staging/dgap/dgap.h
@@ -982,9 +982,6 @@ struct channel_t {
u32 ch_open_count;  /* open count   */
u32 ch_flags;   /* Channel flags*/
 
-   u32 ch_close_delay; /* How long we should drop  */
-   /* RTS/DTR for  */
-
u32 ch_cpstime; /* Time for CPS calculations*/
 
tcflag_t ch_c_iflag;/* channel iflags   */
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/