Re: [PATCH 1/7] selftest: sync: basic tests for sw_sync framework

[Michael Ellerman]

Emilio LÃpez <emilio.lopez@xxxxxxxxxxxxxxx> 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