RE: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access

[KY Srinivasan]

> -----Original Message-----
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Thursday, April 26, 2018 2:49 PM
> To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> Cc: x86@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx;
> apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx; hpa@xxxxxxxxx; Stephen
> Hemminger <sthemmin@xxxxxxxxxxxxx>; Michael Kelley (EOSG)
> <Michael.H.Kelley@xxxxxxxxxxxxx>; vkuznets@xxxxxxxxxx
> Subject: Re: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access
> 
> On Wed, 25 Apr 2018, kys@xxxxxxxxxxxxxxxxx wrote:
> > --- /dev/null
> > +++ b/arch/x86/hyperv/hv_apic.c
> > @@ -0,0 +1,98 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> Thanks for putting the license identifier in.
> 
> > +
> > +/*
> > + * Hyper-V specific APIC code.
> > + *
> > + * Copyright (C) 2018, Microsoft, Inc.
> > + *
> > + * Author : K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> 
> But can you please check with your lawyers whether you can avoid the
> pointless boilerplate? The SPDX identifier should cover it.

I will consult with MSFT legal on this.
> 
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as
> published
> > + * by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD
> TITLE or
> > + * NON INFRINGEMENT.  See the GNU General Public License for more
> > + * details.
> > + *
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <asm/hypervisor.h>
> > +#include <asm/mshyperv.h>
> > +#include <linux/version.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/mm.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/hyperv.h>
> > +#include <linux/slab.h>
> > +#include <linux/cpuhotplug.h>
> 
> We usually order the includes
> 
> #include <linux/....>
> ...
> #include <linux/....>
> 
> #include <asm/....>
> #include <asm/....>
> 
> 
> > -void hyperv_init(void);
> > +void __init hyperv_init(void);
> >  void hyperv_setup_mmu_ops(void);
> >  void hyper_alloc_mmu(void);
> >  void hyperv_report_panic(struct pt_regs *regs, long err);
> > @@ -269,14 +269,16 @@ void hyperv_reenlightenment_intr(struct pt_regs
> *regs);
> >  void set_hv_tscchange_cb(void (*cb)(void));
> >  void clear_hv_tscchange_cb(void);
> >  void hyperv_stop_tsc_emulation(void);
> > +void hv_apic_init(void);
> >  #else /* CONFIG_HYPERV */
> > -static inline void hyperv_init(void) {}
> > +static __init  inline void hyperv_init(void) {}
> 
> The __init on the empty inline function is pointless.
> 
> Other than the few nits. This looks well done!

Thanks Thomas. I will address all the issues you have brought up in
the next version.

Regards,

K. Y