Re: [PATCH 12/21] clk: sunxi: clk-sun6i-ar100: Demote non-conformant kernel-doc header

[Lee Jones]

On Wed, 03 Feb 2021, Maxime Ripard wrote:

> On Tue, Jan 26, 2021 at 04:54:59PM +0000, Lee Jones wrote:
> > On Tue, 26 Jan 2021, Maxime Ripard wrote:
> > 
> > > On Tue, Jan 26, 2021 at 12:45:31PM +0000, Lee Jones wrote:
> > > > Fixes the following W=1 kernel build warning(s):
> > > > 
> > > >  drivers/clk/sunxi/clk-sun6i-ar100.c:26: warning: Function parameter or member 'req' not described in 'sun6i_get_ar100_factors'
> > > > 
> > > > Cc: "Emilio López" <emilio@xxxxxxxxxxxxx>
> > > > Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
> > > > Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> > > > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > > > Cc: Chen-Yu Tsai <wens@xxxxxxxx>
> > > > Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > > > Cc: Boris BREZILLON <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > > > Cc: linux-clk@xxxxxxxxxxxxxxx
> > > > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > > > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> > > > ---
> > > >  drivers/clk/sunxi/clk-sun6i-ar100.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/clk/sunxi/clk-sun6i-ar100.c b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > > index e1b7d0929cf7f..54babc2b4b9ee 100644
> > > > --- a/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > > +++ b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > > @@ -16,7 +16,7 @@
> > > >  
> > > >  #include "clk-factors.h"
> > > >  
> > > > -/**
> > > > +/*
> > > >   * sun6i_get_ar100_factors - Calculates factors p, m for AR100
> > > >   *
> > > >   * AR100 rate is calculated as follows
> > > 
> > > This is the sixth patch doing the exact same thing over the files in
> > > that folder you sent. Please fix all the occurences at once
> > 
> > No.  That would make the whole clean-up process 10x harder than it
> > already is
> > 
> > Before starting this endeavour there were 18,000+ warnings spread over
> > 100's of files and 10's of subsystems that needed addressing (only a
> > couple thousand left now thankfully).  Some issues vastly different,
> > some duplicated (much too much copy/pasting going which made things
> > very frustrating at times).
> > 
> > Anyway, in order to work though them all gracefully and in a sensible
> > time-frame I had to come up with a workable plan.  Each subsystem is
> > compiled separately and a script attempts to take out duplicate
> > warnings and takes me through the build-log one file at a time.  Once
> > all of the warnings are fixed in a source-file, it moves on to the
> > next file.  The method is clean and allows me to handle this
> > gargantuan task in bite-sized chunks.
> 
> I mean, you have literally used the same commit log and the same changes
> over six different files in the same directory.

Yes, that happens.  It's an unfortunate side-effect of the same ol'
issues repeating themselves over and over.  Mostly due to copy/paste
of mundane code segments such as function documentation.

> Sure changes across
> different parts of the kernel can be painful, but it's really not what
> we're discussing here.

It would have even been painful to post-process patches within the
same subsystem.  For instance, I've just finished cleaning up GPU
which was a mammoth task where most of the issues were perpetually
duplicated.

I will admit though, that here in Clock, it would be somewhat easier.

> > Going though and pairing up similar changes is unsustainable for a
> > task like this.  It would add a lot of additional overhead and would
> > slow down the rate of acceptance since source files tend to have
> > different reviewers/maintainers - some working faster to review
> > patches than others, leading to excessive lag times waiting for that
> > one reviewer who takes weeks to review.
> 
> Are you arguing that sending the same patch 6 times is easier and faster
> to review for the maintainer than the same changes in a single patch?

The issue I see with the Clock, is that some files are maintained by
individual driver Maintainers and others by subsystem Maintainers.  So
the post-process here is that much more painful (as it can't be
easily scripted using get_maintainer.pl) and the aforementioned
lag-time issues come into play while we wait for sleepy reviewers.

> > Having each file addressed in a separate patch also helps
> > revertability and bisectability. Not such a big problem with the
> > documentation patches, but still.
> 
> There's nothing to revert or bisect, those changes aren't functional
> changes.

Right, I did mention that.

> > Admittedly doing it this way *can* look a bit odd in *some* patch-sets
> > when they hit the MLs - particularly clock it seems, where there
> > hasn't even been a vague attempt to document any of the parameters in
> > the kernel-doc headers - however the alternative would mean nothing
> > would get done!
> 
> Yeah, and even though properly documenting the functions would have been
> the right way to fix those warnings, I didn't ask you to do that since I
> was expecting it to be daunting.

There are a couple of schools of thought on function documentation.
The conflicting one to yours is that Kernel-doc headers should only be
used if they are part of an API and have an accompanying kernel-doc::
tag in Documentation.  The functions touched here do not.

NB: Fortunately the functions we're discussing are all static or else
`scripts/find-unused-docs.sh` would complain about them also.

Personally, I am in the middle.  If authors have had a good go at
documenting functions and their parameters, I'll make the effort to
fix any doc-rot or oversights.  However if, like here, no such effort
has been made, they get demoted.  Nothing stopping authors fixing them
up properly and re-promoting them again though.  Essentially I'm
trying to avoid a situation where authors throw something together
half-heatedly, safe in the knowledge that someone will come fix and
beautify things for them.

> Surely we can meet half-way

I'm always happy to collaborate.  What does half-way look like?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog