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
prev parent reply other threads:[~2016-12-30 20:59 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-18 1:08 [PATCH] ALSA: snd-usb: fix IRQ triggered NULL pointer dereference Ioan-Adrian Ratiu
2016-12-20 1:25 ` Takashi Sakamoto
2016-12-20 11:47 ` Ioan-Adrian Ratiu
2016-12-20 13:04 ` Takashi Sakamoto
2016-12-21 10:17 ` Takashi Iwai
2016-12-21 10:38 ` Ioan-Adrian Ratiu
2016-12-30 14:39 ` Takashi Iwai
2016-12-30 21:00 ` Ioan-Adrian Ratiu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87inq1i13l.fsf@adiPC.i-did-not-set--mail-host-address--so-tickle-me \
--to=adi@adirat.com \
--cc=alsa-devel@alsa-project.org \
--cc=clemens@ladisch.de \
--cc=linux-kernel@vger.kernel.org \
--cc=o-takashi@sakamocchi.jp \
--cc=tiwai@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).