Date: Fri, 30 Dec 2016 15:39:32 +0100
Message-ID: <s5hd1g9zdjv.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Ioan-Adrian Ratiu <adi@adirat.com>
Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>, clemens@ladisch.de,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ALSA: snd-usb: fix IRQ triggered NULL pointer dereference
In-Reply-To: <87d1glmurl.fsf@ni.com>
References: <20161218010800.2173-1-adi@adirat.com>
        <20161220012514.9348-1-o-takashi@sakamocchi.jp>
        <87r352n7pe.fsf@adi-pc-linux.i-did-not-set--mail-host-address--so-tickle-me>
        <84e2a40b-902c-acfa-771c-d0a675e3228e@sakamocchi.jp>
        <s5hh95xwpqf.wl-tiwai@suse.de>
        <87d1glmurl.fsf@ni.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.1
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2646
Lines: 63

On Wed, 21 Dec 2016 11:38:54 +0100,
Ioan-Adrian Ratiu wrote:
> 
> >> > Please take the time to fully analyze my patch and let's have a
> >> > discussion based on it, not reject it outright and replace it with
> >> > a quick and dirty delay hack.
> >> 
> >> OK. I'll deliberately check it again so that I have no overlooks. I
> >> with this thread also catch the other developers enough helpful to
> >> you. (I just eventually caught your patch in LKML and not developer
> >> for this category of devices.)
> >
> > Sorry for the late reply, as I've been (still) off and had bad net
> > connections.
> >
> > About your fix: Sakamoto-san is right, it's no good way to fix this
> > kind or problem.  The easiest option right now is just to revert my
> > previous fix, as it obviously introduces another regression.  The
> > correct fix will be taken after that.
> >
> > I'm going to prepare a revert patch and ask Linus to take it before
> > rc1.
> 
> I agree with reverting the initial commit decision because my problem
> disappears with it.
> 
> But can you please state a reason for why my patch is "no good way to
> fix"? Being too intrusive is not a good reason.

"Being too intrusive" is the exact reason why it's not good as a
"regression fix" like this case.  The logic you've implemented in the
patch itself looks good (although the code introduces a bug, the
unbalance of snd_usb_*lock_shutdown()).  The only point I couldn't
take it is that it's rather a fundamental change, not a quick fix for
a regression.

What's the worst case scenario in a regression fix?  It's when a fix
introduces yet another regression.  It'd be much worse if the new
regression is deeper.  The complicated logical change has a potential
risk of such.  Thus an intrusive change is not always good as a
"regression fix".

In general, if the change were trivial, it's obviously OK to take as a
regression fix -- where the logic is also trivial in most cases, too.
But when the fix becomes complex, we need to rethink.  Especially when
the original buggy commit is small, reverting it is often better as a
clear cut.

Think in that way: you're addressing a deeper problem that was
revealed accidentally by my previous bad fix.  Writing the change as
if it were merely a regression fix gives the wrong understanding to
readers :)

That said, I'd appreciate if you respin the fix again with a
combination of my previous fix and brush up the code a bit as a whole,
and document it not as a regression fix but as a complete fix of the
existing races.  I can write it in my side quickly, but it'd be almost
in the same form as you posted, I suppose.


thanks,

Takashi