[PATCH] mips/ad1843: convert time to jiffies HZ independent

[PATCH] mips/ad1843: convert time to jiffies HZ independent

[Nicholas Mc Guire]

PI compliance scanning with coccinelle flagged:
./sound/mips/ad1843.c:503:2-32: WARNING: 
	timeout (5) seems HZ dependent

This was introduced in 'commit 862c2c0a61c5 ("ALSA: ALSA driver for SGI O2
audio board")'. schedule_timeout_interruptible() expects a timeout in
jiffies so the numeric constant makes the effective timeout HZ dependent.
Simply put it through msecs_to_jiffies() to make it HZ independent.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

The loop here is bounded by 500 jiffies and it does not seem reasonable that
it would loop 10 times faster on HZ=1000 configs over HZ=100 configs. The
conversion via msecs_to_jiffies(5) ensures that it will loop at most 100
times for all configs of HZ which seems reasonable (to me) but needs to be
checked by someone that knows the details of this driver. 

Note that the CONFIG_SND_PCM_OSS dependency was not listed in the 
Depends on: in Kconfig (which it probably should)

Patch was compile tested with ip32_defconfig + CONFIG_SOUND=m,
CONFIG_SND=m, CONFIG_SND_PCM_OSS=y, CONFIG_SND_SGI_O2=m

Patch is against 4.1-rc7 (localversion-next is -next-20150612)

 sound/mips/ad1843.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/mips/ad1843.c b/sound/mips/ad1843.c
index 5869075..ec22b55 100644
--- a/sound/mips/ad1843.c
+++ b/sound/mips/ad1843.c
@@ -500,7 +500,7 @@ int ad1843_init(struct snd_ad1843 *ad1843)
 			       "ad1843: AD1843 won't power up\n");
 			return -EIO;
 		}
-		schedule_timeout_interruptible(5);
+		schedule_timeout_interruptible(msecs_to_jiffies(5));
 	}
 
 	/* 5. Power up the clock generators and enable clock output pins. */
-- 
1.7.10.4

Re: [PATCH] mips/ad1843: convert time to jiffies HZ independent

[Takashi Iwai]

At Sun, 14 Jun 2015 10:20:59 +0200,
Nicholas Mc Guire wrote:
> 
> PI compliance scanning with coccinelle flagged:
> ./sound/mips/ad1843.c:503:2-32: WARNING: 
> 	timeout (5) seems HZ dependent
> 
> This was introduced in 'commit 862c2c0a61c5 ("ALSA: ALSA driver for SGI O2
> audio board")'. schedule_timeout_interruptible() expects a timeout in
> jiffies so the numeric constant makes the effective timeout HZ dependent.
> Simply put it through msecs_to_jiffies() to make it HZ independent.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
> 
> The loop here is bounded by 500 jiffies and it does not seem reasonable that
> it would loop 10 times faster on HZ=1000 configs over HZ=100 configs. The
> conversion via msecs_to_jiffies(5) ensures that it will loop at most 100
> times for all configs of HZ which seems reasonable (to me) but needs to be
> checked by someone that knows the details of this driver. 

Using 5ms looks reasonable for this case.  However,
schedule_timeout_interruptible() isn't.  This is a loop that won't
break.  So, the best would be to replace it with msleep() or its
friends.

> Note that the CONFIG_SND_PCM_OSS dependency was not listed in the 
> Depends on: in Kconfig (which it probably should)

Why at all?  It has nothing to do with OSS.


thanks,

Takashi


> Patch was compile tested with ip32_defconfig + CONFIG_SOUND=m,
> CONFIG_SND=m, CONFIG_SND_PCM_OSS=y, CONFIG_SND_SGI_O2=m
> 
> Patch is against 4.1-rc7 (localversion-next is -next-20150612)
> 
>  sound/mips/ad1843.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/mips/ad1843.c b/sound/mips/ad1843.c
> index 5869075..ec22b55 100644
> --- a/sound/mips/ad1843.c
> +++ b/sound/mips/ad1843.c
> @@ -500,7 +500,7 @@ int ad1843_init(struct snd_ad1843 *ad1843)
>  			       "ad1843: AD1843 won't power up\n");
>  			return -EIO;
>  		}
> -		schedule_timeout_interruptible(5);
> +		schedule_timeout_interruptible(msecs_to_jiffies(5));
>  	}
>  
>  	/* 5. Power up the clock generators and enable clock output pins. */
> -- 
> 1.7.10.4
> 

