On Fri Feb 13, 2026 at 10:48 PM CET, M Henning wrote: > On Fri, Feb 13, 2026 at 12:12 PM Danilo Krummrich <[email protected]> wrote: >> >> On Thu Feb 5, 2026 at 7:56 PM CET, Mel Henning wrote: >> > This information will be exposed to userspace in the following commit. >> > >> > Signed-off-by: Mel Henning <[email protected]> >> >> For someone looking at this commit, this commit message is not very useful. >> >> Please add at least a brief explanation of what the patch does and - even >> more >> important - why it does it. See also [1]. >> >> [1] >> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > > What I'm struggling with is that I don't know how to do this without > repeating myself. If you want, I can copy-paste my explanation of > zcull here too and then it will appear three times, once in each > commit and once in the cover letter. But that kind of repetition > doesn't seem very helpful to me.
Again, the commit message should explain what the commit does and why. For instance, I asked you why you did combine those two callbacks below. The commit message could mention this, e.g. it could be something along the lines of: "Add struct nvkm_gr_zcull_info, which serves as abstraction layer between the corresponding uAPI (added in a subsequent patch) and the firmware (version specific) structure. This is needed in order to not leak the uAPI layer into nvkm. Also note that we are bypassing the nvif layer, since ... Also note that we reuse the get_ctxbufs_info() callback, since ..." I.e. make it obvious to maintainers what's going on and what's the motivation for the patch and it's implementation details. > Because of this, I decided that it was simplest to combine them in a > single call, which avoids repeated rpc calls to the gpu without the > complexity of handling partially complete states. Ok, that seems reasonable. >> > + if (WARN_ON(IS_ERR(zcull_info))) >> >> What justifies this WARN_ON()? To me this seems like normal error handling, >> i.e. >> it is not a violation of some API invariant, etc. Also, this is in the >> driver's >> probe() path. > > I was just copying the error handling that already exists in this function. > > I do think these are weird error cases though - they mean that the gpu > was partially but not fully initialized which shouldn't happen during > normal usage. The only cases I can think of that would trigger this > warning are a kernel bug or an intermittent PCI link, which I think > are both reasonable to warn on. It could also be that the firmware is buggy, etc. In any case, I don't see that a WARN_ON() is justified. Please use dev_err() instead.
