Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754221AbcL3U7w (ORCPT ); Fri, 30 Dec 2016 15:59:52 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:35759 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752421AbcL3U7v (ORCPT ); Fri, 30 Dec 2016 15:59:51 -0500 From: Ioan-Adrian Ratiu To: Takashi Iwai Cc: Takashi Sakamoto , 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: 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> <87d1glmurl.fsf@ni.com> Date: Fri, 30 Dec 2016 23:00:30 +0200 Message-ID: <87inq1i13l.fsf@adiPC.i-did-not-set--mail-host-address--so-tickle-me> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2930 Lines: 73 On Fri, 30 Dec 2016, Takashi Iwai 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