[03/13] cpufreq: cpufreq_governor: Demote store_sampling_rate() header to standard comment block

Message ID 20200714145049.2496163-4-lee.jones@linaro.org
State Superseded
Headers show
Series
  • Rid W=1 warnings in CPUFreq
Related show

Commit Message

Lee Jones July 14, 2020, 2:50 p.m.
There is no need for this to be denoted as kerneldoc.

Fixes the following W=1 kernel build warning(s):

 drivers/cpufreq/cpufreq_governor.c:46: warning: Function parameter or member 'attr_set' not described in 'store_sampling_rate'
 drivers/cpufreq/cpufreq_governor.c:46: warning: Function parameter or member 'buf' not described in 'store_sampling_rate'
 drivers/cpufreq/cpufreq_governor.c:46: warning: Function parameter or member 'count' not described in 'store_sampling_rate'

Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Alexander Clouter <alex@digriz.org.uk>
Signed-off-by: Lee Jones <lee.jones@linaro.org>

---
 drivers/cpufreq/cpufreq_governor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.25.1

Comments

Viresh Kumar July 15, 2020, 2:52 a.m. | #1
On 14-07-20, 15:50, Lee Jones wrote:
> There is no need for this to be denoted as kerneldoc.

> 

> Fixes the following W=1 kernel build warning(s):

> 

>  drivers/cpufreq/cpufreq_governor.c:46: warning: Function parameter or member 'attr_set' not described in 'store_sampling_rate'

>  drivers/cpufreq/cpufreq_governor.c:46: warning: Function parameter or member 'buf' not described in 'store_sampling_rate'

>  drivers/cpufreq/cpufreq_governor.c:46: warning: Function parameter or member 'count' not described in 'store_sampling_rate'

> 

> Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

> Cc: Jun Nakajima <jun.nakajima@intel.com>

> Cc: Alexander Clouter <alex@digriz.org.uk>

> Signed-off-by: Lee Jones <lee.jones@linaro.org>

> ---

>  drivers/cpufreq/cpufreq_governor.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c

> index f99ae45efaea7..63f7c219062b9 100644

> --- a/drivers/cpufreq/cpufreq_governor.c

> +++ b/drivers/cpufreq/cpufreq_governor.c

> @@ -26,7 +26,7 @@ static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);

>  static DEFINE_MUTEX(gov_dbs_data_mutex);

>  

>  /* Common sysfs tunables */

> -/**

> +/*


This is an important routine with good documentation details already
there, though internal to governors and so I would rather keep it.

>   * store_sampling_rate - update sampling rate effective immediately if needed.

>   *

>   * If new rate is smaller than the old, simply updating

> -- 

> 2.25.1


-- 
viresh
Lee Jones July 15, 2020, 6:45 a.m. | #2
On Wed, 15 Jul 2020, Viresh Kumar wrote:

> On 14-07-20, 15:50, Lee Jones wrote:

> > There is no need for this to be denoted as kerneldoc.

> > 

> > Fixes the following W=1 kernel build warning(s):

> > 

> >  drivers/cpufreq/cpufreq_governor.c:46: warning: Function parameter or member 'attr_set' not described in 'store_sampling_rate'

> >  drivers/cpufreq/cpufreq_governor.c:46: warning: Function parameter or member 'buf' not described in 'store_sampling_rate'

> >  drivers/cpufreq/cpufreq_governor.c:46: warning: Function parameter or member 'count' not described in 'store_sampling_rate'

> > 

> > Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

> > Cc: Jun Nakajima <jun.nakajima@intel.com>

> > Cc: Alexander Clouter <alex@digriz.org.uk>

> > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > ---

> >  drivers/cpufreq/cpufreq_governor.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c

> > index f99ae45efaea7..63f7c219062b9 100644

> > --- a/drivers/cpufreq/cpufreq_governor.c

> > +++ b/drivers/cpufreq/cpufreq_governor.c

> > @@ -26,7 +26,7 @@ static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);

> >  static DEFINE_MUTEX(gov_dbs_data_mutex);

> >  

> >  /* Common sysfs tunables */

> > -/**

> > +/*

> 

> This is an important routine with good documentation details already

> there, though internal to governors and so I would rather keep it.


It maybe documented, but it isn't kerneldoc, for 2 reasons; a) it
doesn't meet the standards required qualify as kerneldoc i.e. it's
missing descriptions for each of the function parameters, which is why
the kerneldoc checker is complaining about it and b) it is not
referenced by any *.rst file:

 git grep kernel-doc::.*cpufreq_governor.c
 /* no results */

So what is the justification for keeping it as kerneldoc?

> >   * store_sampling_rate - update sampling rate effective immediately if needed.

