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:

quoted

--- /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.

quoted

+config EXPERIMENTAL
+	def_bool y

Unneeded: removed treewide in, I think, 2014.

thanks. fixed.

quoted

+config MMU
+        def_bool n

Add empty line.

quoted

+config FPU
+        def_bool n

Ditto.

both are fixed.

quoted

+config KTIME_SCALAR
+       def_bool y

This one is unused.

deleted.

quoted

+config GENERIC_BUG
+	def_bool y
+	depends on BUG

Add empty line here.

fixed.

quoted

+config GENERIC_FIND_NEXT_BIT
+	def_bool y

This one is unused too.

deleted.

quoted

+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.

quoted

+source "net/Kconfig"
+
+source "drivers/base/Kconfig"
+
+source "crypto/Kconfig"
+
+source "lib/Kconfig"
+
+

Trailing empty lines.

deleted. thanks.

quoted

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.

quoted

+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.

quoted

+.PHONY : core
+.NOTPARALLEL : print $(subdirs) $(final-obj-m)

quoted

--- /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@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>