| Message ID | 20200714145049.2496163-4-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rid W=1 warnings in CPUFreq | expand |
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
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 >
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.
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?
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.
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.
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
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(-)