From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yanfei Zhang Subject: Re: [PATCH v3 4/5] Sysfs: Export VMCSINFO via sysfs Date: Tue, 03 Jul 2012 17:05:19 +0800 Message-ID: <4FF2B5CF.2000707@cn.fujitsu.com> References: <4FEAC945.50700@cn.fujitsu.com> <4FEACA5E.4090009@cn.fujitsu.com> <20120627192236.GB1965@kroah.com> <4FEC29D6.5020109@cn.fujitsu.com> <20120628113738.GA5499@kroah.com> <20120629025848.GC8074@kroah.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Avi Kivity , mtosatti@redhat.com, ebiederm@xmission.com, luto@mit.edu, Joerg Roedel , dzickus@redhat.com, paul.gortmaker@windriver.com, ludwig.nussel@suse.de, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, kexec@lists.infradead.org To: Greg KH Return-path: In-Reply-To: <20120629025848.GC8074@kroah.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org =E4=BA=8E 2012=E5=B9=B406=E6=9C=8829=E6=97=A5 10:58, Greg KH =E5=86=99=E9= =81=93: > On Thu, Jun 28, 2012 at 04:37:38AM -0700, Greg KH wrote: >> On Thu, Jun 28, 2012 at 05:54:30PM +0800, Yanfei Zhang wrote: >>> =E4=BA=8E 2012=E5=B9=B406=E6=9C=8828=E6=97=A5 03:22, Greg KH =E5=86= =99=E9=81=93: >>>> On Wed, Jun 27, 2012 at 04:54:54PM +0800, Yanfei Zhang wrote: >>>>> This patch export offsets of fields via /sys/devices/cpu/vmcs/. >>>>> Individual offsets are contained in subfiles named by the filed's >>>>> encoding, e.g.: /sys/devices/cpu/vmcs/0800 >>>>> >>>>> Signed-off-by: zhangyanfei >>>>> --- >>>>> drivers/base/core.c | 13 +++++++++++++ >>>>> 1 files changed, 13 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c >>>>> index 346be8b..dd05ee7 100644 >>>>> --- a/drivers/base/core.c >>>>> +++ b/drivers/base/core.c >>>>> @@ -26,6 +26,7 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>> >>>> Did you just break the build on all other arches? Not nice. >>>> >>>>> @@ -1038,6 +1039,11 @@ int device_add(struct device *dev) >>>>> error =3D dpm_sysfs_add(dev); >>>>> if (error) >>>>> goto DPMError; >>>>> +#if defined(CONFIG_KVM_INTEL) || defined(CONFIG_KVM_INTEL_MODULE= ) >>>>> + error =3D vmcs_sysfs_add(dev); >>>>> + if (error) >>>>> + goto VMCSError; >>>>> +#endif >>>> >>>> Oh my no, that's no way to ever do this, you know better than that= , >>>> please fix. >>>> >>>> greg k-h >>>> >>> >>> Sorry for my thoughtless, Here is the new patch. >>> >>> --- >>> drivers/base/core.c | 13 +++++++++++++ >>> 1 files changed, 13 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/base/core.c b/drivers/base/core.c >>> index 346be8b..7b5266a 100644 >>> --- a/drivers/base/core.c >>> +++ b/drivers/base/core.c >>> @@ -30,6 +30,13 @@ >>> #include "base.h" >>> #include "power/power.h" >>> =20 >>> +#if defined(CONFIG_KVM_INTEL) || defined(CONFIG_KVM_INTEL_MODULE) >>> +#include >>> +#else >>> +static inline int vmcs_sysfs_add(struct device *dev) { return 0; } >>> +static inline void vmcs_sysfs_remove(struct device *dev) { } >>> +#endif >> >> {sigh} No, again, you know better, don't do this. >=20 > Ok, as others have rightly pointed out, this wasn't the most helpful > review comment, sorry about that. >=20 > In Linux, we don't put ifdefs in .c files, we put them in .h files. = See > many examples of this all over the place. That's my main complaints = the > past two times of this patch. >=20 > But, for this, I would question why you even want / need to do this i= n > the drivers/base/core/ file in the first place. Shouldn't it be in s= ome > arch or cpu specific file instead that already handles the cpu files? >=20 > thanks, >=20 > greg k-h >=20 Many thanks. I have moved the code to my vmcsinfo_intel module. Thanks again for your helpful comment. Thanks Zhang Yanfei