[alsa-devel] [PATCH 1/9] ALSA: Compress - dont use lock for all ioctls

[Takashi Iwai]

At Tue, 27 Aug 2013 15:14:15 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 12:22:31PM +0200, Takashi Iwai wrote:
> > At Tue, 27 Aug 2013 12:10:31 +0530,
> > Vinod Koul wrote:
> > > 
> > > Some simple ioctls like timsetamp query, capabities query can be done anytime
> > > and should not be under the stream lock. Move these to
> > > snd_compress_simple_iotcls() which is invoked without lock held
> > > 
> > > While at it, improve readblity a bit by sprinkling some empty lines
> > > 
> > > Signed-off-by: Vinod Koul <vinod.koul at intel.com>
> > > Cc: <stable at vger.kernel.org>
> > 
> > Why it's needed for stable?  Fixing any real bugs?
> yup, users are complaining that while streams are draining they can't read
> timstamps. Also one case where a user hit pause didnt go thru as lock preveted
> the pause to be executed..

Then write the problems more clearly.  I saw no such information in
the changelog.

> Since 3.10 is next LTS kernel, I forsee lots of folks using taht for a while so
> makes sense to fix there as well

Depends on the fix and the problem.  The fact that this can't be
tested only with 3.10 kernel (no real driver exists), I doubt it's
worth.  Stable kernels aren't for out-of-tree drivers.

And, if you really target to the stable tree, don't put any
unnecessary changes like white space fixes.  Read
stable_kernel_rules.txt once more.


thanks,

Takashi

> > > +	default:
> > > +		mutex_unlock(&stream->device->lock);
> > > +		return snd_compress_simple_ioctls(f, stream, cmd, arg);
> > 
> > In this code, it still locks/unlocks shortly unnecessarily.
> > It should be rather like:
> > 	switch (_IOC_NR(cmd)) {
> >  	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
> > 		...
> >  	case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
> > 		....	
> > 	default:
> > 		retval = snd_compress_locked_ioctls(f, stream, cmd, arg);
> > 	}
> Hmmm, okay no point in blocking. I will reverse the flow
> 
> ~Vinod
> 
> -- 
>