Messages in this thread | | | Subject | Re: [PATCH] x86/resctrl: Fix kernel-doc in internal.h | From | Reinette Chatre <> | Date | Fri, 11 Jun 2021 16:17:10 -0700 |
| |
Hi Fabio,
Please also consider my comments regarding the goal of this patch similar to what I mentioned in my response to your changes to the pseudo_lock.c file. I updated a few descriptions to improve accuracy and noted some formatting issues. Apart from these small issues it is looking good, thank you.
On 6/8/2021 5:54 PM, Fabio M. De Francesco wrote: > Added description of undocumented parameters. Fixed some minor > kernel-doc grammar issues. Issues largely detected by > scripts/kernel-doc. > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > arch/x86/kernel/cpu/resctrl/internal.h | 28 ++++++++++++++++---------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index c4d320d02fd5..f360944a7ae1 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -68,8 +68,9 @@ 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 > + * @evtid: Event id > + * @name: Name of the event > + * @list: List head
The only kernel-doc issue here is the missing @list. To just fix the issue while remaining consistent with the existing formatting you could continue by describing @list with lower case even if other areas do so with upper case.
For that description could you please use more descriptive language - writing something like "List head" does not help the reader. Something like:
"@list: list entry in &rdt_resource->evt_list"
> */ > struct mon_evt { > u32 evtid; > @@ -78,10 +79,12 @@ 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: Private data for the union > + * @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 > */
Spacing got broken here with some unintended tabs added as well as trailing space.
This is a union where @priv and @u refers to the same storage. More detail can be added to help the reader: "@priv: used to store monitoring event data in @u as kernfs private data"
> union mon_data_bits { > void *priv; > @@ -119,6 +122,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 +146,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 > * @parent: parent rdtgrp > * @crdtgrp_list: child rdtgroup node list > * @rmid: rmid for this rdtgroup > @@ -282,11 +286,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;
Above changes look good, thanks.
> @@ -450,18 +454,20 @@ 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 > - * @fflags: flags to choose base and info files > + * @mbm_width: Width of memory bandwidth monitoring counter > + * @fflags: Flags to choose base and info files > */ > struct rdt_resource { > int rid; >
I think one small addition would be helpful to the reader: "@mbm_width: Width of memory bandwidth monitoring hardware counter"
Thank you!
Reinette
| |