Re: [4/5] powerpc, dscr: Added some in-code documentation

[Anshuman Khandual <khandual@linux.vnet.ibm.com>]

On 12/09/2014 03:33 PM, Michael Ellerman wrote:
> On Mon, 2014-08-12 at 06:30:11 UTC, Anshuman Khandual wrote:
>> This patch adds some in-code documentation to the DSCR related
>> code to make it more readable without having any functional
>> change to it.
> 
> Adding documentation is always good, but ...
> 
>> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
>> index dda7ac4..81c1aeb 100644
>> --- a/arch/powerpc/include/asm/processor.h
>> +++ b/arch/powerpc/include/asm/processor.h
>> @@ -295,6 +295,14 @@ struct thread_struct {
>>  #endif
>>  #ifdef CONFIG_PPC64
>>  	unsigned long	dscr;
>> +	/*
>> +	 * XXX: dscr_inherit indicates that the process has explicitly
> 
> Please don't use XXX as a matter of practice.
> 
> It should be saved for *really* tricky/complicated code, and this isn't that.

Sure, got it. Will remove them.

> 
>> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
>> index 67fd2fd..edde3f0 100644
>> --- a/arch/powerpc/kernel/sysfs.c
>> +++ b/arch/powerpc/kernel/sysfs.c
>> @@ -496,8 +496,21 @@ static DEVICE_ATTR(spurr, 0400, show_spurr, NULL);
>>  static DEVICE_ATTR(purr, 0400, show_purr, store_purr);
>>  static DEVICE_ATTR(pir, 0400, show_pir, NULL);
>>  
>> +/*
>> + * XXX: This is the system wide DSCR register default value.
>> + * Any change to this value through the sysfs interface will
>> + * update all per-cpu DSCR default values across the system
>> + * stored in their respective PACA structures.
>> + */
>>  static unsigned long dscr_default;
> 
> Yeah it seems you're right, writing updates the values in all pacas, reading
> returns the value in the current cpu's paca. So why do we need this copy of the
> value?

My comment here might be little confusing. The read_dscr/write_dscr functions
are used for per-CPU PACA DSCR values which can read/update the per-CPU PACA
variable directly. The functions show_dscr_default/store_dscr_default are used
to read/update the system wide DSCR default which is the above 'dscr_default'
variable. Function store_dscr_default also calls write_dscr to update PACA on
every CPU present on the system. I will re-write the code comment to make more
sense.

> 
>> +/*
>> + * XXX: read_dscr and write_dscr are the functions for the
>> + * per-cpu DSCR default sysfs files present for each cpu.
>> + * Though updates to per-cpu DSCR value also gets called
>> + * for all the CPUs on the system when the system wide
>> + * global dscr_default gets changed.
>> + */
>>  static void read_dscr(void *val)
>>  {
> 
> Please make these proper kernel-doc comments. I've definitely asked you to do
> that at least once before on a different patch, to check you can do:

Yes you had. Thought that this function is too small but as you said if we
are writing documentation for it we should write kernel-doc format only.
Will make sure about this now onward.

> 
> $ ./scripts/kernel-doc -text arch/powerpc/kernel/sysfs.c
> 
> The comments for write_dscr() should be attached to that function.

Sure.