On Wed, Feb 21, 2024 at 10:00 AM Valentin Obst kernel@valentinobst.de wrote:
While debugging the `X86_DECODER_SELFTEST` failure first reported in [1], [In this case the line causing the failure is interpreted as two lines by the tool (due to its length, but this is fixed by [1, 2]), and the second line is reported. Still the spatial closeness between the reported line and the line causing the failure would have made debugging a lot easier.]
Thanks Valentin, John et al. for digging into this (and the related issue) -- very much appreciated.
It looks good to me:
Reviewed-by: Miguel Ojeda ojeda@kernel.org Tested-by: Miguel Ojeda ojeda@kernel.org
This should probably have a Fixes tag -- from a quick look, the original test did not seem to have the problem because `insns` was equivalent to the number of lines since there was no `if ... { continue; }` for the symbol case. At some point that branch was added, so that was not true anymore, thus that one should probably be the Fixes tag, though please double-check:
Fixes: 35039eb6b199 ("x86: Show symbol name if insn decoder test failed")
It is a minor issue for sure, so perhaps not worth backporting, but nevertheless the hash is in a very old kernel, and thus the issue applies to all stable kernels. So it does not hurt flagging it to the stable team:
Cc: stable@vger.kernel.org
In addition, John reported the original issue, but this one was found due to that one, and I am not exactly sure who did what here. Please consider whether one of the following (or similar) may be fair:
Reported-by: John Baublitz john.m.baublitz@gmail.com Debugged-by: John Baublitz john.m.baublitz@gmail.com
An extra Link to the discussion in Zulip could be nice too:
Link: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/insn_d...
Finally, a nit: links are typically written like the following -- you can still use bracket references at the end:
Link: https://lore.kernel.org/lkml/Y9ES4UKl%2F+DtvAVS@gmail.com/T/ [1] Link: https://lore.kernel.org/rust-for-linux/20231119180145.157455-1-sergio.collad... [2]
Cheers, Miguel
On Wed, Feb 21, 2024 at 10:00 AM Valentin Obst kernel@valentinobst.de wrote:
While debugging the `X86_DECODER_SELFTEST` failure first reported in [1], [In this case the line causing the failure is interpreted as two lines by the tool (due to its length, but this is fixed by [1, 2]), and the second line is reported. Still the spatial closeness between the reported line and the line causing the failure would have made debugging a lot easier.]
Thanks Valentin, John et al. for digging into this (and the related issue) -- very much appreciated.
It looks good to me:
Reviewed-by: Miguel Ojeda ojeda@kernel.org Tested-by: Miguel Ojeda ojeda@kernel.org
Thanks!
This should probably have a Fixes tag -- from a quick look, the original test did not seem to have the problem because `insns` was equivalent to the number of lines since there was no `if ... { continue; }` for the symbol case. At some point that branch was added, so that was not true anymore, thus that one should probably be the Fixes tag, though please double-check:
Fixes: 35039eb6b199 ("x86: Show symbol name if insn decoder test failed")
Cross checked this as well, can confirm your assessment. Thanks for bringing this up.
It is a minor issue for sure, so perhaps not worth backporting, but nevertheless the hash is in a very old kernel, and thus the issue applies to all stable kernels. So it does not hurt flagging it to the stable team:
Cc: stable@vger.kernel.orgIn addition, John reported the original issue, but this one was found due to that one, and I am not exactly sure who did what here. Please consider whether one of the following (or similar) may be fair:
Reported-by: John Baublitz <john.m.baublitz@gmail.com> Debugged-by: John Baublitz <john.m.baublitz@gmail.com>
Absolutely, without him reporting the test failure and narrowing down the config I'd have never looked at this file. Adding him for **both** is fair. (This particular fix was not discussed on Zulip though, its just something I noticed along the way.)
An extra Link to the discussion in Zulip could be nice too:
Link: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/insn_decoder_test.20failure/near/421075039
Didn't add it because the discussion does not mention this particular issue, but it might indeed be good for some context.
Will this need a v2, or are all of the 'Fixes', 'Reported-By', 'Debugged-By', 'Tested-By', 'Reviewed-By' and 'Link' tags something that maintainers may add when merging?
- Best Valentin
Finally, a nit: links are typically written like the following -- you can still use bracket references at the end:
Link: https://lore.kernel.org/lkml/Y9ES4UKl%2F+DtvAVS@gmail.com/T/ [1] Link: https://lore.kernel.org/rust-for-linux/20231119180145.157455-1-sergio.collado@gmail.com/[2]
Cheers, Miguel
On Wed, Feb 21, 2024 at 10:51 PM Valentin Obst kernel@valentinobst.de wrote:
Thanks!
Cross checked this as well, can confirm your assessment. Thanks for bringing this up.
My pleasure!
Absolutely, without him reporting the test failure and narrowing down the config I'd have never looked at this file. Adding him for **both** is fair. (This particular fix was not discussed on Zulip though, its just something I noticed along the way.)
In that case, up to you -- whatever you consider fair for this particular patch.
Didn't add it because the discussion does not mention this particular issue, but it might indeed be good for some context.
Makes sense -- I saw the [1] reference and I thought it could be a nice complement to it, but it is true that it may be not that useful, so please feel free to leave it out.
Will this need a v2, or are all of the 'Fixes', 'Reported-By', 'Debugged-By', 'Tested-By', 'Reviewed-By' and 'Link' tags something that maintainers may add when merging?
Typically, tags are picked up by maintainers when they apply the patch (if it is the last version, otherwise you would already pick them up in the next version you send).
However, in this case, since we have the Cc stable@ and you also have the most context to decide on the tags (e.g. for the Reported-by etc.), I would send a v2.
Thanks!
Cheers, Miguel
linux-stable-mirror@lists.linaro.org
-
Miguel Ojeda -
Valentin Obst