On Sun, Jul 28, 2024, at 22:13, Linus Torvalds wrote:
> On Sun, 28 Jul 2024 at 13:10, David Laight <David.Laight@xxxxxxxxxx> wrote:
>>
>> I think they just need to be MIN_CONST() (without the casts).
>
> I'll just convert the existing cases of min_t/max_t to MIN_T/MAX_T,
> which I already added for other reasons anyway.
>
> That makes min_t/max_t not have to care about the nasty special cases
> (really just array sizes in these cases, and they all wanted MAX_T).
I had prototyped something similar end of last week but didn't manage
to get my version out to you before the weekend. Comparing mine with
what you ended up committing:
- You found exactly the same array index uses I found in
randconfig testing, so I'm not aware of anything missing
there.
- My macros use __builtin_choose_expr() instead of ?: to
ensure that the arguments are constant, this produces a
relatively clear compiler warning when they are not.
Without that, I would expect random drivers to start
using MIN()/MAX() in places where it's not safe.
- I went with the belts-and-suspenders version of MIN()/MAX()
that works when comparing a negative constant against
an unsigned one. This requires expanding each argument
four or five times instead of two, so you might still
want the simpler version (like MIN_T/MAX_T):
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -295,12 +271,18 @@ static inline bool in_range32(u32 val, u32 start, u32 len)
do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
/*
- * Use these carefully: no type checking, and uses the arguments
- * multiple times. Use for obvious constants only.
+ * These only work on constant values but return a constant value that
+ * can be used as an array size
*/
-#define MIN(a,b) __cmp(min,a,b)
-#define MAX(a,b) __cmp(max,a,b)
-#define MIN_T(type,a,b) __cmp(min,(type)(a),(type)(b))
-#define MAX_T(type,a,b) __cmp(max,(type)(a),(type)(b))
+#define MIN(x, y) \
+ __builtin_choose_expr(((x) < 0 && (y) > 0), (x), \
+ __builtin_choose_expr((((y) < 0 && (x) > 0) || (y) < (x)), (y), (x)))
+
+#define MAX(x, y) \
+ __builtin_choose_expr(((x) > 0 && (y) < 0), (x), \
+ __builtin_choose_expr((((y) > 0 && (x) < 0) || (y) > (x)), (y), (x)))
+
+#define MIN_T(type,a,b) (type)__builtin_choose_expr((type)(a) < (type)(b), (a), (b))
+#define MAX_T(type,a,b) (type)__builtin_choose_expr((type)(a) > (type)(b), (a), (b))
#endif /* _LINUX_MINMAX_H */
- The change above requires changing a number of files that were
previously using their own MIN()/MAX() macros over to using
min()/max(), as they are passing non-constant values in:
drivers/gpu/drm/amd/display/dc/core/dc_stream.c | 12 ++++--------
.../drm/amd/display/dc/dio/dcn20/dcn20_link_encoder.c | 9 +--------
.../drm/amd/display/dc/dio/dcn31/dcn31_dio_link_encoder.c | 8 ++------
.../drm/amd/display/dc/dio/dcn32/dcn32_dio_link_encoder.c | 6 +-----
.../drm/amd/display/dc/dio/dcn321/dcn321_dio_link_encoder.c | 4 ----
.../drm/amd/display/dc/dio/dcn401/dcn401_dio_link_encoder.c | 8 --------
.../drm/amd/display/dc/dml/dcn20/dcn20_fpu.c | 13 +++----------
.../drm/amd/display/dc/dsc/dc_dsc.c | 9 +--------
.../drm/amd/display/dc/link/protocols/link_dp_capability.c | 13 +++----------
drivers/gpu/drm/amd/display/modules/hdcp/hdcp_ddc.c | 11 ++++-------
drivers/gpu/drm/radeon/evergreen_cs.c | 9 ++-------
drivers/platform/x86/sony-laptop.c | 4 ++--
kernel/trace/preemptirq_delay_test.c | 2 +-
lib/decompress_unlzma.c | 7 ++-----
14 files changed, 26 insertions(+), 89 deletions(-)
Changing these is probably a good idea regardless.
- I also tried simplifying __types_ok() further, which as
you already mentioned doesn't easily work with pointer
arguments. Again we could work around this with a separate
min_ptr()/max_ptr() helper. I only found 11 files that
actually compare pointers (on x86/arm/arm64 randconfig):
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 2 +-
crypto/skcipher.c | 2 +-
drivers/gpu/drm/drm_modes.c | 2 +-
drivers/infiniband/hw/hfi1/pio_copy.c | 4 ++--
drivers/irqchip/irq-bcm7120-l2.c | 2 +-
drivers/spi/spi-cs42l43.c | 8 ++++----
fs/ntfs3/lznt.c | 2 +-
lib/lzo/lzo1x_compress.c | 2 +-
mm/kmemleak.c | 4 ++--
mm/percpu.c | 2 +-
net/ceph/osdmap.c | 6 +++---
11 files changed, 25 insertions(+), 18 deletions(-)
The simpler __types_ok() needs more testing across all
compiler versions, so that wouldn't be for 6.11 anyway.
I can send the min_ptr()/max_ptr() stuff anyway if
you think that's a good idea.
Arnd
