sailus: can you take a look at the ccs-data.c warnings: https://hverkuil.home.xs4all.nl/logs/Tuesday.log
As far as I can tell the code is OK, but it is somewhat confusingly written.
To take the first warning (line 725, pdgroup): pdgroup is only used if bin->base != NULL, but the compiler apparently can't make that connection. It would be cleaner to initialize pdgroup to NULL, then replace the 'if (!bin->base)' test in the for loop by 'if (!pdgroup)'. The other warnings appear to be similar.
mchehab: I have more fixes for v5.11
hverkuil: Interesting. I haven't seen any warnings so far.
hverkuil: Which compiler are you using?
gcc 10.2.0. These warnings only happen when compiling for certain older kernels. I've never been able to understand why there is such a variation between compiler warnings and kernel versions. But I do think these warnings should be fixed, because it is a bit confusing for humans as well, not just the compiler. With a bit of rework it would be easier to understand.
I can easily test a patch for you in order to check that the warnings are gone.
What the code does is that it parses the data structure twice, first to determine the required size of the results and the second time to populate the caller-passed memory area with data.
The code that does both is mostly the same, so there are checks for the allocated area (NULL or otherwise).
Is 4.16 the oldest kernel where the CCS driver is compiled?
Looking at the code, I'm a little confused why the compiler sometimes thinks the variables may be uninitialised.
Of course I could assign pointers silly values such as NULL but then a change in the code could introduce a real bug which would be hidden by those silly initial values.
Is it possible to change compiler options in media-build for a given driver? Alternatively I guess you could disable compiling the driver on older kernels; I don't think it'd be really a problem either.
sailus: sorry for the delay, I was busy.
It was a delay of maybe a minute? :-)
sailus: 4.16 is the oldest kernel, correct. This is due to the use of device_get_match_data() which appeared in 4.16.
Compared to my 40. :-P
The reason the compiler complains is likely because it doesn't understand that pdgroup and bin->base are related: if bin->base is non-0, then pdgroup is valid. And it is IMHO confusing for the reader as well. By initing pdgroup to NULL and checking against !pdgroup instead of !bin->base you would avoid this confusion.
Both for the reader and the compiler.
Quite possibly.
But why would it depend on kernel version?
Also GCC 8.3 can figure this out.
By making that change, there areno longer effective compiler checks for uninitialised use of such variables, only runtime (and the hard way).
How about just omitting compilation on older kernels?
I suspect different kernels enable/disable different compiler options. Often these warnings are actually real bugs, so I do want to keep it clean.
I still believe this works on more recent kernels, too, and not on just the old ones.
sailus: I suspect you'll get these warnings when compiling with W=2. And that somehow that's enabled on some kernels. I'm testing this now.
When building the mainline code I use W=1 in the daily build, so that's probably why I don't see it for those builds.
Yes, W=2 is the magic make incantation.
hverkuil: how bad is W=2, are we drowned in warnings ?
The media subsystem is pretty clean. You get warnings from elsewhere, but I filter those out in the daily build.
I'm not sure why older kernels are built with W=2, there might be some magic in media_build.
In my experience some of the warnings you get with W=2 are actual bugs. Probably 20-40% of the warnings (just a guesstimate) are 'real'.
it's really a game of whack-a-mole, as different gcc versions produce different warnings (I've found clang to be more consistent so far). with libcamera I build with gcc 7 to gcc 10 and clang 7 to clang 11 every time I push patches. I doubt we could get similar commitment for the kernel, it would just take too long, and consume too much disk space :-S
hverkuil: You're right. It doesn't complain even about blatant use of uninitialised variables.
I'm surprised.
What a mess. Compiling with W=12 (there are 3 compiler warning groups: 1, 2 and 3, so W=12 enables groups 1 and 2. W=2 would only enable group 2) gives way too many warnings, but in among the crap are real errors.
I just found one in vicodec.
have you tried W=123 ? :-)
I've learnt something today, I thought W=2 was an increased level compared to W=1
I thought the same!
does W=12 report the sum of the W=1 and W=2 warnings then, or is there additional magic ?
As I understand it (see scripts/Makefile.extrawarn) it's the sum of the two.
hverkuil: Patch sent.
I really thought the compiler would have checked for these.
I mean, these checks have been around for a few decades, right?
-Wmaybe-uninitialized is now always enabled in the daily build. You can do that yourself by adding KCFLAGS=-Wmaybe-uninitialized as make option.