kfifo: remove unnecessary type check

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---
 include/linux/kfifo.h | 20 --------------------
 1 file changed, 20 deletions(-)

include/linux/kfifo.h +0 -16

index 10308c6..b8c1d03 100644
@@ -390,10 +390,6 @@ __kfifo_int_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
-		__dummy = (typeof(__val))NULL; \
-	} \
 	if (__recsize) \
 		__ret = __kfifo_in_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -432,8 +428,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) \
-		__val = (typeof(__tmp->ptr))0; \
 	if (__recsize) \
 		__ret = __kfifo_out_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -473,8 +467,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) \
-		__val = (typeof(__tmp->ptr))NULL; \
 	if (__recsize) \
 		__ret = __kfifo_out_peek_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -512,10 +504,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
-		__dummy = (typeof(__buf))NULL; \
-	} \
 	(__recsize) ?\
 	__kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_in(__kfifo, __buf, __n); \

@@ -565,10 +553,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr) __dummy = NULL; \
-		__buf = __dummy; \
-	} \
 	(__recsize) ?\
 	__kfifo_out_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_out(__kfifo, __buf, __n); \

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 90 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---
 include/linux/kfifo.h | 20 --------------------
 1 file changed, 20 deletions(-)

include/linux/kfifo.h +0 -18

index 10308c6..b8c1d03 100644
@@ -390,10 +390,6 @@ __kfifo_int_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
-		__dummy = (typeof(__val))NULL; \
-	} \
 	if (__recsize) \
 		__ret = __kfifo_in_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -432,8 +428,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) \
-		__val = (typeof(__tmp->ptr))0; \
 	if (__recsize) \
 		__ret = __kfifo_out_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -473,8 +467,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) \
-		__val = (typeof(__tmp->ptr))NULL; \
 	if (__recsize) \
 		__ret = __kfifo_out_peek_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -512,10 +504,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
-		__dummy = (typeof(__buf))NULL; \
-	} \
 	(__recsize) ?\
 	__kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_in(__kfifo, __buf, __n); \

@@ -565,10 +553,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr) __dummy = NULL; \
-		__buf = __dummy; \
-	} \
 	(__recsize) ?\
 	__kfifo_out_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_out(__kfifo, __buf, __n); \

@@ -777,10 +761,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr) __dummy __attribute__ ((unused)) = NULL; \

include/linux/kfifo.h __kfifo_uint_must_check_helper( \ · line 766

Quoted 5 lines

include/linux/kfifo.h +0 -2

-		__buf = __dummy; \
-	} \
 	(__recsize) ? \
 	__kfifo_out_peek_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_out_peek(__kfifo, __buf, __n); \

Did you tried to compile the whole kernel including all the drivers with your patch?

Quoted 29 lines

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 19 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Quoted 7 lines

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---

Quoted 2 lines

Did you tried to compile the whole kernel including all the drivers with your patch?

Hi Stefani,

I did a build test, it did't introduce any new compile errors and warnings. While, I haven't tried make allmodconfig then. Does this patch seems wrong to you?

Thanks,
Yuanhan Liu

Am Freitag, den 26.10.2012, 14:11 +0800 schrieb Yuanhan Liu:

Quoted 33 lines · Stefani Seibold

Quoted 29 lines

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 19 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Quoted 7 lines

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---

Quoted 2 lines

Did you tried to compile the whole kernel including all the drivers with your patch?

Quoted 8 lines

Hi Stefani,

I did a build test, it did't introduce any new compile errors and warnings. While, I haven't tried make allmodconfig then. Does this patch seems wrong to you?

Thanks,
Yuanhan Liu

Hi Liu,

no the patch seems not wrong to me. But as you see with the previous patch it is not easy to predict the side effects.

An allmodconfig together with C=2 is necessary to check if there is no side effects which current users of the kfifo API.

That is exactly what
i did again and again as i developed the kfifo API.

Also you have to build the kfifo samples, since this example code use all features of the kfifo API.

And again: The kfifo is designed to do the many things at compile time, not at runtime. If you modify the code, you have to check the compiler assembler output for no degradation, especially in kfifo_put, kfifo_get, kfifo_in, kfifo_out, __kfifo_in and __kfifo_out. Prevent runtime checks if you can do it at compile time. This is the basic reasons to do it in macros.

