Re: Microblaze Linux release

[Arnd Bergmann <arnd@arndb.de>]

On Monday 14 April 2008, Arnd Bergmann wrote:

> I'm certainly willing to help review it, but I think the timing is too short
> for asking for a 2.6.26 merge. Judging from experience, a new architecture
> review takes a few weeks to sort out all the issues you want resolved before
> merging, so posting them now would put you in a much more comfortable position
> when targeting a 2.6.27 merge.

I've just looked at your git tree, and it confirmed my point that we need
a little more time. Please don't hesitate to submit patches to the mailing
list though. You may have misunderstood the idea of the merge window: The
patches can go in during that time if they have been reviewed before, so
it is not the time to post patches for review.

More to the point, here are a few things that I noticed on a brief
review:

* Your arch code in general looks nice and clean, don't worry too much
about the coding style right now.

* The code level is 2.6.24-rc8, while the current head is at 2.6.25-rc9.
You should really upgrade the kernel level, because a number of changes
have gone in that directly affect your code. Your best bet is probably
now to have a patch against the linux-next tree, which also has the changes
that will go into 2.6.26.

* Your syscall ABI is largely obsolete. A third of the syscalls you
define should not even be there in the first place as they have been
replaced by newer versions. E.g. you have select, _newselect and pselect6,
where just pselect6 would be sufficient -- you don't need to worry about
backwards compatibility if you don't have existing code. A good start
would be to take the arch/blackfin syscall list and further reduce it
from there. Further examples are:
- replace socketcall with separate syscall entry points
- replace ipc with a separate entry points
- remove all the legacy signal handling from arch/microblaze/kernel/signal.c
- remove sys_mmap, sys_olduname and sys_vfork
- finally define a generic sys_mmap2 and sys_uname in kernel/ so you don't
  need another copy in arch/microblaze/kernel
- Use 64 bit off_t, and 32 bit uid_t, gid_t etc.

* Your device tree functions in of_device.c, of_platform.c, prom.c and
prom_parse.c are basically copies of the powerpc versions. This duplication
is not helpful, better merge that code into new files in drivers/of/
so that it can be shared.

* The semaphore implementation will be obsolete in 2.6.26

* The EXPORT_SYMBOL definitions for csum_partial etc should be in
arch/microblaze/lib/checksum.c, not in microblaze_ksyms.c, same
for memcpy/memmove/memset.

* The platform directory is noticably empty. You can probably judge better
how much platform specific code there will be in the future, but I would
suspect that you're better off with a flat platform directory instead
of subdirectories below that.

* the consistent.c implementation looks strange. Is any of that code
even used anywhere? What is it good for, considering that you don't have
an mmu?

* Your dma_mapping.h does BUG() for almost everything. I suspect you could
easily replace it with a trivial implementation, like the x86_32 one.

* I'm not sure if you really need something as sophisticated as the lmb
code, if you don't have a real firmware, or even an MMU. setup_memory
can probably work with something much simpler, and for the prom.c stuff,
you may get away with #defining them to bootmem_alloc etc.

* You don't seem to have any code for PCI, so the pci.h and pci-bridge.h
files don't make much sense.

* The following files:
atomic.h checksum.h io.h ioctl.h ioctls.h ipcbuf.h msgbuf.h param.h
poll.h posix_types.h sembuf.h shmbuf.h shmparam.h sigcontext.h signal.h
socket.h sockios.h stat.h termbits.h termios.h types.h ucontext.h
all look like verbatim copies of some other architecture. Please do
the next person to come up with a new architecture a favor and stick them
in include/asm-generic instead, including them from your own headers.

* You unistd.h contains _syscall* macros. These were removed some time
ago from all other architectures.

* You define a lot of __ARCH_WANT_SYS_ macros. Generally, these are
for the syscalls you don't want.

* You are missing a include/asm-microblaze/Kbuild file that lists all
the header files you want to export to user space.

	Arnd <><