Re: [alsa-devel] [PATCH] ALSA: snd-usb: fix IRQ triggered NULL pointer dereference
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
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
participants (2)
-
Ioan-Adrian Ratiu -
Takashi Iwai