> >   *

> >   * If new rate is smaller than the old, simply updating

> 


-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Viresh Kumar July 15, 2020, 7:08 a.m. | #3
On 15-07-20, 07:45, Lee Jones wrote:
> On Wed, 15 Jul 2020, Viresh Kumar wrote:

> > On 14-07-20, 15:50, Lee Jones wrote:

> > > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c

> > > index f99ae45efaea7..63f7c219062b9 100644

> > > --- a/drivers/cpufreq/cpufreq_governor.c

> > > +++ b/drivers/cpufreq/cpufreq_governor.c

> > > @@ -26,7 +26,7 @@ static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);

> > >  static DEFINE_MUTEX(gov_dbs_data_mutex);

> > >  

> > >  /* Common sysfs tunables */

> > > -/**

> > > +/*

> > 

> > This is an important routine with good documentation details already

> > there, though internal to governors and so I would rather keep it.

> 

> It maybe documented, but it isn't kerneldoc, for 2 reasons; a) it

> doesn't meet the standards required qualify as kerneldoc i.e. it's

> missing descriptions for each of the function parameters, which is why

> the kerneldoc checker is complaining about it


Right, so this is a mistake and not intentional probably.

> and b) it is not

> referenced by any *.rst file:

> 

>  git grep kernel-doc::.*cpufreq_governor.c

>  /* no results */


I believed (and it may be wrong) that there are two categories of
routines/structures which can be put in kernel documentation, the
exported ones and the internal ones which are important and are very
useful in understanding the algorithms/logic in the drivers.

I did try to go and look into Documentation/doc-guide/ but couldn't
find any details on this.

You said that it needs to be referenced from some *.rst file, but why
is that necessary ? What if people don't add any documentation in
Documentation/ for their framework or driver but still want stuff to
appear in kernel-doc as they can keep the documentation in comments
more up to date.

-- 
viresh
Lee Jones July 15, 2020, 7:31 a.m. | #4
On Wed, 15 Jul 2020, Viresh Kumar wrote:

> On 15-07-20, 07:45, Lee Jones wrote:

> > On Wed, 15 Jul 2020, Viresh Kumar wrote:

> > > On 14-07-20, 15:50, Lee Jones wrote:

> > > > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c

> > > > index f99ae45efaea7..63f7c219062b9 100644

> > > > --- a/drivers/cpufreq/cpufreq_governor.c

> > > > +++ b/drivers/cpufreq/cpufreq_governor.c

> > > > @@ -26,7 +26,7 @@ static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);

> > > >  static DEFINE_MUTEX(gov_dbs_data_mutex);

> > > >  

> > > >  /* Common sysfs tunables */

> > > > -/**

> > > > +/*

> > > 

> > > This is an important routine with good documentation details already

> > > there, though internal to governors and so I would rather keep it.

> > 

> > It maybe documented, but it isn't kerneldoc, for 2 reasons; a) it

> > doesn't meet the standards required qualify as kerneldoc i.e. it's

> > missing descriptions for each of the function parameters, which is why

> > the kerneldoc checker is complaining about it

> 

> Right, so this is a mistake and not intentional probably.

> 

> > and b) it is not

> > referenced by any *.rst file:

> > 

> >  git grep kernel-doc::.*cpufreq_governor.c

> >  /* no results */

> 

> I believed (and it may be wrong) that there are two categories of

> routines/structures which can be put in kernel documentation, the

> exported ones and the internal ones which are important and are very

> useful in understanding the algorithms/logic in the drivers.

> 

> I did try to go and look into Documentation/doc-guide/ but couldn't

> find any details on this.

> 

> You said that it needs to be referenced from some *.rst file, but why

> is that necessary ? What if people don't add any documentation in

> Documentation/ for their framework or driver but still want stuff to

> appear in kernel-doc as they can keep the documentation in comments

> more up to date.


I'm not sure what you mean.  Kerneldoc headers are designed to be
extracted and converted into mediums which are easy to read/browse.
For example, see the online documentation for 'debug_object_init':

 https://www.kernel.org/doc/html/latest/core-api/debug-objects.html?highlight=debug_object_init#c.debug_object_init

They are generally meant to be referenced/consumed.  There is even a
script provided inside the kernel to find offending instances where
kerneldoc headers are provided, but not *yet* referenced:

 `scripts/find-unused-docs.sh`

HINT: There are many.

There *could* be and argument to use kerneldoc *just* so you can use
the kerneldoc checker `scripts/kernel-doc` (which is invoked by W=1
builds), in order to ensure the parameter descriptions are kept in
check.

