[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87fuos8e25.fsf@concordia.ellerman.id.au>
Date: Thu, 22 Sep 2016 19:43:46 +1000
From: Michael Ellerman <mpe@...erman.id.au>
To: Emilio López <emilio.lopez@...labora.co.uk>,
shuah@...nel.org
Cc: devel@...verdev.osuosl.org, gustavo.padovan@...labora.co.uk,
riandrews@...roid.com, daniel.vetter@...ll.ch,
John.C.Harrison@...el.com, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org, ghackmann@...gle.com,
robdclark@...il.com, linux-kselftest@...r.kernel.org,
maarten.lankhorst@...ux.intel.com, daniels@...labora.com,
arve@...roid.com, emil.l.velikov@...il.com,
Emilio López <emilio.lopez@...labora.co.uk>
Subject: Re: [PATCH 1/7] selftest: sync: basic tests for sw_sync framework
Emilio López <emilio.lopez@...labora.co.uk> writes:
> These tests are based on the libsync test suite from Android.
> This commit lays the ground for future tests, as well as includes
> tests for a variety of basic allocation commands.
Hi Emilio,
Just a few comments on the Makefile.
> diff --git a/tools/testing/selftests/sync/Makefile b/tools/testing/selftests/sync/Makefile
> new file mode 100644
> index 0000000..f67827f
> --- /dev/null
> +++ b/tools/testing/selftests/sync/Makefile
> @@ -0,0 +1,24 @@
> +CC = $(CROSS_COMPILE)gcc
lib.mk does that for you.
> +CFLAGS := -O2 -g -std=gnu89 -pthread -Wall -Wextra
It's more polite to just add to CFLAGS rather than defining it from
scratch. That way a user can add CFLAGS via setting them in the
environment.
ie. this would be:
CFLAGS += -O2 -g -std=gnu89 -pthread -Wall -Wextra
or:
CFLAGS := -O2 -g -std=gnu89 -pthread -Wall -Wextra $(CFLAGS)
> +CFLAGS += -I../../../../include/uapi/
Please don't include the unprocessed uapi headers, they are not meant to
be directly included in userspace programs.
They even say so:
include/uapi/linux/types.h:#warning "Attempt to use kernel headers from user space, see http://kernelnewbies.org/KernelHeaders"
> +CFLAGS += -I../../../../include/
Please don't include the *kernel* headers, they're really not meant to
be used in userspace programs :)
> +CFLAGS += -I../../../../usr/include/
That is the correct place to get them from. They'll have been put there
by 'make headers_install'.
> +CFLAGS += -I../../../../drivers/dma-buf/
That's also a bit fishy.
> +LDFLAGS += -pthread
> +
> +TEST_PROGS = sync_test
> +
> +all: $(TEST_PROGS)
> +
> +include ../lib.mk
> +
> +SRC = sync_test.o sync.o
SRC would usually point to .c files, .o's would be in OBJS or something
similar. Though it's not that important obviously.
Just listing the .c files in SRC should work, make knows how to turn
them into .o's for you.
> +TESTS += sync_alloc.o
And similarly there.
So I think you could just use:
SRC := sync_test.c sync.c sync_alloc.c
Or if you actually just want to list all the .c files in the directory
then this would also work:
SRC := $(wildcard *.c)
> +sync_test: $(SRC) $(TESTS)
> +
> +.PHONY: clean
lib.mk did that for you.
> +
> +clean:
> + $(RM) sync_test $(SRC) $(TESTS)
If you redefined SRC above to be the actual sources then you obviously
don't want to clean them, so here you would just use *.o.
cheers
Powered by blists - more mailing lists