[PATCH] ALSA: pcm: fix snd_pcm_playback_silence() with free-running mode
Turns out that we cannot rely on the application pointer being updated in top-up mode, as its primary purpose is to remain operational in free-running mode as used by dmix.
So we logically revert some bits from commit 9f656705c5faa ("ALSA: pcm: rewrite snd_pcm_playback_silence()"), but we retain the bug fixes and try to make the code paths congruent.
Reported-by: Jeff Chua jeff.chua.linux@gmail.com Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/core/pcm_lib.c | 82 +++++++++++++++++++++++++++-------------- sound/core/pcm_local.h | 3 +- sound/core/pcm_native.c | 6 +-- 3 files changed, 60 insertions(+), 31 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index d21c73944efd..cd5f2ef14ab4 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -42,41 +42,69 @@ static int fill_silence_frames(struct snd_pcm_substream *substream, * * when runtime->silence_size >= runtime->boundary - fill processed area with silence immediately */ -void snd_pcm_playback_silence(struct snd_pcm_substream *substream) +void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_uframes_t new_hw_ptr) { struct snd_pcm_runtime *runtime = substream->runtime; - snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr); - snd_pcm_sframes_t added, hw_avail, frames; + snd_pcm_sframes_t hw_avail, frames; snd_pcm_uframes_t noise_dist, ofs, transfer; int err;
- added = appl_ptr - runtime->silence_start; - if (added) { - if (added < 0) - added += runtime->boundary; - if (added < runtime->silence_filled) - runtime->silence_filled -= added; - else - runtime->silence_filled = 0; - runtime->silence_start = appl_ptr; - } - - // This will "legitimately" turn negative on underrun, and will be mangled - // into a huge number by the boundary crossing handling. The initial state - // might also be not quite sane. The code below MUST account for these cases. - hw_avail = appl_ptr - runtime->status->hw_ptr; - if (hw_avail < 0) - hw_avail += runtime->boundary; - - noise_dist = hw_avail + runtime->silence_filled; if (runtime->silence_size < runtime->boundary) { + snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr); + snd_pcm_sframes_t added = appl_ptr - runtime->silence_start; + if (added) { + if (added < 0) + added += runtime->boundary; + if (added < runtime->silence_filled) + runtime->silence_filled -= added; + else + runtime->silence_filled = 0; + runtime->silence_start = appl_ptr; + } + + if (new_hw_ptr == ULONG_MAX) // initialization + new_hw_ptr = runtime->status->hw_ptr; + // This will "legitimately" turn negative on underrun, and will be mangled + // into a huge number by the boundary crossing handling. The initial state + // might also be not quite sane. The code below MUST account for these cases. + hw_avail = appl_ptr - new_hw_ptr; + if (hw_avail < 0) + hw_avail += runtime->boundary; + + noise_dist = hw_avail + runtime->silence_filled; frames = runtime->silence_threshold - noise_dist; if (frames <= 0) return; if (frames > runtime->silence_size) frames = runtime->silence_size; } else { - frames = runtime->buffer_size - noise_dist; + // This filling mode aims at free-running mode (used for example by dmix), + // which doesn't update the application pointer. + snd_pcm_uframes_t hw_ptr = runtime->status->hw_ptr; + if (new_hw_ptr == ULONG_MAX) { // initialization + // Usually, this is entered while stopped, before data is queued, + // so both pointers are expected to be zero. + hw_avail = runtime->control->appl_ptr - hw_ptr; + if (hw_avail < 0) + hw_avail += runtime->boundary; + // In free-running mode, appl_ptr will be zero even while running, + // so we end up with a huge number. There is no useful way to + // handle this, so we just clear the whole buffer. + runtime->silence_filled = hw_avail > runtime->buffer_size ? 0 : hw_avail; + runtime->silence_start = hw_ptr; + } else { + snd_pcm_sframes_t played = new_hw_ptr - hw_ptr; + if (played) { + if (played < 0) + played += runtime->boundary; + if (played < runtime->silence_filled) + runtime->silence_filled -= played; + else // This may happen due to a reset. + runtime->silence_filled = 0; + runtime->silence_start = new_hw_ptr; + } + } + frames = runtime->buffer_size - runtime->silence_filled; if (frames <= 0) return; } @@ -425,6 +453,10 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, return 0; }
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && + runtime->silence_size > 0) + snd_pcm_playback_silence(substream, new_hw_ptr); + if (in_interrupt) { delta = new_hw_ptr - runtime->hw_ptr_interrupt; if (delta < 0) @@ -442,10 +474,6 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, runtime->hw_ptr_wrap += runtime->boundary; }
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && - runtime->silence_size > 0) - snd_pcm_playback_silence(substream); - update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
return snd_pcm_update_state(substream, runtime); diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h index 42fe3a4e9154..ecb21697ae3a 100644 --- a/sound/core/pcm_local.h +++ b/sound/core/pcm_local.h @@ -29,7 +29,8 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream, struct snd_pcm_runtime *runtime); int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream);
-void snd_pcm_playback_silence(struct snd_pcm_substream *substream); +void snd_pcm_playback_silence(struct snd_pcm_substream *substream, + snd_pcm_uframes_t new_hw_ptr);
static inline snd_pcm_uframes_t snd_pcm_avail(struct snd_pcm_substream *substream) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 3d0c4a5b701b..94185267a7b9 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -958,7 +958,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream, if (snd_pcm_running(substream)) { if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0) - snd_pcm_playback_silence(substream); + snd_pcm_playback_silence(substream, ULONG_MAX); err = snd_pcm_update_state(substream, runtime); } snd_pcm_stream_unlock_irq(substream); @@ -1455,7 +1455,7 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream, __snd_pcm_set_state(runtime, state); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0) - snd_pcm_playback_silence(substream); + snd_pcm_playback_silence(substream, ULONG_MAX); snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTART); }
@@ -1916,7 +1916,7 @@ static void snd_pcm_post_reset(struct snd_pcm_substream *substream, runtime->control->appl_ptr = runtime->status->hw_ptr; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0) - snd_pcm_playback_silence(substream); + snd_pcm_playback_silence(substream, ULONG_MAX); snd_pcm_stream_unlock_irq(substream); }
On 04. 05. 23 15:00, Oswald Buddenhagen wrote:
Turns out that we cannot rely on the application pointer being updated in top-up mode, as its primary purpose is to remain operational in free-running mode as used by dmix.
So we logically revert some bits from commit 9f656705c5faa ("ALSA: pcm: rewrite snd_pcm_playback_silence()"), but we retain the bug fixes and try to make the code paths congruent.
It would be better to revert the broken patch and make changes on top of the original code. This is really difficult to review.
Jaroslav
On Thu, May 4, 2023 at 9:08 PM Jaroslav Kysela perex@perex.cz wrote:
On 04. 05. 23 15:00, Oswald Buddenhagen wrote:
Turns out that we cannot rely on the application pointer being updated in top-up mode, as its primary purpose is to remain operational in free-running mode as used by dmix.
So we logically revert some bits from commit 9f656705c5faa ("ALSA: pcm: rewrite snd_pcm_playback_silence()"), but we retain the bug fixes and try to make the code paths congruent.
It would be better to revert the broken patch and make changes on top of the original code. This is really difficult to review.
Just to confirm that the patch works. Thanks!
Jeff
On Thu, May 04, 2023 at 03:08:14PM +0200, Jaroslav Kysela wrote:
On 04. 05. 23 15:00, Oswald Buddenhagen wrote:
So we logically revert some bits from commit 9f656705c5faa ("ALSA: pcm: rewrite snd_pcm_playback_silence()"), but we retain the bug fixes and try to make the code paths congruent.
It would be better to revert the broken patch and make changes on top of the original code. This is really difficult to review.
the diff to the old code is just as big, which is a somewhat inevitable effect of it being the middle way between both.
the best way to review it is with `git show -b`.
regards
On Thu, 04 May 2023 15:00:07 +0200, Oswald Buddenhagen wrote:
Turns out that we cannot rely on the application pointer being updated in top-up mode, as its primary purpose is to remain operational in free-running mode as used by dmix.
So we logically revert some bits from commit 9f656705c5faa ("ALSA: pcm: rewrite snd_pcm_playback_silence()"), but we retain the bug fixes and try to make the code paths congruent.
Reported-by: Jeff Chua jeff.chua.linux@gmail.com Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de
Honestly speaking, this is really hard to review. As most of changes here are the revert of the previous commit, I'd rather like to get it reverted whole once, and re-apply the proper fix gradually.
The auto-silence function is really messy and fragile, and we can't follow the code changes easily if the things go from left to right and return again.
That said, don't mix the fix and the code refactoring and the revert into a pot, but let's separate them.
Through a quick glance over the patch, my concern is how runtime->silence_start is handled. In the older code, silence_start is the starting offset, while the offset in the current tree is silence_start + silence_filled. Is it really OK to reset the silence_start always when it's updated (in silent_size==boundary case) as in this patch?
thanks,
Takashi
sound/core/pcm_lib.c | 82 +++++++++++++++++++++++++++-------------- sound/core/pcm_local.h | 3 +- sound/core/pcm_native.c | 6 +-- 3 files changed, 60 insertions(+), 31 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index d21c73944efd..cd5f2ef14ab4 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -42,41 +42,69 @@ static int fill_silence_frames(struct snd_pcm_substream *substream,
- when runtime->silence_size >= runtime->boundary - fill processed area with silence immediately
*/ -void snd_pcm_playback_silence(struct snd_pcm_substream *substream) +void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_uframes_t new_hw_ptr) { struct snd_pcm_runtime *runtime = substream->runtime;
- snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);
- snd_pcm_sframes_t added, hw_avail, frames;
- snd_pcm_sframes_t hw_avail, frames; snd_pcm_uframes_t noise_dist, ofs, transfer; int err;
- added = appl_ptr - runtime->silence_start;
- if (added) {
if (added < 0)added += runtime->boundary;if (added < runtime->silence_filled)runtime->silence_filled -= added;elseruntime->silence_filled = 0;runtime->silence_start = appl_ptr;- }
- // This will "legitimately" turn negative on underrun, and will be mangled
- // into a huge number by the boundary crossing handling. The initial state
- // might also be not quite sane. The code below MUST account for these cases.
- hw_avail = appl_ptr - runtime->status->hw_ptr;
- if (hw_avail < 0)
hw_avail += runtime->boundary;- noise_dist = hw_avail + runtime->silence_filled; if (runtime->silence_size < runtime->boundary) {
snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);snd_pcm_sframes_t added = appl_ptr - runtime->silence_start;if (added) {if (added < 0)added += runtime->boundary;if (added < runtime->silence_filled)runtime->silence_filled -= added;elseruntime->silence_filled = 0;runtime->silence_start = appl_ptr;}if (new_hw_ptr == ULONG_MAX) // initializationnew_hw_ptr = runtime->status->hw_ptr;// This will "legitimately" turn negative on underrun, and will be mangled// into a huge number by the boundary crossing handling. The initial state// might also be not quite sane. The code below MUST account for these cases.hw_avail = appl_ptr - new_hw_ptr;if (hw_avail < 0)hw_avail += runtime->boundary;noise_dist = hw_avail + runtime->silence_filled;
frames = runtime->buffer_size - noise_dist;
// This filling mode aims at free-running mode (used for example by dmix),// which doesn't update the application pointer.snd_pcm_uframes_t hw_ptr = runtime->status->hw_ptr;if (new_hw_ptr == ULONG_MAX) { // initialization// Usually, this is entered while stopped, before data is queued,// so both pointers are expected to be zero.hw_avail = runtime->control->appl_ptr - hw_ptr;if (hw_avail < 0)hw_avail += runtime->boundary;// In free-running mode, appl_ptr will be zero even while running,// so we end up with a huge number. There is no useful way to// handle this, so we just clear the whole buffer.runtime->silence_filled = hw_avail > runtime->buffer_size ? 0 : hw_avail;runtime->silence_start = hw_ptr;} else {snd_pcm_sframes_t played = new_hw_ptr - hw_ptr;if (played) {if (played < 0)played += runtime->boundary;if (played < runtime->silence_filled)runtime->silence_filled -= played;else // This may happen due to a reset.runtime->silence_filled = 0;runtime->silence_start = new_hw_ptr;}}frames = runtime->buffer_size - runtime->silence_filled;@@ -425,6 +453,10 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, return 0; }
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
runtime->silence_size > 0)snd_pcm_playback_silence(substream, new_hw_ptr);- if (in_interrupt) { delta = new_hw_ptr - runtime->hw_ptr_interrupt; if (delta < 0)
@@ -442,10 +474,6 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, runtime->hw_ptr_wrap += runtime->boundary; }
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
runtime->silence_size > 0)snd_pcm_playback_silence(substream);update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
return snd_pcm_update_state(substream, runtime);
diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h index 42fe3a4e9154..ecb21697ae3a 100644 --- a/sound/core/pcm_local.h +++ b/sound/core/pcm_local.h @@ -29,7 +29,8 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream, struct snd_pcm_runtime *runtime); int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream);
-void snd_pcm_playback_silence(struct snd_pcm_substream *substream); +void snd_pcm_playback_silence(struct snd_pcm_substream *substream,
snd_pcm_uframes_t new_hw_ptr);static inline snd_pcm_uframes_t snd_pcm_avail(struct snd_pcm_substream *substream) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 3d0c4a5b701b..94185267a7b9 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -958,7 +958,7 @@ static int snd_pcm_sw_params(struct snd_pcm_substream *substream, if (snd_pcm_running(substream)) { if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0)
snd_pcm_playback_silence(substream);
snd_pcm_playback_silence(substream, ULONG_MAX);@@ -1455,7 +1455,7 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream, __snd_pcm_set_state(runtime, state); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0)
snd_pcm_playback_silence(substream);
snd_pcm_playback_silence(substream, ULONG_MAX);}
@@ -1916,7 +1916,7 @@ static void snd_pcm_post_reset(struct snd_pcm_substream *substream, runtime->control->appl_ptr = runtime->status->hw_ptr; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0)
snd_pcm_playback_silence(substream);
snd_pcm_playback_silence(substream, ULONG_MAX);}
-- 2.40.0.152.g15d061e6df
On 04. 05. 23 15:28, Takashi Iwai wrote:
On Thu, 04 May 2023 15:00:07 +0200, Oswald Buddenhagen wrote:
Turns out that we cannot rely on the application pointer being updated in top-up mode, as its primary purpose is to remain operational in free-running mode as used by dmix.
So we logically revert some bits from commit 9f656705c5faa ("ALSA: pcm: rewrite snd_pcm_playback_silence()"), but we retain the bug fixes and try to make the code paths congruent.
Reported-by: Jeff Chua jeff.chua.linux@gmail.com Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de
Honestly speaking, this is really hard to review. As most of changes here are the revert of the previous commit, I'd rather like to get it reverted whole once, and re-apply the proper fix gradually.
I fully agree here. Takashi, please, revert the broken patch right now. I think that the review and improving the code may take some days.
Jaroslav
On Thu, May 04, 2023 at 03:33:01PM +0200, Jaroslav Kysela wrote:
On 04. 05. 23 15:28, Takashi Iwai wrote:
Honestly speaking, this is really hard to review.
As most of changes here are the revert of the previous commit,
no they aren't.
I'd rather like to get it reverted whole once, and re-apply the proper fix gradually.
I fully agree here. Takashi, please, revert the broken patch right now.
actually reverting would include reverting the comments, which would be just plain stupid.
I think that the review and improving the code may take some days.
i'm not going to deliver anything more on that matter just to satisfy some arbitrary process. i think the patch does the right thing, with the right granularity, and i'm not going to spend time breaking my head on producing broken intermediate states which i cannot properly reason about due to their internal inconsistency.
you can "rebase" my patch by checking it out on top of a partial revert, but you need to come up with your own commit message then. and i think that it would be utterly counterproductive. viewing the diff for review purposes may make sense, though.
regards
On Thu, 04 May 2023 15:54:15 +0200, Oswald Buddenhagen wrote:
On Thu, May 04, 2023 at 03:33:01PM +0200, Jaroslav Kysela wrote:
On 04. 05. 23 15:28, Takashi Iwai wrote:
Honestly speaking, this is really hard to review.
As most of changes here are the revert of the previous commit,
no they aren't.
Erm, that is a big part of problems. We don't see clearly what part is the revert to the old logic and what is the actual fix. You mixed things, and it's really hard to follow.
I'd rather like to get it reverted whole once, and re-apply the proper fix gradually.
I fully agree here. Takashi, please, revert the broken patch right now.
actually reverting would include reverting the comments, which would be just plain stupid.
A whole revert sounds too much, but it makes the changes more straightforward afterwards. This is the biggest win. We want to see the fix applied in each commit in the right way. Not in a mixture.
I think that the review and improving the code may take some days.
i'm not going to deliver anything more on that matter just to satisfy some arbitrary process.
It's not "some arbitrary process". The patch review is _the_ most important process, and if a patch makes it difficult, it implies that it's already fundamentally bad.
i think the patch does the right thing, with the right granularity, and i'm not going to spend time breaking my head on producing broken intermediate states which i cannot properly reason about due to their internal inconsistency.
you can "rebase" my patch by checking it out on top of a partial revert, but you need to come up with your own commit message then. and i think that it would be utterly counterproductive. viewing the diff for review purposes may make sense, though.
Sorry, that doesn't work. The review is done upon the patch, and if this patch can't be reviewed easily, it's simply no-go.
Again, the problem is the mixture; it partially reverts to the original code while it modifies some part in some other way. For example, as already pointed, only from this patch, it's not clear whether the handling of silence_start in the patched code is OK or not. That's no part of revert, and I can't judge whether it's the correct and intended change or an oversight.
By reverting the whole and reapplying fixes -- although it'll need more steps -- we can see more clearly what change fixes which part. The patch granularity, the patch description, rules and whatever, all of these are just for reviewing the patches properly; it results in better understanding and in the good code in the end.
Takashi
On Thu, May 04, 2023 at 04:49:58PM +0200, Takashi Iwai wrote:
Sorry, that doesn't work. The review is done upon the patch, and if this patch can't be reviewed easily, it's simply no-go.
that's a self-imposed limitation.
it's beyond me why in 2023 anyone working on a bigger project is still using a patch-based review process, given the existence of proper review tools like gerrit (and crutches like github and gitlab).
i always view patches with 10 lines of context, and regularly use the "expand context" widgets to get even more into view. in the small projects i maintain i apply more complex patches first and view them with -U10 or more.
"i don't see enough to judge this" isn't a complaint that would ever occur to me leveling at a contributor.
Again, the problem is the mixture; it partially reverts to the original code while it modifies some part in some other way.
the baseline is irrelevant - it was already broken, and almost impossible to reason about.
By reverting the whole and reapplying fixes -- although it'll need more steps -- we can see more clearly what change fixes which part.
that's not what actually happens. this is all deeply intertwined code. splitting up the patch will merely give you the *illusion* of better understanding the pieces. but to actually make sense of it, you need to see the whole, in its end state, because there are no fully functional intermediate states.
the original patch was three patches at first. when i was attempting to write proper commit messages explaining what fixes what, i found that it's just impossible to untangle it without lying by omission. so i squashed them and wrote a cumulative description. and you accepted it.
regards
On Thu, 04 May 2023 17:36:11 +0200, Oswald Buddenhagen wrote:
On Thu, May 04, 2023 at 04:49:58PM +0200, Takashi Iwai wrote:
Sorry, that doesn't work. The review is done upon the patch, and if this patch can't be reviewed easily, it's simply no-go.
that's a self-imposed limitation.
And that's how the process works. Over decades.
it's beyond me why in 2023 anyone working on a bigger project is still using a patch-based review process, given the existence of proper review tools like gerrit (and crutches like github and gitlab).
All those git-based work flows are based on commits, and commits consist of patches. So, reviewing each commit is nothing but reviewing a patch. IOW, if you do a PR via github, it'll hit the very same problem; when the commit is not understandable enough for reviewers, it'll be NAK'ed.
i always view patches with 10 lines of context, and regularly use the "expand context" widgets to get even more into view. in the small projects i maintain i apply more complex patches first and view them with -U10 or more.
"i don't see enough to judge this" isn't a complaint that would ever occur to me leveling at a contributor.
It's not only about contexts. It's just not clear what your patch does as partial revert and as fix. I really would like to see one change for one fix, and one change for one code refactoring. It's difficult to achieve if we have to partially revert and partially fix in a shot.
Again, the problem is the mixture; it partially reverts to the original code while it modifies some part in some other way.
the baseline is irrelevant - it was already broken, and almost impossible to reason about.
Reverting temporarily to the original state (even if it's the broken state) is the very standard way. It's a clear way to start from the scratch and build up things again more cleanly.
And, if the complain were only mine, I'd reconsider. But it's not one person's view, but multiple reviewers think so, so it's a sign of no-go.
By reverting the whole and reapplying fixes -- although it'll need more steps -- we can see more clearly what change fixes which part.
that's not what actually happens. this is all deeply intertwined code.
Err, I can't follow; in the previous patch and this patch, you added more comments, use different terms and variable names, and use different way to calculate the hw_avail value, etc -- which are basically irrelevant from the behavior fix itself, but they are just code refactoring. Those could be separated easily.
splitting up the patch will merely give you the *illusion* of better understanding the pieces. but to actually make sense of it, you need to see the whole, in its end state, because there are no fully functional intermediate states.
Again, I can't follow your logic. Why splitting into small pieces can't lead to a better understanding *at all*? Why you must refuse it? Certainly one needs to take a look at big picture. But it's a different story.
the original patch was three patches at first. when i was attempting to write proper commit messages explaining what fixes what, i found that it's just impossible to untangle it without lying by omission. so i squashed them and wrote a cumulative description. and you accepted it.
The acceptance of your patch was my failure, yeah. I should have rejected it. So this failure doesn't happen again. You're seeing the result now.
Takashi
On 04. 05. 23 18:34, Takashi Iwai wrote:
The acceptance of your patch was my failure, yeah. I should have rejected it. So this failure doesn't happen again. You're seeing the result now.
We can keep the header comments and just revert the code for now. If Oswald is not willing to work on this further, I'll look into this tomorrow. I see the points to be fixed. But I don't think that we are in hurry - the code was there for years so it's time to fix it properly.
Jaroslav
On Thu, 04 May 2023 19:06:12 +0200, Jaroslav Kysela wrote:
On 04. 05. 23 18:34, Takashi Iwai wrote:
The acceptance of your patch was my failure, yeah. I should have rejected it. So this failure doesn't happen again. You're seeing the result now.
We can keep the header comments and just revert the code for now.
Yes, this sounds like a good approach.
If Oswald is not willing to work on this further, I'll look into this tomorrow. I see the points to be fixed. But I don't think that we are in hurry - the code was there for years so it's time to fix it properly.
That'll be highly appreciated!
Takashi
On Thu, May 04, 2023 at 06:34:42PM +0200, Takashi Iwai wrote:
On Thu, 04 May 2023 17:36:11 +0200, Oswald Buddenhagen wrote:
On Thu, May 04, 2023 at 04:49:58PM +0200, Takashi Iwai wrote:
Sorry, that doesn't work. The review is done upon the patch, and if this patch can't be reviewed easily, it's simply no-go.
that's a self-imposed limitation.
And that's how the process works. Over decades.
that doesn't mean that it's the best process. it's the only thing that was available 30 years ago, but technology has moved on.
All those git-based work flows are based on commits, and commits consist of patches.
yes
So, reviewing each commit is nothing but reviewing a patch.
that's technically correct, but completely misses the point. with a proper review tool, looking beyond the patch itself becomes *much* cheaper, which was the argument here.
in fact, gerrit defaults to side-by-side diff view, so people look at the new code rather than at the patch. (i actually think that's a stupid default, but having the option to switch in a second is extremely valuable.)
IOW, if you do a PR via github, it'll hit the very same problem; when the commit is not understandable enough for reviewers, it'll be NAK'ed.
this is true, but with better tooling there are fewer pointless limitations.
It's not only about contexts. It's just not clear what your patch does as partial revert and as fix.
the fact that it's a partial revert is a property of the transition (that is, the patch) and therefore something to note in the history, but for understanding the correctness of the final code it's utterly irrelevant.
this is all deeply intertwined code.
Err, I can't follow; in the previous patch and this patch, you added more comments, use different terms and variable names, and use different way to calculate the hw_avail value, etc -- which are basically irrelevant from the behavior fix itself, but they are just code refactoring. Those could be separated easily.
some bits can be separated, while others can't. you'd just get a bunch of micro-changes, an insane amount of churn, and maybe two "meaty" patches which wouldn't be any simpler to actually understand.
so "partially rewrite" is imo exactly the right granularity for approaching this.
splitting up the patch will merely give you the *illusion* of better understanding the pieces. but to actually make sense of it, you need to see the whole, in its end state, because there are no fully functional intermediate states.
Again, I can't follow your logic. Why splitting into small pieces can't lead to a better understanding *at all*? Why you must refuse it? Certainly one needs to take a look at big picture. But it's a different story.
patches should be atomic. that means each one should contain one *complete* change. if you split a patch into pieces that aren't complete in themselves, you're just obfuscating the complexity.
i'm not going to try to prove to you that this is the case here. i just know that i failed at atomizing this _properly_ the first time around.
the original patch was three patches at first. when i was attempting to write proper commit messages explaining what fixes what, i found that it's just impossible to untangle it without lying by omission. so i squashed them and wrote a cumulative description. and you accepted it.
The acceptance of your patch was my failure, yeah. I should have rejected it. So this failure doesn't happen again. You're seeing the result now.
i think what i'm actually seeing is that you guys got bitten and are over-compensating. but the patch was good, it fulfilled the documented api contract, and was thoroughly tested accordingly. the only problem was that there was an *undocumented* part of the api contract, and you both failed to notice it. atomizing the patch further wouldn't have changed anything about that. shit happens.
regards
On Thu, May 04, 2023 at 07:53:47PM +0200, Oswald Buddenhagen wrote:
i'm not going to try to prove to you that this is the case here. i just know that i failed at atomizing this _properly_ the first time around.
as it goes, my brain won't go to rest over this and has already done half the work, so expect a new series RSN. i'm sure you'll enjoy the churn ...
On 05. 05. 23 9:17, Oswald Buddenhagen wrote:
On Thu, May 04, 2023 at 07:53:47PM +0200, Oswald Buddenhagen wrote:
i'm not going to try to prove to you that this is the case here. i just know that i failed at atomizing this _properly_ the first time around.
as it goes, my brain won't go to rest over this and has already done half the work, so expect a new series RSN. i'm sure you'll enjoy the churn ...
I already finished my patchset based on your reaction yesterday. I will send my code in few minutes.
Jaroslav
participants (4)
-
Jaroslav Kysela -
Jeff Chua -
Oswald Buddenhagen -
Takashi Iwai