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

This patch adds some in-code documentation to the DSCR related
code to make it more readable without having any functional
change to it.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/processor.h |  8 ++++++++
 arch/powerpc/kernel/sysfs.c          | 13 +++++++++++++
 2 files changed, 21 insertions(+)

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.

> 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?

> +/*
> + * 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:

$ ./scripts/kernel-doc -text arch/powerpc/kernel/sysfs.c

The comments for write_dscr() should be attached to that function.

cheers

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.