linux-kernel - Re: [PATCH] arch/tile: new multi-core architecture for Linux

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100524202204.GA9917@merkur.ravnborg.org>
Date:	Mon, 24 May 2010 22:22:05 +0200
From:	Sam Ravnborg <sam@...nborg.org>
To:	Chris Metcalf <cmetcalf@...era.com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-arch@...r.kernel.org
Subject: Re: [PATCH] arch/tile: new multi-core architecture for Linux

Hi Chris.

Kernle code looked good from a quick browsing.

Please explain the need for all the different directories within include/
{arch, hv, netio}

I tried not to repeat comments from Arnd in the below.

arch/tile/Kconfig:
The TILE specific symbols looks like they use several different
naming schemes. In some cases the company name (TILERA) is used
and in some cases TILE is used. And both as prefix and suffix.

Please stick to using TILE_ prefix. And maybe TILEGX_ in the
cases this is relevant.

Keep all the simple settings in the top of the file.
Stuff like:
config ZONE_DMA
	def_bool y

config SEMAPHORE_SLEEPERS
	def_bool y

Belongs in the top of Kconfig - before your first menu.

There is also several TILE specific options missing the TILE_ prefix.
Like:
config XGBE_MAIN
	tristate "Tilera GBE/XGBE character device support"

Drop this:
config XGBE_MAIN
	tristate "Tilera GBE/XGBE character device support"

It is better to test for the gcc version and disable the option
only in the cases where it is known to fail.

arch/tile/Makefile:

Do not mess with CC like this:
CC = $(CROSS_COMPILE)gcc

I guess you had to do this to support:
LIBGCC_PATH     := `$(CC) -print-libgcc-file-name`

If you follow other archs you could do like this:
LIBGCC_PATH     := `$(CC) -print-libgcc-file-name`

This is not needed:
CLEAN_FILES	+= arch/tile/vmlinux.lds

vmlinux.lds lives in kernel/

help is missing.

arch/tile/kernel/Makefile
I has expected that compiling vmlinux.lds required knowledge on $(BITS)
like this:
CPPFLAGS_vmlinux.lds := -m$(BITS)

arch/tile/kernel/vmlinux.lds.S
A lot of effort has been put into unifying the different
variants of vmlinux.lds.
Please see the skeleton outlined in include/asm-generic/vmlinux.lds.h

You include sections.lds - but it is empty.
Drop that file.

You include hvglue.ld.
We use *.lds for linker script file - please rename.
The file looks generated?? How and when?

Futhermore the definitions are not used by vmlinux.lds.S - so drop the include.

arch/tile/initramfs:
Does not look like it belongs in the kernel?

arch/tile/kernel/head_32.S
The file uses:
	.section .text.head, "ax"
etc.

Please use the section definitions from include/linux/init.h

arch/tile/include/asm/spinlock.h
Please make this a one-liner when you uses the asm-generic version only.
Same goes for byteorder (which includes linux/byteorder/little_endian.h)

In your mail you did not say anything about the checkpatch status.
It is better that you make your code reasonable checkpatch clean _before_
merging. Then you will not be hit by a lot of janitorial patches doing so.

Likewise please state sparse status. We do not expect it to be sparse clean.
But getting rid of the obvious issues is good too.

	Sam

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/