Message-ID: <CAKXUXMxSnEEYCtvxVN6Z_QuDTEpLiq=Zsz+=g=kNzwKuLeH=Pg@mail.gmail.com>
Date: Tue, 27 Feb 2024 09:04:48 +0100
From: Lukas Bulwahn <lukas.bulwahn@...il.com>
To: Randy Dunlap <rdunlap@...radead.org>
Cc: Jonathan Corbet <corbet@....net>, workflows@...r.kernel.org, linux-doc@...r.kernel.org, 
	kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] docs: submit-checklist: structure by category

On Tue, Feb 27, 2024 at 1:41 AM Randy Dunlap <rdunlap@...radead.org> 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@...il.com>
> > ---
> > 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

Powered by blists - more mailing lists