Re: [PATCH v20 09/18] x86/resctrl: Add a new field to struct rmid_read for summation of domains

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 6/21/24 9:07 AM, Luck, Tony wrote:
>> I do not think there is a need to introduce new syntax. It will be easiest
>> to just have all sentences end with a period. The benefit of this is that it
>> encourages useful full sentence descriptions. For example, below is a _draft_ of
>> such a description. Please note that I wrote it quickly and hope it will be improved
>> (and corrected!). The goal of it being here is to give ideas on how this kerneldoc
>> can be written to be useful and consistent.
>>
>> /**
>>    * struct rmid_read - Data passed across smp_call*() to read event count
> 
> Should this end with a period too?  In the resctrl code a few cases use ".",
> most don't. So no period matches resctrl style. But the example in
> Documentation/doc-guide/kernel-doc.rst does end with a period.

Having period will be ideal but since that does not match existing style it may
look out of place. I thus do not have strong opinion here.

> 
>>    * @rgrp:  Resource group for which the counter is being read. If it is a parent
>>    *      resource group then its event count is summed with the count from all
>>    *      its child resource groups.
>>    * @r:          Resource describing the properties of the event being read.
>>    * @d:          Domain that the counter should be read from. If NULL then sum all
>>    *      domains in @r sharing L3 @ci.id
>>    * @evtid: Which monitor event to read.
>>    * @first: Initialize MBM counter when true.
>>    * @ci:    Cacheinfo for L3. Only set when @d is NULL. Used when summing domains.
>>    * @err:   Error encountered when reading counter.
>>    * @val:   Returned value of event counter. If @rgrp is a parent resource group,
>>    *      @val contains the sum of event counts from its child resource groups.

contains -> includes (to indicate it contains the count from parent as well as children)

>>    *      If @d is NULL, @val contains the sum of all domains in @r sharing @ci.id,
>>    *      (summed across child resource groups if @rgrp is a parent resource group).
>>    * @arch_mon_ctx: Hardware monitor allocated for this read request (MPAM only).
>>    */
> 
> This all looks good to me.  Since you have supplied 99% of the content for this
> patch in the series I should assign authorship to you (which requires your
> Signed-off-by tag). Is that OK? Should I split into two parts? First to add the
> kerneldoc (by you). Second to add the new field (by me).

No need to split the patch. You can keep authorship. You are welcome to add:

Co-developed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette