On Fri, Aug 22, 2025 at 08:07:12PM -0700, Marc Herbert wrote: > > > On 2025-08-22 11:13, Alison Schofield wrote: > > On Fri, Aug 22, 2025 at 02:55:34AM +0000, marc.herb...@linux.intel.com > > wrote: > >> From: Marc Herbert <marc.herb...@linux.intel.com> > >> > >> This magic number must match kernel code. Make that corresponding kernel > >> code much less time-consuming to find. > > > > The 'must match' is the important part. Include that in the comment. > > Good point, will do! > > > Why expect the user to parse a git describe string and go fishing. > > Just tell them it is defined in the cxl-test module. > > > > git knows how to parse back the git describe string; readers don't need > to parse anything. As explained in the commit message, it's convenient > because it holds in a single string both the immutable commit ID _and_ > an indication of the minimum kernel version required. > > Why tell users to "go fishing" for the cxl-test module when they can > find the location directly with a single git command. This is real: I > actually wasted a fair amount of time searching for that constant in > drivers/cxl/ because I assumed the cxl-test driver was there. This > comment is not meant for experts; if they needed it then it would not > have been missing for so long. > > Last but not least, code and files move around and get renamed. This > commit will never change, so it provides an immutable starting point in > case things change. > > I'll add both, it should still fit on one line.
Marc, I didn't infer the pain you go on to describe here in the original commit message. I read this as a trivial patch, and even questioned (but didn't comment) usage of 'magic number' language on value that had a define. ICYMI "Describe your problem. Whether your patch is a one-line bug fix or 5000 lines of a new feature, there must be an underlying problem that motivated you to do this work. Convince the reviewer that there is a problem worth fixing and that it makes sense for them to read past the first paragraph." That's from https://www.kernel.org/doc/html/latest/process/submitting-patches.html and ndctl/Contributing.md notes that ndctl follows those guidelines. There's a lot more guidance at that link, but following the basic 1-2-3, emphasis on #2 for this patch, will help me review without missing the point: 1- State current situation 2- State why #1 is lacking, painful, broken, needs enhancing, etc. 3- State how this patch addresses #2. Perhaps: [ndctl PATCH] test/common: document the CXL_TEST_QOS_CLASS kernel dependency 1- The CXL_TEST_QOS_CLASS define lacks documentation about its kernel counterpart, making it difficult to locate the corresponding kernel code when debugging or verifying compatibility. 2- When tests depend on kernel-specific constants, like this one does, the lack of clear documentation leads to tedious manual searching. 3- Add a comment that identifies... May also be worth mentioning that this issue is arising during the testing of kernels with backported features where test failures may be due to missing kernel support rather than actual bugs. If that is indeed what is happening and is part of the pain. I'll watch for v2. --Alison
