Re: [PATCH] ALSA: snd-usb: fix IRQ triggered NULL pointer dereference - Ioan-Adrian Ratiu

From: Ioan-Adrian Ratiu <adi@adirat.com>
To: Takashi Iwai <tiwai@suse.de>
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
Date: Fri, 30 Dec 2016 23:00:30 +0200	[thread overview]
Message-ID: <87inq1i13l.fsf@adiPC.i-did-not-set--mail-host-address--so-tickle-me> (raw)
In-Reply-To: <s5hd1g9zdjv.wl-tiwai@suse.de>

On Fri, 30 Dec 2016, Takashi Iwai <tiwai@suse.de> wrote:
> 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 :)

Yes, this makes sense. Thank you.

>
> 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.

I'll find some time to do a rebase either tomorrow (31 Dec) or until
2 Jan at the latest.

Ionel

>
>
> thanks,
>
> Takashi