[alsa-devel] [PATCH 0/9] ALSA: compress offfload fixes

[Takashi Iwai]

At Tue, 27 Aug 2013 15:39:55 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 12:46:36PM +0200, Takashi Iwai wrote:
> > At Tue, 27 Aug 2013 12:10:30 +0530,
> > Vinod Koul wrote:
> > > 
> > > Hi Takashi,
> > > 
> > > Here is the patch series which fixes various issues being reported by users (out
> > > of tree sadly)
> > > 
> > > The first three and and last one are marked to stable as would like these to be
> > > fixed in older kernels as well. It would be good if you can send them as fixes
> > > to linus for 3.11.
> > 
> > Sorry, no go.  If it's only about out-of-tree drivers, it's even
> > questionable whether it worth for stable kernel, because we have no
> > real test case for our own.
> Since lot of embedded folks will be on 3.10 then would have been nicer if it
> went to stable.

"It'd be nice" doesn't satisfy the condition for stable kernels,
unfortunately.

> I know users will backport, but if a kernel provided the desired
> functionalty then would have been great. 
> 
> > > Fixes:
> > >  - using lock for all operation was a very bad idead. This is bad as some of the
> > >    ioctls like drain, partial drain can be time consuming and thus prevent any
> > > other operation while these are ongoing like Pause, Stop or timestamp query, so
> > > fix this be removing bunch of ioctls not to use device lock.
> > 
> > Although not all of them need locks, it's easier to manage in most
> > cases to perform in the lock.  For drain or such operation, you can
> > simply unlock and re-lock in the call itself, as your patch in the
> > series does.a
> > 
> > >  - Now we dont have lock for pointer updates so this maybe racy, so use lock
> > >    for doing lowest level calculation. 
> > 
> > Similarly, introducing yet another lock would just choke you neck.
> Well i thought that pointer updates are orthogonal to other triggers so there is
> no issue if we seprate the two. How do you think in long run will this impact..?

If both locks can be never called in nested way, then it'd work.
But if the locks can be nested, better to avoid as much as possible.
Put in that way: what if the time for critical section via
stream->device->lock is short enough (it should be)?

> > >  - As disscused on our sample rate problem, lets move to use rate values and I
> > >    will fix the lib too. Since the driver are not upstream the impact of this
> > > change wont be huge.
> > 
> > I see no code touching sampling_rate field.
> Yes its passed directly to the drivers, where tehy use values to program
> decoders. Only meaning of the field is changing now.

So you're proposing a patch just changing the comment in the header
file as a stable fix patch?  Please reread stable_kernel_rules.txt
once again.

> > >  - Plus few fix like use snprintf, state chacks for pause, write etc..
> > 
> > I don't like to merge irrelevant space changes into a patch if it's a
> > fix patch, especially if it's targeted to stable.  The fix is the fix.
> > Please separate.
> Sure makes sense...
> 
> So do you want these to be sent to stable or not. I would prefer to be sent

This pretty depends on how it can be "fixed"...


Takashi