On Tue, Feb 27, 2024 at 1:41 AM Randy Dunlap <rdunlap@xxxxxxxxxxxxx> wrote: > > Hi Lukas, > > I'll review the file changes separately. This is just replying > to the patch description comments. > > > On 2/26/24 02:46, Lukas Bulwahn wrote: (snipped) > > > Note that the previous first point still remains the first list even after > > reordering. Based on some vague memory, the first point was important to > > Randy to stay the first one in any reordering. > > Yes, I have said that. Stephen Rothwell wanted it to be first in the list. > I have adjusted my commit message: Note that the previous first point still remains the first list even after reordering. Randy confirmed that it was important to Stephen Rothwell to keep 'include what you use' to be the first in the list. > > > While at it, the reference to CONFIG_SLUB_DEBUG was replaced by > > CONFIG_DEBUG_SLAB. > > I don't understand this comment. DEBUG_SLAB is gone. > I think those 2 symbols might be reversed in your comments. ? > I must have mixed them up while writing down the commit message; so now it reads: While at it, replace the reference to the obsolete CONFIG_DEBUG_SLAB with CONFIG_SLUB_DEBUG. That is what is happening in the file. > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx> > > --- > > So far, no point disappeared and nothing new was added. > > > > That's a good start IMO. > > > Points/Ideas for further improvements (based on my knowledge and judgement): > > > > - The Review Kconfig changes makes sense, but I am not sure if they are > > so central during review. If we keep it, let us see if there are other > > parts for review that are also important similar to Kconfig changes. > > > > - Concerning checking with tools, checkpatch probably still makes sense; > > it pointed out in several places. If sparse and checkstack are really > > the next two tools to point out, I am not so sure about. > > I doubt that ckeckstack is important since gcc & clang warn us about > stack usage. > So, I might drop this later on and replace it with something more important to ask. I have put it on my todo list (but others are welcome as well to pick it up): KTODO: Investigate if the make checkstack tool is really obsolete, as gcc and clang are already set up to warn about large stack usage just as well as make checkstack does. Present how it was investigated and which kind of "benchmark" was set up and how the results were evaluated. If make checkstack is really obsolete, create a patch to remove the tool from the repository, and add some documentation to explain how kernel developers can check for large stack usage. > > sparse has a lot of false positives nowadays, and many things are not > > fixed just because sparse complains about it. > > And I have never used make checkstack and have not found much > > documentation about it. > > So, maybe other tools deserve to be mentioned here instead? > > If make checkstack is removed from the list, this might give rise to another linting/static analysis tool worth mentioning. The candidates that come to my mind are clang-tidy or smatch. I need to check, though, if the installation guides for those tools and the setup for the kernel sources are clear enough to actually promote running these tools. But maybe there is another tool worth mentioning. I know about the coverity setup, but this is not really suitable for checking individual patches. Randy, I will wait for your review and feedback on the file changes and then send out a v2 patch. So far, the changes are only changes to the commit message. Lukas
