Hi Paul, many thanks for your review. all the fixes will be on next patchset. my comments are below. At Mon, 20 Apr 2015 22:43:07 +0200, Paul Bolle wrote: > > Some random observations while I'm still trying to wrap my head around > all this (which might take quite some time). > > On Sun, 2015-04-19 at 22:28 +0900, Hajime Tazaki wrote: > > --- /dev/null > > +++ b/arch/lib/Kconfig > > @@ -0,0 +1,124 @@ > > +menuconfig LIB > > + bool "LibOS-specific options" > > + def_bool n > > This is the start of the Kconfig parse for lib. (That would basically > still be true even if you didn't set KBUILD_KCONFIG, see below.) So why > not do something like all arches do: > > config LIB > def_bool y > select [...] > > Ie, why would someone want to build for ARCH=lib and still not set LIB? agreed. fixed. > > +config EXPERIMENTAL > > + def_bool y > > Unneeded: removed treewide in, I think, 2014. thanks. fixed. > > +config MMU > > + def_bool n > > Add empty line. > > > +config FPU > > + def_bool n > > Ditto. both are fixed. > > +config KTIME_SCALAR > > + def_bool y > > This one is unused. deleted. > > +config GENERIC_BUG > > + def_bool y > > + depends on BUG > > Add empty line here. fixed. > > +config GENERIC_FIND_NEXT_BIT > > + def_bool y > > This one is unused too. deleted. > > +config SLIB > > + def_bool y > > You've also added SLIB to init/Kconfig in 02/10. But "make ARCH=lib > *config" will never visit init/Kconfig, will it? And, apparently, none > of SL[AOU]B are wanted for lib. So I think the entry for config SLIB in > that file can be dropped (as other arches will never see it because it > depends on LIB). > > (Note that I haven't actually looked into all the Kconfig entries added > above. Perhaps I might do that. But I'm pretty sure most of the time all > I can say is: "I have no idea why this entry defaults to $VALUE".) I intended to SLIB be a generic one, not only for the arch/lib, as we discussed during v2 patch. but, you're right: for the moment, no one uses SLIB, we don't visit init/Kconfig, I dropped config SLIB entry from init/Kconfig. > > +source "net/Kconfig" > > + > > +source "drivers/base/Kconfig" > > + > > +source "crypto/Kconfig" > > + > > +source "lib/Kconfig" > > + > > + > > Trailing empty lines. deleted. thanks. > > diff --git a/arch/lib/Makefile b/arch/lib/Makefile > > new file mode 100644 > > index 0000000..d8a0bf9 > > --- /dev/null > > +++ b/arch/lib/Makefile > > @@ -0,0 +1,251 @@ > > +ARCH_DIR := arch/lib > > +SRCDIR=$(dir $(firstword $(MAKEFILE_LIST))) > > Do you use SRCDIR? no. deleted the line. > > +DCE_TESTDIR=$(srctree)/tools/testing/libos/ > > +KBUILD_KCONFIG := arch/$(ARCH)/Kconfig > > I think you copied this from arch/um/Makefile. But arch/um/ is, well, > special. Why should lib not start the kconfig parse in the file named > Kconfig? And if you want to start in arch/lib/Kconfig, it would be nice > to add a mainmenu (just like arch/x86/um/Kconfig does). right now, 'lib' only wants to eat arch/lib/Kconfig so that build and link its wanted files instead of configurable one. so I beilive arch/lib is also special as arch/um is. I added a mainmenu btw. thanks. > (I don't read Makefilese well enough to understand the rest of this > file. I think it's scary.) indeed. thank you again to review the cryptic files.. > When I did > make ARCH=lib menuconfig > > I saw (among other things): > arch/lib/Makefile.print:41: target `trace/' given more than once in the same rule. > arch/lib/Makefile.print:41: target `trace/' given more than once in the same rule. > arch/lib/Makefile.print:41: target `trace/' given more than once in the same rule. > arch/lib/Makefile.print:41: target `trace/' given more than once in the same rule. > arch/lib/Makefile.print:41: target `lzo/' given more than once in the same rule. (snip) > arch/lib/Makefile.print:41: target `ppp/' given more than once in the same rule. > arch/lib/Makefile.print:41: target `slip/' given more than once in the same rule. > > I have no idea why. Unclean tree? this was due to inappropriate handling of the internal directory listing procedure. fixed. > > +.PHONY : core > > +.NOTPARALLEL : print $(subdirs) $(final-obj-m) > > > --- /dev/null > > +++ b/arch/lib/processor.mk > > @@ -0,0 +1,7 @@ > > +PROCESSOR=$(shell uname -m) > > +PROCESSOR_x86_64=64 > > +PROCESSOR_i686=32 > > +PROCESSOR_i586=32 > > +PROCESSOR_i386=32 > > +PROCESSOR_i486=32 > > +PROCESSOR_SIZE=$(PROCESSOR_$(PROCESSOR)) > > The rest of the tree appears to use BITS instead of PROCESSOR_SIZE. And > I do hope there's a cleaner way for lib to set PROCESSOR_SIZE than this. the variable PROCESSOR_SIZE is only used by arch/lib/Makefile, with the following lines. > +ifeq ($(PROCESSOR_SIZE),64) > +CFLAGS+= -DCONFIG_64BIT > +endif Thus it eventually uses CONFIG_64BIT. I think a cleaner way is to follow the way of arch/um, like below: I deleted processor.mk and PROCESSOR_SIZE variable. ifeq ($(SUBARCH),x86) ifeq ($(shell uname -m),x86_64) CFLAGS+= -DCONFIG_64BIT endif though it's not able to cross-compile yet. again, thank you so much. I'll be back very soon (v4 patch). -- Hajime -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>