Re: [PATCH] mips/ad1843: convert time to jiffies HZ independent

[Nicholas Mc Guire]

On Sun, 14 Jun 2015, Takashi Iwai wrote:

> At Sun, 14 Jun 2015 10:20:59 +0200,
> Nicholas Mc Guire wrote:
> > 
> > PI compliance scanning with coccinelle flagged:
> > ./sound/mips/ad1843.c:503:2-32: WARNING: 
> > 	timeout (5) seems HZ dependent
> > 
> > This was introduced in 'commit 862c2c0a61c5 ("ALSA: ALSA driver for SGI O2
> > audio board")'. schedule_timeout_interruptible() expects a timeout in
> > jiffies so the numeric constant makes the effective timeout HZ dependent.
> > Simply put it through msecs_to_jiffies() to make it HZ independent.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > The loop here is bounded by 500 jiffies and it does not seem reasonable that
> > it would loop 10 times faster on HZ=1000 configs over HZ=100 configs. The
> > conversion via msecs_to_jiffies(5) ensures that it will loop at most 100
> > times for all configs of HZ which seems reasonable (to me) but needs to be
> > checked by someone that knows the details of this driver. 
> 
> Using 5ms looks reasonable for this case.  However,
> schedule_timeout_interruptible() isn't.  This is a loop that won't
> break.  So, the best would be to replace it with msleep() or its
> friends.
> 
> > Note that the CONFIG_SND_PCM_OSS dependency was not listed in the 
> > Depends on: in Kconfig (which it probably should)
> 
> Why at all?  It has nothing to do with OSS.
>
the procedure to see the problem I used was
(linux-next 20150612)
make ip32_defconfig ARCH=mips
make menuconfig ARCH=mips
  Device Drivers
    ...
    <M> Sound card support  --->
        <M>   Advanced Linux Sound Architecture  --->
            [*]   MIPS sound devices  --->
                  <M>   SGI O2 Audio

compiling fails with:
  MODPOST 95 modules
