Re: [PATCH v2] x86/resctrl: Fix kernel-doc in internal.h

[Reinette Chatre]

Hi Fabio,

On 6/14/2021 8:44 AM, Fabio M. De Francesco wrote:
> Add description of undocumented parameters. Issues detected by
> scripts/kernel-doc.
>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> ---
>
> v1->v2: According to a first review by Reinette Chartre, remove changes
> unrelated to the subject of this patch and modify the descriptions of
> two parameters.
>   
>   arch/x86/kernel/cpu/resctrl/internal.h | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index c4d320d02fd5..ac691af0174b 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -70,6 +70,7 @@ DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
>    * struct mon_evt - Entry in the event list of a resource
>    * @evtid:		event id
>    * @name:		name of the event
> + * @list:		entry in &rdt_resource->evt_list
>    */
>   struct mon_evt {
>   	u32			evtid;
> @@ -78,10 +79,13 @@ struct mon_evt {
>   };
>   
>   /**
> - * struct mon_data_bits - Monitoring details for each event file
> - * @rid:               Resource id associated with the event file.
> + * union mon_data_bits - Monitoring details for each event file
> + * @priv:	       Used to store monitoring event data in @u
> + * 		       as kernfs private data
> + * @rid:               Resource id associated with the event file
>    * @evtid:             Event id associated with the event file
>    * @domid:             The domain to which the event file belongs
> + * @u:		       Name of the bit fields struct
>    */

This snippet is whitespace damaged. Your changes add tabs as well as 
spaces while the existing code uses just spaces. Please follow existing 
style of this area and just use spaces. As a note for any future 
changes, in one line you add spaces before tabs, that is generally not 
the right formatting in kernel-doc - running scripts/checkpatch.pl on 
this patch would also warn about this.

>   union mon_data_bits {
>   	void *priv;
> @@ -119,6 +123,7 @@ enum rdt_group_type {
>    * @RDT_MODE_PSEUDO_LOCKSETUP: Resource group will be used for Pseudo-Locking
>    * @RDT_MODE_PSEUDO_LOCKED: No sharing of this resource group's allocations
>    *                          allowed AND the allocations are Cache Pseudo-Locked
> + * @RDT_NUM_MODES: Total number of modes
>    *
>    * The mode of a resource group enables control over the allowed overlap
>    * between allocations associated with different resource groups (classes
> @@ -142,7 +147,7 @@ enum rdtgrp_mode {
>   
>   /**
>    * struct mongroup - store mon group's data in resctrl fs.
> - * @mon_data_kn		kernlfs node for the mon_data directory
> + * @mon_data_kn:		kernlfs node for the mon_data directory

Sorry I did not notice this before, could you please also fix the typo 
kernlfs -> kernfs ?

>    * @parent:			parent rdtgrp
>    * @crdtgrp_list:		child rdtgroup node list
>    * @rmid:			rmid for this rdtgroup
> @@ -282,11 +287,11 @@ struct rftype {
>   /**
>    * struct mbm_state - status for each MBM counter in each domain
>    * @chunks:	Total data moved (multiply by rdt_group.mon_scale to get bytes)
> - * @prev_msr	Value of IA32_QM_CTR for this RMID last time we read it
> + * @prev_msr:	Value of IA32_QM_CTR for this RMID last time we read it
>    * @prev_bw_msr:Value of previous IA32_QM_CTR for bandwidth counting
> - * @prev_bw	The most recent bandwidth in MBps
> - * @delta_bw	Difference between the current and previous bandwidth
> - * @delta_comp	Indicates whether to compute the delta_bw
> + * @prev_bw:	The most recent bandwidth in MBps
> + * @delta_bw:	Difference between the current and previous bandwidth
> + * @delta_comp:	Indicates whether to compute the delta_bw
>    */
>   struct mbm_state {
>   	u64	chunks;
> @@ -450,17 +455,19 @@ struct rdt_parse_data {
>    * @name:		Name to use in "schemata" file
>    * @num_closid:		Number of CLOSIDs available
>    * @cache_level:	Which cache level defines scope of this resource
> - * @default_ctrl:	Specifies default cache cbm or memory B/W percent.
> + * @default_ctrl:	Specifies default cache cbm or memory B/W percent
>    * @msr_base:		Base MSR address for CBMs
>    * @msr_update:		Function pointer to update QOS MSRs
>    * @data_width:		Character width of data when displaying
>    * @domains:		All domains for this resource
>    * @cache:		Cache allocation related data
> + * @membw:		Memory bandwidth allocation related data
>    * @format_str:		Per resource format string to show domain value
>    * @parse_ctrlval:	Per resource function pointer to parse control values
>    * @evt_list:		List of monitoring events
>    * @num_rmid:		Number of RMIDs available
>    * @mon_scale:		cqm counter * mon_scale = occupancy in bytes
> + * @mbm_width:		Width of memory bandwidth monitoring hardware counter
>    * @fflags:		flags to choose base and info files
>    */

Fixes to membw and mbm_width are also arriving via another patch series 
(see commit 
https://lore.kernel.org/lkml/20210614200941.12383-2-james.morse@xxxxxxx/).
To make it easier to merge that patch and yours could you please inherit 
the descriptions from there?

@mbm_width: Monitor width, to detect and correct for overflow.
@membw: If the component has bandwidth controls, their properties.

Thank you

Reinette