diff mbox

[OPW,kernel] y2038: small task: dgnc: dgnc_tty.c: convert timval to ktime_t

Message ID 20141020223819.GA3558@localhost.localdomain
State New, archived
Headers show

Commit Message

Aya Mahfouz Oct. 20, 2014, 10:38 p.m. UTC
This is one of the small tasks listed for the y2038 project. The
timeval struct defined and used in dgnc_tty.c was converted to ktime_t.
In addition, some function calls were adjusted accordingly.

Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
---
 drivers/staging/dgnc/dgnc_tty.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Arnd Bergmann Oct. 21, 2014, 8:36 a.m. UTC | #1
On Tuesday 21 October 2014 00:38:19 Aya Mahfouz wrote:
> This is one of the small tasks listed for the y2038 project. The
> timeval struct defined and used in dgnc_tty.c was converted to ktime_t.
> In addition, some function calls were adjusted accordingly.

In the subject line, please drop the 'y2038: small task:' part. It
makes sense to mention that in the description, but it's more important
that you describe which problem you are solving (e.g. will the driver
break in 2038, or do you just change the type to remove all instances
of timeval in general), and why you picked ktime_t over the alternatives
(timespec64, jiffies, ...).

> Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
> ---
>  drivers/staging/dgnc/dgnc_tty.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
> index b0fcbe7..dacecf6 100644
> --- a/drivers/staging/dgnc/dgnc_tty.c
> +++ b/drivers/staging/dgnc/dgnc_tty.c
> @@ -455,7 +455,7 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
>   */
>  void dgnc_sniff_nowait_nolock(struct channel_t *ch, unsigned char *text, unsigned char *buf, int len)
>  {
> -       struct timeval tv;
> +       ktime_t tv;
>         int n;
>         int r;
>         int nbuf;

'tv' is typically only used as a variable name for 'timeval'. Better call it
'time' or 'now' here.

> @@ -474,10 +474,10 @@ void dgnc_sniff_nowait_nolock(struct channel_t *ch, unsigned char *text, unsigne
>         if (!(ch->ch_sniff_flags & SNIFF_OPEN))
>                 goto exit;
>  
> -       do_gettimeofday(&tv);
> +       tv = ktime_get();

This changes the time base from realtime to monotonic time. In a lot of cases,
monotonic is what you want, but for printing to user space it normally is not.

>         /* Create our header for data dump */
> -       p += sprintf(p, "<%ld %ld><%s><", tv.tv_sec, tv.tv_usec, text);
> +       p += sprintf(p, "<%ld><%s><", tv.tv64, text);
>         tmpbuflen = p - tmpbuf;
>  

wrong type: tv.tv64 is a 'long long', so you should now get a warning about the
printf format string. In general, never access the .tv64 member of ktime_t
directly, use ktime_to_ns() or similar get the contents. In this case,
using ktime_get_real_ts64() instead of ktime_get() would have probably been
best.

Regarding the process, have a look at http://kernelnewbies.org/OPWTasks, this 
driver was already claimed by Somya Anand, and she has submitted multiple
versions of the patch already. When you pick one of the tasks, first check
that it's not already claimed by someone else, then enter your name in the list.

	Arnd
Aya Mahfouz Oct. 21, 2014, 9:23 a.m. UTC | #2
On Tue, Oct 21, 2014 at 10:36:43AM +0200, Arnd Bergmann wrote:
> On Tuesday 21 October 2014 00:38:19 Aya Mahfouz wrote:
> > This is one of the small tasks listed for the y2038 project. The
> > timeval struct defined and used in dgnc_tty.c was converted to ktime_t.
> > In addition, some function calls were adjusted accordingly.
> 
> In the subject line, please drop the 'y2038: small task:' part. It
> makes sense to mention that in the description, but it's more important
> that you describe which problem you are solving (e.g. will the driver
> break in 2038, or do you just change the type to remove all instances
> of timeval in general), and why you picked ktime_t over the alternatives
> (timespec64, jiffies, ...).
> 

Ok, will do.

> > Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
> > ---
> >  drivers/staging/dgnc/dgnc_tty.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
> > index b0fcbe7..dacecf6 100644
> > --- a/drivers/staging/dgnc/dgnc_tty.c
> > +++ b/drivers/staging/dgnc/dgnc_tty.c
> > @@ -455,7 +455,7 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
> >   */
> >  void dgnc_sniff_nowait_nolock(struct channel_t *ch, unsigned char *text, unsigned char *buf, int len)
> >  {
> > -       struct timeval tv;
> > +       ktime_t tv;
> >         int n;
> >         int r;
> >         int nbuf;
> 
> 'tv' is typically only used as a variable name for 'timeval'. Better call it
> 'time' or 'now' here.
> 
> > @@ -474,10 +474,10 @@ void dgnc_sniff_nowait_nolock(struct channel_t *ch, unsigned char *text, unsigne
> >         if (!(ch->ch_sniff_flags & SNIFF_OPEN))
> >                 goto exit;
> >  
> > -       do_gettimeofday(&tv);
> > +       tv = ktime_get();
> 
> This changes the time base from realtime to monotonic time. In a lot of cases,
> monotonic is what you want, but for printing to user space it normally is not.
> 
> >         /* Create our header for data dump */
> > -       p += sprintf(p, "<%ld %ld><%s><", tv.tv_sec, tv.tv_usec, text);
> > +       p += sprintf(p, "<%ld><%s><", tv.tv64, text);
> >         tmpbuflen = p - tmpbuf;
> >  
> 
> wrong type: tv.tv64 is a 'long long', so you should now get a warning about the
> printf format string. In general, never access the .tv64 member of ktime_t
> directly, use ktime_to_ns() or similar get the contents. In this case,
> using ktime_get_real_ts64() instead of ktime_get() would have probably been
> best.
> 
> Regarding the process, have a look at http://kernelnewbies.org/OPWTasks, this 
> driver was already claimed by Somya Anand, and she has submitted multiple
> versions of the patch already. When you pick one of the tasks, first check
> that it's not already claimed by someone else, then enter your name in the list.
>

Yes, will do that. Sorry for the inconvenience.
 
> 	Arnd

Kind Regards,
Aya Saif El-yazal Mahfouz
Somya Anand Oct. 21, 2014, 2:01 p.m. UTC | #3
On Tuesday, October 21, 2014 4:08:26 AM UTC+5:30, A M wrote:
>
> This is one of the small tasks listed for the y2038 project. The 
> timeval struct defined and used in dgnc_tty.c was converted to ktime_t. 
> In addition, some function calls were adjusted accordingly. 
>
> Signed-off-by: Aya Mahfouz <mahfouz.sa...@gmail.com <javascript:>> 
> --- 
>  drivers/staging/dgnc/dgnc_tty.c | 6 +++--- 
>  1 file changed, 3 insertions(+), 3 deletions(-) 
>
> diff --git a/drivers/staging/dgnc/dgnc_tty.c 
> b/drivers/staging/dgnc/dgnc_tty.c 
> index b0fcbe7..dacecf6 100644 
> --- a/drivers/staging/dgnc/dgnc_tty.c 
> +++ b/drivers/staging/dgnc/dgnc_tty.c 
> @@ -455,7 +455,7 @@ void dgnc_tty_uninit(struct dgnc_board *brd) 
>   */ 
>  void dgnc_sniff_nowait_nolock(struct channel_t *ch, unsigned char *text, 
> unsigned char *buf, int len) 
>  { 
> -        struct timeval tv; 
> +        ktime_t tv; 
>          int n; 
>          int r; 
>          int nbuf; 
> @@ -474,10 +474,10 @@ void dgnc_sniff_nowait_nolock(struct channel_t *ch, 
> unsigned char *text, unsigne 
>          if (!(ch->ch_sniff_flags & SNIFF_OPEN)) 
>                  goto exit; 
>   
> -        do_gettimeofday(&tv); 
> +        tv = ktime_get(); 
>   
>          /* Create our header for data dump */ 
> -        p += sprintf(p, "<%ld %ld><%s><", tv.tv_sec, tv.tv_usec, text); 
> +        p += sprintf(p, "<%ld><%s><", tv.tv64, text); 
>          tmpbuflen = p - tmpbuf; 
>   
>          do { 
> -- 
> 1.9.3 
> Hi Aya,
>

I have claimed this task on the task list and have submitted link to the 
patch submission also. 

Thanks
Somya
diff mbox

Patch

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index b0fcbe7..dacecf6 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -455,7 +455,7 @@  void dgnc_tty_uninit(struct dgnc_board *brd)
  */
 void dgnc_sniff_nowait_nolock(struct channel_t *ch, unsigned char *text, unsigned char *buf, int len)
 {
-	struct timeval tv;
+	ktime_t tv;
 	int n;
 	int r;
 	int nbuf;
@@ -474,10 +474,10 @@  void dgnc_sniff_nowait_nolock(struct channel_t *ch, unsigned char *text, unsigne
 	if (!(ch->ch_sniff_flags & SNIFF_OPEN))
 		goto exit;
 
-	do_gettimeofday(&tv);
+	tv = ktime_get();
 
 	/* Create our header for data dump */
-	p += sprintf(p, "<%ld %ld><%s><", tv.tv_sec, tv.tv_usec, text);
+	p += sprintf(p, "<%ld><%s><", tv.tv64, text);
 	tmpbuflen = p - tmpbuf;
 
 	do {