Quoted 43 lines · Stefani Seibold

Am Freitag, den 26.10.2012, 14:11 +0800 schrieb Yuanhan Liu:

Quoted 33 lines · Stefani Seibold

Quoted 29 lines

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 19 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Quoted 7 lines

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---

Quoted 2 lines

Did you tried to compile the whole kernel including all the drivers with your patch?

Quoted 8 lines

Hi Stefani,

I did a build test, it did't introduce any new compile errors and warnings. While, I haven't tried make allmodconfig then. Does this patch seems wrong to you?

Thanks,
Yuanhan Liu

Quoted 7 lines

Hi Liu,

no the patch seems not wrong to me. But as you see with the previous patch it is not easy to predict the side effects.

An allmodconfig together with C=2 is necessary to check if there is no side effects which current users of the kfifo API.

Hi Stefani,

Make with C=2 will produce tons of warnings, hard to tell it introduces new warnings or not. I build some drivers used kfifo and samples as you suggested with C=2, find no new warnings. I will build all drivers that used kfifo with C=2 later, and will post the result here.

Quoted 7 lines

That is exactly what
i did again and again as i developed the kfifo API.

Also you have to build the kfifo samples, since this example code use all features of the kfifo API.

Quoted 6 lines

And again: The kfifo is designed to do the many things at compile time, not at runtime. If you modify the code, you have to check the compiler assembler output for no degradation, especially in kfifo_put, kfifo_get, kfifo_in, kfifo_out, __kfifo_in and __kfifo_out. Prevent runtime checks if you can do it at compile time. This is the basic reasons to do it in macros.

Is it enought to check kernel/kfifo.o only? I build that file with and without this patch. And then dump it by objdump -D kernel/fifo.o to /tmp/kfifo.dump.with and /tmp/kfifo.dump.without, respectively. And the two dump file are exactly same.

Thanks,
Yuanhan Liu

Quoted 43 lines

Am Freitag, den 26.10.2012, 14:11 +0800 schrieb Yuanhan Liu:

Quoted 33 lines · Stefani Seibold

Quoted 29 lines

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 19 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Quoted 7 lines

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---

Quoted 2 lines

Did you tried to compile the whole kernel including all the drivers with your patch?

Quoted 8 lines

Hi Stefani,

I did a build test, it did't introduce any new compile errors and warnings. While, I haven't tried make allmodconfig then. Does this patch seems wrong to you?

Thanks,
Yuanhan Liu

Quoted 7 lines

Hi Liu,

no the patch seems not wrong to me. But as you see with the previous patch it is not easy to predict the side effects.

An allmodconfig together with C=2 is necessary to check if there is no side effects which current users of the kfifo API.

Hi Stefani,

Make with C=2 will produce tons of warnings, hard to tell it introduces new warnings or not. I build some drivers used kfifo and samples as you suggested with C=2, find no new warnings. I will build all drivers that used kfifo with C=2 later, and will post the result here.

Quoted 10 lines

Also you have to build the kfifo samples, since this example code use all features of the kfifo API.

And again: The kfifo is designed to do the many things at compile time, not at runtime. If you modify the code, you have to check the compiler assembler output for no degradation, especially in kfifo_put, kfifo_get, kfifo_in, kfifo_out, __kfifo_in and __kfifo_out. Prevent runtime checks if you can do it at compile time. This is the basic reasons to do it in macros.

Is it enought to check kernel/kfifo.o only? I build that file with and without this patch. And then dump it by objdump -D kernel/fifo.o to /tmp/kfifo.dump.with and /tmp/kfifo.dump.without, respectively. And the two dump file are exactly same.

Am Freitag, den 26.10.2012, 15:17 +0800 schrieb Yuanhan Liu:

Quoted 31 lines · Stefani Seibold

Quoted 30 lines

Am Freitag, den 26.10.2012, 14:11 +0800 schrieb Yuanhan Liu:

Quoted 28 lines · Stefani Seibold

Quoted 27 lines

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 26 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---

Quoted 11 lines

Quoted 10 lines

Also you have to build the kfifo samples, since this example code use all features of the kfifo API.