ERROR: "snd_pcm_period_elapsed" [sound/mips/snd-sgi-o2.ko] undefined!
ERROR: "snd_pcm_set_ops" [sound/mips/snd-sgi-o2.ko] undefined!
ERROR: "snd_pcm_lib_get_vmalloc_page" [sound/mips/snd-sgi-o2.ko] undefined!
ERROR: "snd_pcm_lib_ioctl" [sound/mips/snd-sgi-o2.ko] undefined!
ERROR: "_snd_pcm_lib_alloc_vmalloc_buffer" [sound/mips/snd-sgi-o2.ko] undefined!
ERROR: "snd_pcm_new" [sound/mips/snd-sgi-o2.ko] undefined!
ERROR: "snd_pcm_lib_free_vmalloc_buffer" [sound/mips/snd-sgi-o2.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

as the pcm_lib functions depend on SND_PCM I added that - So
if I add the below patch the problem goes away and the module 
build correclty - not saying that this is the right fix though
(did not look into it in more detail yet)

>From d9cb7d166613e6129f021a913f66f377f5ac56c0 Mon Sep 17 00:00:00 2001
From: Nicholas Mc Guire <hofrat@osadl.org>
Date: Sun, 14 Jun 2015 19:06:19 +0200
Subject: [PATCH] ALSA: let SND_SGI_O2 select SND_PCM

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
 sound/mips/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/mips/Kconfig b/sound/mips/Kconfig
index d2f615a..7875af0 100644
--- a/sound/mips/Kconfig
+++ b/sound/mips/Kconfig
@@ -12,6 +12,7 @@ if SND_MIPS
 config SND_SGI_O2
 	tristate "SGI O2 Audio"
 	depends on SGI_IP32
+	select SND_PCM
         help
                 Sound support for the SGI O2 Workstation. 
 
-- 
1.7.10.4

Re: [PATCH] mips/ad1843: convert time to jiffies HZ independent

[Takashi Iwai]

At Sun, 14 Jun 2015 19:16:59 +0200,
Nicholas Mc Guire wrote:
> 
> On Sun, 14 Jun 2015, Takashi Iwai wrote:
> 
> > At Sun, 14 Jun 2015 10:20:59 +0200,
> > Nicholas Mc Guire wrote:
> > > 
> > > PI compliance scanning with coccinelle flagged:
> > > ./sound/mips/ad1843.c:503:2-32: WARNING: 
> > > 	timeout (5) seems HZ dependent
> > > 
> > > This was introduced in 'commit 862c2c0a61c5 ("ALSA: ALSA driver for SGI O2
> > > audio board")'. schedule_timeout_interruptible() expects a timeout in
> > > jiffies so the numeric constant makes the effective timeout HZ dependent.
> > > Simply put it through msecs_to_jiffies() to make it HZ independent.
> > > 
> > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > ---
> > > 
> > > The loop here is bounded by 500 jiffies and it does not seem reasonable that
> > > it would loop 10 times faster on HZ=1000 configs over HZ=100 configs. The
> > > conversion via msecs_to_jiffies(5) ensures that it will loop at most 100
> > > times for all configs of HZ which seems reasonable (to me) but needs to be
> > > checked by someone that knows the details of this driver. 
> > 
> > Using 5ms looks reasonable for this case.  However,
> > schedule_timeout_interruptible() isn't.  This is a loop that won't
> > break.  So, the best would be to replace it with msleep() or its
> > friends.
> > 
> > > Note that the CONFIG_SND_PCM_OSS dependency was not listed in the 
> > > Depends on: in Kconfig (which it probably should)
> > 
> > Why at all?  It has nothing to do with OSS.
> >
> the procedure to see the problem I used was
> (linux-next 20150612)
> make ip32_defconfig ARCH=mips
> make menuconfig ARCH=mips
>   Device Drivers
>     ...
>     <M> Sound card support  --->
>         <M>   Advanced Linux Sound Architecture  --->
>             [*]   MIPS sound devices  --->
>                   <M>   SGI O2 Audio
> 
> compiling fails with:
>   MODPOST 95 modules
> ERROR: "snd_pcm_period_elapsed" [sound/mips/snd-sgi-o2.ko] undefined!
> ERROR: "snd_pcm_set_ops" [sound/mips/snd-sgi-o2.ko] undefined!
> ERROR: "snd_pcm_lib_get_vmalloc_page" [sound/mips/snd-sgi-o2.ko] undefined!
> ERROR: "snd_pcm_lib_ioctl" [sound/mips/snd-sgi-o2.ko] undefined!
> ERROR: "_snd_pcm_lib_alloc_vmalloc_buffer" [sound/mips/snd-sgi-o2.ko] undefined!
> ERROR: "snd_pcm_new" [sound/mips/snd-sgi-o2.ko] undefined!
> ERROR: "snd_pcm_lib_free_vmalloc_buffer" [sound/mips/snd-sgi-o2.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> 
> as the pcm_lib functions depend on SND_PCM I added that - So
> if I add the below patch the problem goes away and the module 
> build correclty - not saying that this is the right fix though
> (did not look into it in more detail yet)

Yes, this dependency fix is correct, and I queued your patch now.
But the dependency on CONFIG_SND_PCM_OSS is wrong.

A similar fix is needed for HAL2, and I fixed it, too.


thanks,

Takashi

> >From d9cb7d166613e6129f021a913f66f377f5ac56c0 Mon Sep 17 00:00:00 2001
> From: Nicholas Mc Guire <hofrat@osadl.org>
> Date: Sun, 14 Jun 2015 19:06:19 +0200
> Subject: [PATCH] ALSA: let SND_SGI_O2 select SND_PCM
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>  sound/mips/Kconfig |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/mips/Kconfig b/sound/mips/Kconfig
> index d2f615a..7875af0 100644
> --- a/sound/mips/Kconfig
> +++ b/sound/mips/Kconfig
> @@ -12,6 +12,7 @@ if SND_MIPS
>  config SND_SGI_O2
>  	tristate "SGI O2 Audio"
>  	depends on SGI_IP32
> +	select SND_PCM
>          help
>                  Sound support for the SGI O2 Workstation. 
>  
> -- 
> 1.7.10.4
> 

Re: [PATCH] mips/ad1843: convert time to jiffies HZ independent

[Nicholas Mc Guire]

On Mon, 15 Jun 2015, Takashi Iwai wrote:

> At Sun, 14 Jun 2015 19:16:59 +0200,
> Nicholas Mc Guire wrote:
> > 
> > On Sun, 14 Jun 2015, Takashi Iwai wrote:
> > 
> > > At Sun, 14 Jun 2015 10:20:59 +0200,
> > > Nicholas Mc Guire wrote:
> > > > 
> > > > PI compliance scanning with coccinelle flagged:
> > > > ./sound/mips/ad1843.c:503:2-32: WARNING: 
> > > > 	timeout (5) seems HZ dependent
> > > > 
> > > > This was introduced in 'commit 862c2c0a61c5 ("ALSA: ALSA driver for SGI O2
> > > > audio board")'. schedule_timeout_interruptible() expects a timeout in
> > > > jiffies so the numeric constant makes the effective timeout HZ dependent.
> > > > Simply put it through msecs_to_jiffies() to make it HZ independent.
> > > > 
> > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > > ---
> > > > 
> > > > The loop here is bounded by 500 jiffies and it does not seem reasonable that
> > > > it would loop 10 times faster on HZ=1000 configs over HZ=100 configs. The
> > > > conversion via msecs_to_jiffies(5) ensures that it will loop at most 100
> > > > times for all configs of HZ which seems reasonable (to me) but needs to be
> > > > checked by someone that knows the details of this driver. 
> > > 
> > > Using 5ms looks reasonable for this case.  However,
> > > schedule_timeout_interruptible() isn't.  This is a loop that won't
> > > break.  So, the best would be to replace it with msleep() or its
> > > friends.
> > > 
> > > > Note that the CONFIG_SND_PCM_OSS dependency was not listed in the 
> > > > Depends on: in Kconfig (which it probably should)
> > > 
> > > Why at all?  It has nothing to do with OSS.
> > >
> > the procedure to see the problem I used was
> > (linux-next 20150612)
> > make ip32_defconfig ARCH=mips
> > make menuconfig ARCH=mips
> >   Device Drivers
> >     ...
> >     <M> Sound card support  --->
> >         <M>   Advanced Linux Sound Architecture  --->
> >             [*]   MIPS sound devices  --->
> >                   <M>   SGI O2 Audio
> > 
> > compiling fails with:
> >   MODPOST 95 modules
> > ERROR: "snd_pcm_period_elapsed" [sound/mips/snd-sgi-o2.ko] undefined!
> > ERROR: "snd_pcm_set_ops" [sound/mips/snd-sgi-o2.ko] undefined!
> > ERROR: "snd_pcm_lib_get_vmalloc_page" [sound/mips/snd-sgi-o2.ko] undefined!
> > ERROR: "snd_pcm_lib_ioctl" [sound/mips/snd-sgi-o2.ko] undefined!
> > ERROR: "_snd_pcm_lib_alloc_vmalloc_buffer" [sound/mips/snd-sgi-o2.ko] undefined!
> > ERROR: "snd_pcm_new" [sound/mips/snd-sgi-o2.ko] undefined!
> > ERROR: "snd_pcm_lib_free_vmalloc_buffer" [sound/mips/snd-sgi-o2.ko] undefined!
> > make[1]: *** [__modpost] Error 1
> > make: *** [modules] Error 2
> > 
> > as the pcm_lib functions depend on SND_PCM I added that - So
> > if I add the below patch the problem goes away and the module 
> > build correclty - not saying that this is the right fix though
> > (did not look into it in more detail yet)
> 
> Yes, this dependency fix is correct, and I queued your patch now.
> But the dependency on CONFIG_SND_PCM_OSS is wrong.
>

Not too suprised I got this wrong - which is also why I did not submit
this patch - but if you took care of it all the better !

> A similar fix is needed for HAL2, and I fixed it, too.
>

thx!
hofrat
 

Re: [alsa-devel] [PATCH] mips/ad1843: convert time to jiffies HZ independent

[Takashi Iwai]

At Mon, 15 Jun 2015 18:27:27 +0200,
Nicholas Mc Guire wrote:
> 
> On Mon, 15 Jun 2015, Takashi Iwai wrote:
> 
> > At Sun, 14 Jun 2015 19:16:59 +0200,
> > Nicholas Mc Guire wrote:
> > > 
> > > On Sun, 14 Jun 2015, Takashi Iwai wrote:
> > > 
> > > > At Sun, 14 Jun 2015 10:20:59 +0200,
> > > > Nicholas Mc Guire wrote:
> > > > > 
> > > > > PI compliance scanning with coccinelle flagged:
> > > > > ./sound/mips/ad1843.c:503:2-32: WARNING: 
> > > > > 	timeout (5) seems HZ dependent
> > > > > 
> > > > > This was introduced in 'commit 862c2c0a61c5 ("ALSA: ALSA driver for SGI O2
> > > > > audio board")'. schedule_timeout_interruptible() expects a timeout in
> > > > > jiffies so the numeric constant makes the effective timeout HZ dependent.
> > > > > Simply put it through msecs_to_jiffies() to make it HZ independent.
> > > > > 
> > > > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > > > > ---
> > > > > 
> > > > > The loop here is bounded by 500 jiffies and it does not seem reasonable that
> > > > > it would loop 10 times faster on HZ=1000 configs over HZ=100 configs. The
> > > > > conversion via msecs_to_jiffies(5) ensures that it will loop at most 100
> > > > > times for all configs of HZ which seems reasonable (to me) but needs to be
> > > > > checked by someone that knows the details of this driver. 
> > > > 
> > > > Using 5ms looks reasonable for this case.  However,
> > > > schedule_timeout_interruptible() isn't.  This is a loop that won't
> > > > break.  So, the best would be to replace it with msleep() or its
> > > > friends.
> > > > 
> > > > > Note that the CONFIG_SND_PCM_OSS dependency was not listed in the 
> > > > > Depends on: in Kconfig (which it probably should)
> > > > 
> > > > Why at all?  It has nothing to do with OSS.
> > > >
> > > the procedure to see the problem I used was
> > > (linux-next 20150612)
> > > make ip32_defconfig ARCH=mips
> > > make menuconfig ARCH=mips
> > >   Device Drivers
> > >     ...
> > >     <M> Sound card support  --->
> > >         <M>   Advanced Linux Sound Architecture  --->
> > >             [*]   MIPS sound devices  --->
> > >                   <M>   SGI O2 Audio
> > > 
> > > compiling fails with:
> > >   MODPOST 95 modules
> > > ERROR: "snd_pcm_period_elapsed" [sound/mips/snd-sgi-o2.ko] undefined!
> > > ERROR: "snd_pcm_set_ops" [sound/mips/snd-sgi-o2.ko] undefined!
> > > ERROR: "snd_pcm_lib_get_vmalloc_page" [sound/mips/snd-sgi-o2.ko] undefined!
> > > ERROR: "snd_pcm_lib_ioctl" [sound/mips/snd-sgi-o2.ko] undefined!
> > > ERROR: "_snd_pcm_lib_alloc_vmalloc_buffer" [sound/mips/snd-sgi-o2.ko] undefined!
> > > ERROR: "snd_pcm_new" [sound/mips/snd-sgi-o2.ko] undefined!
> > > ERROR: "snd_pcm_lib_free_vmalloc_buffer" [sound/mips/snd-sgi-o2.ko] undefined!
> > > make[1]: *** [__modpost] Error 1
> > > make: *** [modules] Error 2
> > > 
> > > as the pcm_lib functions depend on SND_PCM I added that - So
> > > if I add the below patch the problem goes away and the module 
> > > build correclty - not saying that this is the right fix though
> > > (did not look into it in more detail yet)
> > 
> > Yes, this dependency fix is correct, and I queued your patch now.
> > But the dependency on CONFIG_SND_PCM_OSS is wrong.
> >
> 
> Not too suprised I got this wrong - which is also why I did not submit
> this patch - but if you took care of it all the better !
> 
> > A similar fix is needed for HAL2, and I fixed it, too.
> >
> 
> thx!
> hofrat

Oh, the original patch to fix jiffies value wasn't applied yet, since
I prefer replacing the call with msleep().  Could you respin?


thanks,

Takashi