On Tue, Feb 16, 2016 at 03:45:11PM -0600, Aravind Gopalakrishnan wrote: > In an attempt to help folks not very familiar with the code to > understand what the code is doing, adding a bit of helper > comments around some more important functions in the driver > to describe them. > > No functional change is introduced. > > Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrish...@amd.com> > --- > arch/x86/kernel/cpu/mcheck/mce_amd.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c > b/arch/x86/kernel/cpu/mcheck/mce_amd.c > index 4bdc836..d2b6001 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c > +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c > @@ -184,6 +184,11 @@ static int lvt_off_valid(struct threshold_block *b, int > apic, u32 lo, u32 hi) > }; > > /* > + * Set the error_count and interrupt_enable sysfs attributes here. > + * This function gets called during the init phase and when someone > + * makes changes to either of the sysfs attributes. > + * During init phase, we also program Interrupt type as 'APIC' and > + * verify if LVT offset obtained from MCx_MISC is valid. > * Called via smp_call_function_single(), must be called with correct > * cpu affinity. > */
I don't think that's what threshold_restart_bank() does... Also, that comment is too much - it shouldn't explain "what" but "why". > @@ -262,6 +267,11 @@ static int setup_APIC_deferred_error(int reserved, int > new) > return reserved; > } > > +/* > + * Obtain LVT offset from MSR_CU_DEF_ERR and call > + * setup_APIC_deferred_error() to program relevant APIC register. > + * Also, register a deferred error interrupt handler > + */ No, that's basically spelling what the code does. > static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c) > { > u32 low = 0, high = 0; > @@ -338,6 +348,14 @@ nextaddr_out: > return addr; > } > > +/* > + * struct threshold_block descriptor tracks useful info regarding the > + * banks' MISC register. Among other things, it tracks whether interrupt > + * is possible for the given bank, the threshold limit and the sysfs object > + * that outputs these info. That should be in form of comments explaining what the members of struct threshold_block are, where that struct is defined. > Initializing the struct here, programming > + * LVT offset for threshold interrupts and registering a interrupt handler > + * if we haven't already done so Also spelling the code. > + */ > static int > prepare_threshold_block(unsigned int bank, unsigned int block, u32 addr, > int offset, u32 misc_high) -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.