And again: The kfifo is designed to do the many things at compile time, not at runtime. If you modify the code, you have to check the compiler assembler output for no degradation, especially in kfifo_put, kfifo_get, kfifo_in, kfifo_out, __kfifo_in and __kfifo_out. Prevent runtime checks if you can do it at compile time. This is the basic reasons to do it in macros.

Quoted 5 lines

Is it enought to check kernel/kfifo.o only? I build that file with and without this patch. And then dump it by objdump -D kernel/fifo.o to /tmp/kfifo.dump.with and /tmp/kfifo.dump.without, respectively. And the two dump file are exactly same.

No, since most of the code is inlined due performace reasons, you have to hack the kfifo examples output code for regressions and code increase.

Quoted 32 lines

Am Freitag, den 26.10.2012, 15:17 +0800 schrieb Yuanhan Liu:

Quoted 31 lines · Stefani Seibold

Quoted 30 lines

Am Freitag, den 26.10.2012, 14:11 +0800 schrieb Yuanhan Liu:

Quoted 28 lines · Stefani Seibold

Quoted 27 lines

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 26 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---

[snip]...

Quoted 17 lines

Quoted 11 lines

Quoted 10 lines

Also you have to build the kfifo samples, since this example code use all features of the kfifo API.

And again: The kfifo is designed to do the many things at compile time, not at runtime. If you modify the code, you have to check the compiler assembler output for no degradation, especially in kfifo_put, kfifo_get, kfifo_in, kfifo_out, __kfifo_in and __kfifo_out. Prevent runtime checks if you can do it at compile time. This is the basic reasons to do it in macros.

Quoted 5 lines

Is it enought to check kernel/kfifo.o only? I build that file with and without this patch. And then dump it by objdump -D kernel/fifo.o to /tmp/kfifo.dump.with and /tmp/kfifo.dump.without, respectively. And the two dump file are exactly same.

Quoted 3 lines

No, since most of the code is inlined due performace reasons, you have to hack the kfifo examples output code for regressions and code increase.

In my test, this patch doesn't change anything. Here are some data to prove that:

$ make samples/kfifo/
$ cp samples/kfifo/*.o /tmp/before/

$ git am this-patch
$ make samples/kfifo/
$ cp samples/kfifo/*.o /tmp/after/

$ for i in /tmp/before/*.o; do size $i /tmp/after/`basename $i`; done
   text    data     bss     dec     hex filename
   1939     464     456    2859     b2b /tmp/before/bytestream-example.o
   1939     464     456    2859     b2b /tmp/after/bytestream-example.o
   text    data     bss     dec     hex filename
   1423     112     296    1831     727 /tmp/before/dma-example.o
   1423     112     296    1831     727 /tmp/after/dma-example.o
   text    data     bss     dec     hex filename
   1864     624     376    2864     b30 /tmp/before/inttype-example.o
   1864     624     376    2864     b30 /tmp/after/inttype-example.o
   text    data     bss     dec     hex filename
   1916     464     472    2852     b24 /tmp/before/record-example.o
   1916     464     472    2852     b24 /tmp/after/record-example.o
# You will see that it changed nothing.

$ objdump -d /tmp/before/bytestream-example.o >/tmp/bytestream-example.before
$ objdump -d /tmp/after/bytestream-example.o >/tmp/bytestream-example.after
$ diff /tmp/bytestream.before /tmp/bytestream.after -urN

bytestream.after 2012-10-26 20:55:26.520578669 +0800 renamed from bytestream.before 2012-10-26 20:55:33.645578668 +0800 +1 -1

@@ -1,5 +1,5 @@
 
-/tmp/bytestream-example.o:     file format elf64-x86-64
+/tmp/bytestream-example.o:     file format elf64-x86-64
 
 # So, as you can see, expect the filename, they are same.
 

So, Stefani, is it what you want? Does this looks OK to you?

Am Freitag, den 26.10.2012, 21:04 +0800 schrieb Yuanhan Liu:

Quoted 93 lines · Stefani Seibold

Quoted 32 lines

Am Freitag, den 26.10.2012, 15:17 +0800 schrieb Yuanhan Liu:

Quoted 31 lines · Stefani Seibold

Quoted 30 lines

Am Freitag, den 26.10.2012, 14:11 +0800 schrieb Yuanhan Liu:

Quoted 28 lines · Stefani Seibold

Quoted 27 lines

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 26 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---

[snip]...

Quoted 17 lines

Quoted 11 lines

Quoted 10 lines

Also you have to build the kfifo samples, since this example code use all features of the kfifo API.

And again: The kfifo is designed to do the many things at compile time, not at runtime. If you modify the code, you have to check the compiler assembler output for no degradation, especially in kfifo_put, kfifo_get, kfifo_in, kfifo_out, __kfifo_in and __kfifo_out. Prevent runtime checks if you can do it at compile time. This is the basic reasons to do it in macros.

Quoted 5 lines

Is it enought to check kernel/kfifo.o only? I build that file with and without this patch. And then dump it by objdump -D kernel/fifo.o to /tmp/kfifo.dump.with and /tmp/kfifo.dump.without, respectively. And the two dump file are exactly same.

Quoted 3 lines

No, since most of the code is inlined due performace reasons, you have to hack the kfifo examples output code for regressions and code increase.

In my test, this patch doesn't change anything. Here are some data to prove that:

$ make samples/kfifo/
$ cp samples/kfifo/*.o /tmp/before/

$ git am this-patch
$ make samples/kfifo/
$ cp samples/kfifo/*.o /tmp/after/

$ for i in /tmp/before/*.o; do size $i /tmp/after/`basename $i`; done
   text    data     bss     dec     hex filename
   1939     464     456    2859     b2b /tmp/before/bytestream-example.o
   1939     464     456    2859     b2b /tmp/after/bytestream-example.o
   text    data     bss     dec     hex filename
   1423     112     296    1831     727 /tmp/before/dma-example.o
   1423     112     296    1831     727 /tmp/after/dma-example.o
   text    data     bss     dec     hex filename
   1864     624     376    2864     b30 /tmp/before/inttype-example.o
   1864     624     376    2864     b30 /tmp/after/inttype-example.o
   text    data     bss     dec     hex filename
   1916     464     472    2852     b24 /tmp/before/record-example.o
   1916     464     472    2852     b24 /tmp/after/record-example.o
# You will see that it changed nothing.

$ objdump -d /tmp/before/bytestream-example.o >/tmp/bytestream-example.before
$ objdump -d /tmp/after/bytestream-example.o >/tmp/bytestream-example.after
$ diff /tmp/bytestream.before /tmp/bytestream.after -urN

bytestream.after 2012-10-26 20:55:26.520578669 +0800 renamed from bytestream.before 2012-10-26 20:55:33.645578668 +0800 +1 -1

@@ -1,5 +1,5 @@
 
-/tmp/bytestream-example.o:     file format elf64-x86-64
+/tmp/bytestream-example.o:     file format elf64-x86-64
 

Quoted 4 lines

bytestream.after 2012-10-26 20:55:26.520578669 +0800 renamed from bytestream.before 2012-10-26 20:55:33.645578668 +0800 +0 -0

 # So, as you can see, expect the filename, they are same.
 

So, Stefani, is it what you want? Does this looks OK to you?

Perfect. It looks okay for me and i hope for you too ;-)

Acked by [email protected]

Quoted 101 lines · Yuanhan Liu

Quoted 99 lines · Stefani Seibold

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 85 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---
 include/linux/kfifo.h | 20 --------------------
 1 file changed, 20 deletions(-)

include/linux/kfifo.h +0 -16

index 10308c6..b8c1d03 100644
@@ -390,10 +390,6 @@ __kfifo_int_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
-		__dummy = (typeof(__val))NULL; \
-	} \
 	if (__recsize) \
 		__ret = __kfifo_in_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -432,8 +428,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) \
-		__val = (typeof(__tmp->ptr))0; \
 	if (__recsize) \
 		__ret = __kfifo_out_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -473,8 +467,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) \
-		__val = (typeof(__tmp->ptr))NULL; \
 	if (__recsize) \
 		__ret = __kfifo_out_peek_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -512,10 +504,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
-		__dummy = (typeof(__buf))NULL; \
-	} \
 	(__recsize) ?\
 	__kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_in(__kfifo, __buf, __n); \

@@ -565,10 +553,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr) __dummy = NULL; \
-		__buf = __dummy; \
-	} \
 	(__recsize) ?\
 	__kfifo_out_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_out(__kfifo, __buf, __n); \

include/linux/kfifo.h __kfifo_uint_must_check_helper( \ · line 766

Quoted 10 lines

include/linux/kfifo.h +0 -4

@@ -777,10 +761,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr) __dummy __attribute__ ((unused)) = NULL; \
-		__buf = __dummy; \
-	} \
 	(__recsize) ? \
 	__kfifo_out_peek_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_out_peek(__kfifo, __buf, __n); \

Did you tried to compile the whole kernel including all the drivers with your patch?

Quoted 5 lines

Hi Stefani,

I did a build test, it did't introduce any new compile errors and warnings. While, I haven't tried make allmodconfig then. Does this patch seems wrong to you?

Hmm, in my mind, those warnings are produced by the code you removed. So it is reasonable that you see no new warnings.

Quoted 112 lines · Yuanhan Liu

Quoted 102 lines · Richard Yang

Quoted 101 lines · Yuanhan Liu

Quoted 99 lines · Stefani Seibold

Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:

Quoted 85 lines

From: Yuanhan Liu <[email protected]>

Firstly, this kind of type check doesn't work. It does something similay like following:

	void * __dummy = NULL;
	__buf = __dummy;

__dummy is defined as void *. Thus it will not trigger warnings as expected.

Second, we don't need that kind of check. Since the prototype
of __kfifo_out is:
unsigned int __kfifo_out(struct __kfifo *fifo, void *buf, unsigned int len)

buf is defined as void *, so we don't need do the type check. Remove it.

LINK https://lkml.org/lkml/2012/10/25/386

LINK https://lkml.org/lkml/2012/10/25/584

Cc Andrew Morton <[email protected]>

Cc Wei Yang <[email protected]>

Cc Stefani Seibold <[email protected]>

Cc Fengguang Wu <[email protected]>

Cc Stephen Rothwell <[email protected]>

Signed-off-by Yuanhan Liu <[email protected]>

---
 include/linux/kfifo.h | 20 --------------------
 1 file changed, 20 deletions(-)

include/linux/kfifo.h +0 -16

index 10308c6..b8c1d03 100644
@@ -390,10 +390,6 @@ __kfifo_int_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
-		__dummy = (typeof(__val))NULL; \
-	} \
 	if (__recsize) \
 		__ret = __kfifo_in_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -432,8 +428,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) \
-		__val = (typeof(__tmp->ptr))0; \
 	if (__recsize) \
 		__ret = __kfifo_out_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -473,8 +467,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned int __ret; \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) \
-		__val = (typeof(__tmp->ptr))NULL; \
 	if (__recsize) \
 		__ret = __kfifo_out_peek_r(__kfifo, __val, sizeof(*__val), \
 			__recsize); \

@@ -512,10 +504,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
-		__dummy = (typeof(__buf))NULL; \
-	} \
 	(__recsize) ?\
 	__kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_in(__kfifo, __buf, __n); \

@@ -565,10 +553,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr) __dummy = NULL; \
-		__buf = __dummy; \
-	} \
 	(__recsize) ?\
 	__kfifo_out_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_out(__kfifo, __buf, __n); \

include/linux/kfifo.h __kfifo_uint_must_check_helper( \ · line 766

Quoted 10 lines

include/linux/kfifo.h +0 -4

@@ -777,10 +761,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr) __dummy __attribute__ ((unused)) = NULL; \
-		__buf = __dummy; \
-	} \
 	(__recsize) ? \
 	__kfifo_out_peek_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_out_peek(__kfifo, __buf, __n); \

Did you tried to compile the whole kernel including all the drivers with your patch?

Quoted 8 lines

Quoted 5 lines

Hi Stefani,

I did a build test, it did't introduce any new compile errors and warnings. While, I haven't tried make allmodconfig then. Does this patch seems wrong to you?

Hmm, in my mind, those warnings are produced by the code you removed. So it is reasonable that you see no new warnings.

Quoted 7 lines

Hi Yang,

Nope. It's not hard to tell they are old warnings. And as you can see from the email I posted, I did a build compile with and without applying this patch, and does make C=2 2>/tmp/out.{before,after}. I then compared the two file, they are exactly same. So, no new warnings introduced.

I don't understand this mail https://lkml.org/lkml/2012/10/25/584.

So Andrew means just remove the type check?