| Message ID | 1596891052-24052-1-git-send-email-chenhc@lemote.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64 | expand |
On Sat, Aug 08, 2020 at 08:50:52PM +0800, Huacai Chen wrote: > Only Loongson64 platform has and needs loongson_regs.h, including it > unconditionally will cause build errors. > > Fixes: 7f2a83f1c2a941ebfee5 ("KVM: MIPS: Add CPUCFG emulation for Loongson-3") > Cc: stable@vger.kernel.org > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Huacai Chen <chenhc@lemote.com> > --- > arch/mips/kvm/vz.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c > index 3932f76..a474578 100644 > --- a/arch/mips/kvm/vz.c > +++ b/arch/mips/kvm/vz.c > @@ -29,7 +29,9 @@ > #include <linux/kvm_host.h> > > #include "interrupt.h" > +#ifdef CONFIG_CPU_LOONGSON64 > #include "loongson_regs.h" > +#endif The fix for this should be in the .h file, no #ifdef should be needed in a .c file. thanks, greg k-h
在 2020/8/8 下午11:31, Greg KH 写道: > On Sat, Aug 08, 2020 at 08:50:52PM +0800, Huacai Chen wrote: >> Only Loongson64 platform has and needs loongson_regs.h, including it >> unconditionally will cause build errors. >> >> Fixes: 7f2a83f1c2a941ebfee5 ("KVM: MIPS: Add CPUCFG emulation for Loongson-3") >> Cc: stable@vger.kernel.org >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Huacai Chen <chenhc@lemote.com> >> --- >> arch/mips/kvm/vz.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c >> index 3932f76..a474578 100644 >> --- a/arch/mips/kvm/vz.c >> +++ b/arch/mips/kvm/vz.c >> @@ -29,7 +29,9 @@ >> #include <linux/kvm_host.h> >> >> #include "interrupt.h" >> +#ifdef CONFIG_CPU_LOONGSON64 >> #include "loongson_regs.h" >> +#endif > The fix for this should be in the .h file, no #ifdef should be needed in > a .c file. The header file can only be reached when CONFIG_CPU_LOONGSON64 is selected... Otherwise the include path of this file won't be a part of CFLAGS. Thanks. - Jiaxun > > thanks, > > greg k-h
On Sat, Aug 08, 2020 at 11:35:54PM +0800, Jiaxun Yang wrote: > > > 在 2020/8/8 下午11:31, Greg KH 写道: > > On Sat, Aug 08, 2020 at 08:50:52PM +0800, Huacai Chen wrote: > > > Only Loongson64 platform has and needs loongson_regs.h, including it > > > unconditionally will cause build errors. > > > > > > Fixes: 7f2a83f1c2a941ebfee5 ("KVM: MIPS: Add CPUCFG emulation for Loongson-3") > > > Cc: stable@vger.kernel.org > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Huacai Chen <chenhc@lemote.com> > > > --- > > > arch/mips/kvm/vz.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c > > > index 3932f76..a474578 100644 > > > --- a/arch/mips/kvm/vz.c > > > +++ b/arch/mips/kvm/vz.c > > > @@ -29,7 +29,9 @@ > > > #include <linux/kvm_host.h> > > > #include "interrupt.h" > > > +#ifdef CONFIG_CPU_LOONGSON64 > > > #include "loongson_regs.h" > > > +#endif > > The fix for this should be in the .h file, no #ifdef should be needed in > > a .c file. > > The header file can only be reached when CONFIG_CPU_LOONGSON64 is > selected... > Otherwise the include path of this file won't be a part of CFLAGS. That sounds like you should fix up the path of this file in the #include line as well :)
On 09/08/20 09:02, Greg KH wrote: >>>> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c >>>> index 3932f76..a474578 100644 >>>> --- a/arch/mips/kvm/vz.c >>>> +++ b/arch/mips/kvm/vz.c >>>> @@ -29,7 +29,9 @@ >>>> #include <linux/kvm_host.h> >>>> #include "interrupt.h" >>>> +#ifdef CONFIG_CPU_LOONGSON64 >>>> #include "loongson_regs.h" >>>> +#endif >>> The fix for this should be in the .h file, no #ifdef should be needed in >>> a .c file. >> The header file can only be reached when CONFIG_CPU_LOONGSON64 is >> selected... >> Otherwise the include path of this file won't be a part of CFLAGS. > That sounds like you should fix up the path of this file in the > #include line as well :) > There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and more #include "loongson_regs.h" in arch/mips. So while I agree with Greg that this idiom is quite unusual, it seems to be the expected way to use this header. I queued the patch. Paolo
On Sun, Aug 09, 2020 at 07:23:28PM +0200, Paolo Bonzini wrote: >On 09/08/20 09:02, Greg KH wrote: >>>>> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c >>>>> index 3932f76..a474578 100644 >>>>> --- a/arch/mips/kvm/vz.c >>>>> +++ b/arch/mips/kvm/vz.c >>>>> @@ -29,7 +29,9 @@ >>>>> #include <linux/kvm_host.h> >>>>> #include "interrupt.h" >>>>> +#ifdef CONFIG_CPU_LOONGSON64 >>>>> #include "loongson_regs.h" >>>>> +#endif >>>> The fix for this should be in the .h file, no #ifdef should be needed in >>>> a .c file. >>> The header file can only be reached when CONFIG_CPU_LOONGSON64 is >>> selected... >>> Otherwise the include path of this file won't be a part of CFLAGS. >> That sounds like you should fix up the path of this file in the >> #include line as well :) >> > >There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and >more #include "loongson_regs.h" in arch/mips. So while I agree with >Greg that this idiom is quite unusual, it seems to be the expected way >to use this header. I queued the patch. Where is that header coming from anyway? I can't find it in the tree, nor anything that would generate it.
On Sun, Aug 09, 2020 at 01:38:22PM -0400, Sasha Levin wrote: >On Sun, Aug 09, 2020 at 07:23:28PM +0200, Paolo Bonzini wrote: >>On 09/08/20 09:02, Greg KH wrote: >>>>>>diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c >>>>>>index 3932f76..a474578 100644 >>>>>>--- a/arch/mips/kvm/vz.c >>>>>>+++ b/arch/mips/kvm/vz.c >>>>>>@@ -29,7 +29,9 @@ >>>>>> #include <linux/kvm_host.h> >>>>>> #include "interrupt.h" >>>>>>+#ifdef CONFIG_CPU_LOONGSON64 >>>>>> #include "loongson_regs.h" >>>>>>+#endif >>>>>The fix for this should be in the .h file, no #ifdef should be needed in >>>>>a .c file. >>>>The header file can only be reached when CONFIG_CPU_LOONGSON64 is >>>>selected... >>>>Otherwise the include path of this file won't be a part of CFLAGS. >>>That sounds like you should fix up the path of this file in the >>>#include line as well :) >>> >> >>There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and >>more #include "loongson_regs.h" in arch/mips. So while I agree with >>Greg that this idiom is quite unusual, it seems to be the expected way >>to use this header. I queued the patch. > >Where is that header coming from anyway? I can't find it in the tree, >nor anything that would generate it. Woops, nevermind - sorry for the noise.
On Sun, Aug 09, 2020 at 07:23:28PM +0200, Paolo Bonzini wrote: > On 09/08/20 09:02, Greg KH wrote: > >>>> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c > >>>> index 3932f76..a474578 100644 > >>>> --- a/arch/mips/kvm/vz.c > >>>> +++ b/arch/mips/kvm/vz.c > >>>> @@ -29,7 +29,9 @@ > >>>> #include <linux/kvm_host.h> > >>>> #include "interrupt.h" > >>>> +#ifdef CONFIG_CPU_LOONGSON64 > >>>> #include "loongson_regs.h" > >>>> +#endif > >>> The fix for this should be in the .h file, no #ifdef should be needed in > >>> a .c file. > >> The header file can only be reached when CONFIG_CPU_LOONGSON64 is > >> selected... > >> Otherwise the include path of this file won't be a part of CFLAGS. > > That sounds like you should fix up the path of this file in the > > #include line as well :) > > > > There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and > more #include "loongson_regs.h" in arch/mips. So while I agree with > Greg that this idiom is quite unusual, it seems to be the expected way > to use this header. I queued the patch. Or you all could fix it up to work properly like all other #include lines in the kernel source tree. There's no reason mips should be "special" here, right? thanks, greg k-h
On 10/08/20 09:44, Greg KH wrote: >> There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and >> more #include "loongson_regs.h" in arch/mips. So while I agree with >> Greg that this idiom is quite unusual, it seems to be the expected way >> to use this header. I queued the patch. > Or you all could fix it up to work properly like all other #include > lines in the kernel source tree. There's no reason mips should be > "special" here, right? It's not just this #include, there's a couple dozen mach-* directories; changing how they work would be up to the MIPS maintainers (CCed), and it would certainly not be a patch that can be merged in stable@ kernels. arch/mips/kernel/cpu-probe.c has the same #ifdef CONFIG_CPU_LOONGSON64 #include <loongson_regs.h> for example, so apparently they're good with this. So if I don't pick up the patch to fix the build it would be in all likelihood merged by MIPS maintainers. The only difference will be how long the build remains broken and the fact that they need to worry about KVM despite the presence of a specific maintainer. KVM could of course just #include <asm/mach-loongson64/loongson_regs.h>, which would be found unconditionally. But there is some assembly in the header, so even if it would compile (I didn't check) it seems wrong to include it unconditionally and in fact it would be the only case of a file including <asm/mach-*/*.h> even if it is not compiled for that platform. Another alternative would be to move CONFIG_CPU_LOONGSON64 code out of arch/mips/kvm/vz.c and include it with obj-$(CONFIG_CPU_LOONGSON64). I'll gladly accept a patch to do that, but I won't write it since I don't have access to the hardware in order to test it. For now, and for the immediate purpose of not breaking the build, when in Rome I'll do as the Romans do. Paolo
On Mon, Aug 10, 2020 at 10:56:48AM +0200, Paolo Bonzini wrote: > On 10/08/20 09:44, Greg KH wrote: > >> There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and > >> more #include "loongson_regs.h" in arch/mips. So while I agree with > >> Greg that this idiom is quite unusual, it seems to be the expected way > >> to use this header. I queued the patch. > > Or you all could fix it up to work properly like all other #include > > lines in the kernel source tree. There's no reason mips should be > > "special" here, right? > > It's not just this #include, there's a couple dozen mach-* directories; > changing how they work would be up to the MIPS maintainers (CCed), and > it would certainly not be a patch that can be merged in stable@ kernels. > > arch/mips/kernel/cpu-probe.c has the same > > #ifdef CONFIG_CPU_LOONGSON64 > #include <loongson_regs.h> > > for example, so apparently they're good with this. So if I don't pick > up the patch to fix the build it would be in all likelihood merged by > MIPS maintainers. The only difference will be how long the build > remains broken and the fact that they need to worry about KVM despite > the presence of a specific maintainer. Ok, fair enough, but in the long-run, this should probably be fixed up "properly" if this arch is still being maintained. thanks, greg k-h
在 2020/8/10 下午5:03, Greg KH 写道: > On Mon, Aug 10, 2020 at 10:56:48AM +0200, Paolo Bonzini wrote: >> On 10/08/20 09:44, Greg KH wrote: >>>> There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and >>>> more #include "loongson_regs.h" in arch/mips. So while I agree with >>>> Greg that this idiom is quite unusual, it seems to be the expected way >>>> to use this header. I queued the patch. >>> Or you all could fix it up to work properly like all other #include >>> lines in the kernel source tree. There's no reason mips should be >>> "special" here, right? >> It's not just this #include, there's a couple dozen mach-* directories; >> changing how they work would be up to the MIPS maintainers (CCed), and >> it would certainly not be a patch that can be merged in stable@ kernels. >> >> arch/mips/kernel/cpu-probe.c has the same >> >> #ifdef CONFIG_CPU_LOONGSON64 >> #include <loongson_regs.h> >> >> for example, so apparently they're good with this. So if I don't pick >> up the patch to fix the build it would be in all likelihood merged by >> MIPS maintainers. The only difference will be how long the build >> remains broken and the fact that they need to worry about KVM despite >> the presence of a specific maintainer. > Ok, fair enough, but in the long-run, this should probably be fixed up > "properly" if this arch is still being maintained. MIPS is using another approach to management machine specified headers. It changes include path per-machine to get ride of the generic one. I'm a little bit confused about the proper way to do that. I thought the design overall, is just fine. Should we try to migrate it to another style? Thanks. - Jiaxun > > thanks, > > greg k-h
On Mon, Aug 10, 2020 at 11:03:10AM +0200, Greg KH wrote: > On Mon, Aug 10, 2020 at 10:56:48AM +0200, Paolo Bonzini wrote: > > On 10/08/20 09:44, Greg KH wrote: > > >> There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and > > >> more #include "loongson_regs.h" in arch/mips. So while I agree with > > >> Greg that this idiom is quite unusual, it seems to be the expected way > > >> to use this header. I queued the patch. > > > Or you all could fix it up to work properly like all other #include > > > lines in the kernel source tree. There's no reason mips should be > > > "special" here, right? > > > > It's not just this #include, there's a couple dozen mach-* directories; > > changing how they work would be up to the MIPS maintainers (CCed), and > > it would certainly not be a patch that can be merged in stable@ kernels. > > > > arch/mips/kernel/cpu-probe.c has the same > > > > #ifdef CONFIG_CPU_LOONGSON64 > > #include <loongson_regs.h> > > > > for example, so apparently they're good with this. So if I don't pick > > up the patch to fix the build it would be in all likelihood merged by > > MIPS maintainers. The only difference will be how long the build > > remains broken and the fact that they need to worry about KVM despite > > the presence of a specific maintainer. > > Ok, fair enough, but in the long-run, this should probably be fixed up > "properly" if this arch is still being maintained. I have it on my todo list. My plan is to move stuff out of mach-* directories, which aren't needed there. This should solve issues like the one here. Thomas.
On Mon, 10 Aug 2020, Thomas Bogendoerfer wrote: > > > It's not just this #include, there's a couple dozen mach-* directories; > > > changing how they work would be up to the MIPS maintainers (CCed), and > > > it would certainly not be a patch that can be merged in stable@ kernels. > > > > > > arch/mips/kernel/cpu-probe.c has the same > > > > > > #ifdef CONFIG_CPU_LOONGSON64 > > > #include <loongson_regs.h> > > > > > > for example, so apparently they're good with this. So if I don't pick > > > up the patch to fix the build it would be in all likelihood merged by > > > MIPS maintainers. The only difference will be how long the build > > > remains broken and the fact that they need to worry about KVM despite > > > the presence of a specific maintainer. > > > > Ok, fair enough, but in the long-run, this should probably be fixed up > > "properly" if this arch is still being maintained. > > I have it on my todo list. My plan is to move stuff out of mach-* directories, > which aren't needed there. This should solve issues like the one here. Correct, it looks like another maintainer's oversight. The asm/mach-<platform>/ directories are there for platform variants of generic stuff found in asm/mach-generic/. So if something is not there in asm/mach-generic/, then it must not be in any other asm/mach-<plaftorm>/ subdirectory either. Regular platform headers need to go under asm/<plaftorm>/. Compare asm/mach-dec/ vs asm/dec/, or asm/mach-sibyte/ vs asm/sibyte/, and so on. So this `loongson_regs.h' piece belongs to asm/loongson64/ rather than asm/mach-loongson64/. Maciej
diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c index 3932f76..a474578 100644 --- a/arch/mips/kvm/vz.c +++ b/arch/mips/kvm/vz.c @@ -29,7 +29,9 @@ #include <linux/kvm_host.h> #include "interrupt.h" +#ifdef CONFIG_CPU_LOONGSON64 #include "loongson_regs.h" +#endif #include "trace.h"
Only Loongson64 platform has and needs loongson_regs.h, including it unconditionally will cause build errors. Fixes: 7f2a83f1c2a941ebfee5 ("KVM: MIPS: Add CPUCFG emulation for Loongson-3") Cc: stable@vger.kernel.org Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Huacai Chen <chenhc@lemote.com> --- arch/mips/kvm/vz.c | 2 ++ 1 file changed, 2 insertions(+)