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. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev