lkml.org 
[lkml]   [2018]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access
Date


> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Thursday, April 26, 2018 2:49 PM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: x86@kernel.org; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com; hpa@zytor.com; Stephen
> Hemminger <sthemmin@microsoft.com>; Michael Kelley (EOSG)
> <Michael.H.Kelley@microsoft.com>; vkuznets@redhat.com
> Subject: Re: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access
>
> On Wed, 25 Apr 2018, kys@linuxonhyperv.com 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@microsoft.com>
>
> 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

\
 
 \ /
  Last update: 2018-04-27 07:53    [W:0.371 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site