However, in this case, there are no descriptions provided.  So, in
reference to my previous question, what are your reasons for wanting
to keep kerneldoc here?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Viresh Kumar July 15, 2020, 8:02 a.m. | #5
On 15-07-20, 08:31, Lee Jones wrote:
> I'm not sure what you mean.  Kerneldoc headers are designed to be

> extracted and converted into mediums which are easy to read/browse.

> For example, see the online documentation for 'debug_object_init':

> 

>  https://www.kernel.org/doc/html/latest/core-api/debug-objects.html?highlight=debug_object_init#c.debug_object_init

> 

> They are generally meant to be referenced/consumed.  There is even a

> script provided inside the kernel to find offending instances where

> kerneldoc headers are provided, but not *yet* referenced:

> 

>  `scripts/find-unused-docs.sh`

> 

> HINT: There are many.

> 

> There *could* be and argument to use kerneldoc *just* so you can use

> the kerneldoc checker `scripts/kernel-doc` (which is invoked by W=1

> builds), in order to ensure the parameter descriptions are kept in

> check.

> 

> However, in this case, there are no descriptions provided.  So, in

> reference to my previous question, what are your reasons for wanting

> to keep kerneldoc here?


I think the code did the right thing by keeping them as kernel doc
type comments. What we missed then is getting them used in the *.rst
documentation.

A simple way of doing that could be just adding this to the cpu-freq
rst file, like:

-------------------------8<-------------------------
Here are the bits from the in-source documentation:

.. kernel-doc:: include/linux/cpufreq.h
.. kernel-doc:: drivers/cpufreq/cpufreq.c
.. kernel-doc:: drivers/cpufreq/freq_table.c
.. kernel-doc:: drivers/cpufreq/cpufreq_governor.c
-------------------------8<-------------------------

This will make the script stop complaining about these. But the layout
of things wont' be very nice right now.

-- 
viresh
Lee Jones July 15, 2020, 8:15 a.m. | #6
On Wed, 15 Jul 2020, Viresh Kumar wrote:

> On 15-07-20, 08:31, Lee Jones wrote:

> > I'm not sure what you mean.  Kerneldoc headers are designed to be

> > extracted and converted into mediums which are easy to read/browse.

> > For example, see the online documentation for 'debug_object_init':

> > 

> >  https://www.kernel.org/doc/html/latest/core-api/debug-objects.html?highlight=debug_object_init#c.debug_object_init

> > 

> > They are generally meant to be referenced/consumed.  There is even a

> > script provided inside the kernel to find offending instances where

> > kerneldoc headers are provided, but not *yet* referenced:

> > 

> >  `scripts/find-unused-docs.sh`

> > 

> > HINT: There are many.

> > 

> > There *could* be and argument to use kerneldoc *just* so you can use

> > the kerneldoc checker `scripts/kernel-doc` (which is invoked by W=1

> > builds), in order to ensure the parameter descriptions are kept in

> > check.

> > 

> > However, in this case, there are no descriptions provided.  So, in

> > reference to my previous question, what are your reasons for wanting

> > to keep kerneldoc here?

> 

> I think the code did the right thing by keeping them as kernel doc

> type comments. What we missed then is getting them used in the *.rst

> documentation.

> 

> A simple way of doing that could be just adding this to the cpu-freq

> rst file, like:

> 

> -------------------------8<-------------------------

> Here are the bits from the in-source documentation:

> 

> .. kernel-doc:: include/linux/cpufreq.h

> .. kernel-doc:: drivers/cpufreq/cpufreq.c

> .. kernel-doc:: drivers/cpufreq/freq_table.c

> .. kernel-doc:: drivers/cpufreq/cpufreq_governor.c

> -------------------------8<-------------------------

> 

> This will make the script stop complaining about these.


This will stop `scripts/find-unused-docs.sh` from mentioning these
files as an offender, but `scripts/kernel-doc` and by extension W=1
builds (which is the point of this patch-set) will still complain.

Before you add the lines above, you need to provide descriptions for
each of the function parameters or else they will not reach the
required standards expected of kerneldoc.

My suggestion would be to take this (and the other) patch and
subsequently provide your own set i) providing the required parameter
descriptions ii) re-promoting the comment blocks to kerneldoc and iii)
adding the aforementioned lines to the *.rst file(s).

> But the layout of things wont' be very nice right now.


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

Patch

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index f99ae45efaea7..63f7c219062b9 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -26,7 +26,7 @@  static DEFINE_PER_CPU(struct cpu_dbs_info, cpu_dbs);
 static DEFINE_MUTEX(gov_dbs_data_mutex);
 
 /* Common sysfs tunables */
-/**
+/*
  * store_sampling_rate - update sampling rate effective immediately if needed.
  *
  * If new rate is smaller than the old, simply updating