Re: [PATCH] Enable '-Werror' by default for all kernel builds

["Christian König" <christian.koenig@amd.com>]

Am 07.09.21 um 07:32 schrieb Huang Rui:
> On Tue, Sep 07, 2021 at 07:06:04AM +0800, Linus Torvalds wrote:
>> [ Adding some subsystem maintainers ]
>>
>> On Mon, Sep 6, 2021 at 10:06 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>>> But hopefully most cases are just "people haven't cared enough" and
>>>> easily fixed.
>>> We'll see. For my testbed I disabled the new configuration flag
>>> for the time being because its primary focus is boot tests, and
>>> there won't be any boot tests if images fail to build.
>> Sure, reasonable.
>>
>> I've checked a few of the build errors by doing the appropriate cross
>> compiles, and it doesn't seem bad - but it does seem like we have a
>> number of really pointless long-standing warnings that should have
>> been fixed long ago.
>>
>> For example, looking at sparc64, there are several build errors due to
>> those warnings now being fatal:
>>
>>   - drivers/gpu/drm/ttm/ttm_pool.c:386
>>
>>     This is a type mismatch error. It looks like __fls() on sparc64
>> returns 'int'. And the ttm_pool.c code assumes it returns 'unsigned
>> long'.
>>
>>     Oddly enough, the very line after that line does "min_t(unsigned
>> int" to get the types in line.
>>
>>     So  the immediate reason is "sparc64 is different". But the deeper
>> reason seems to be that ttm_pool.c has odd type assumptions. But that
>> warning should have been fixed long ago, either way.
>>
>>     Christian/Huang? I get the feeling that both lines in that file
>> should use the min_t(). Hmm?
>
> Shall we align the return type like __fls() on all the arches?

I think so, yes. IIRC I was a bit surprised that it returns UL on x86. I 
mean the maximum possible value here is 63.

>
> But it looks OK for me to change min to min_t() here as well, I can file a
> patch to the update:
>
> -       for (order = min(MAX_ORDER - 1UL, __fls(num_pages)); num_pages;
> +       for (order = min_t(unsigned int, MAX_ORDER - 1UL, __fls(num_pages)); num_pages;
>
> Christian, what's your opinion?

The "MAX_ORDER - 1UL" can now be changed to "MAX_ORDER - 1", but apart 
from that looks good to me.

Thanks,
Christian.

>
> Thanks,
> Ray
>
>>   - drivers/input/joystick/analog.c:160
>>
>>     #warning Precise timer not defined for this architecture.
>>
>>     Unfortunate. I suspect that warning just has to be removed. It has
>> never caused anything to be fixed, it's old to the point of predating
>> the git history. Dmitry?
>>
>>   - at least a couple of stringop-overread errors. Attached is a
>> possible for for one of them.
>>
>> The stringop overread is odd, because another one of them is
>>
>>     fs/qnx4/dir.c: In function ‘qnx4_readdir’:
>>     fs/qnx4/dir.c:51:32: error: ‘strnlen’ specified bound 48 exceeds
>> source size 16 [-Werror=stringop-overread]
>>        51 |                         size = strnlen(de->di_fname, size);
>>           |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> but I'm not seeing why that one happens on sparc64, but not on arm64
>> or x86-64. There doesn't seem to be anything architecture-specific
>> anywhere in that area.
>>
>> Funky.
>>
>> Davem - attached patch compiles cleanly for me, but I'm not sure it's
>> necessarily the right thing to do, and I didn't check the code
>> generation. Maybe it screws up. Can somebody test on sparc64 and
>> perhaps think about it more than I did?
>>
>>                 Linus
>>   arch/sparc/kernel/mdesc.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
>> index 8e645ddac58e..30f171b7b00c 100644
>> --- a/arch/sparc/kernel/mdesc.c
>> +++ b/arch/sparc/kernel/mdesc.c
>> @@ -39,6 +39,7 @@ struct mdesc_hdr {
>>   	u32	node_sz; /* node block size */
>>   	u32	name_sz; /* name block size */
>>   	u32	data_sz; /* data block size */
>> +	char	data[];
>>   } __attribute__((aligned(16)));
>>   
>>   struct mdesc_elem {
>> @@ -612,7 +613,7 @@ EXPORT_SYMBOL(mdesc_get_node_info);
>>   
>>   static struct mdesc_elem *node_block(struct mdesc_hdr *mdesc)
>>   {
>> -	return (struct mdesc_elem *) (mdesc + 1);
>> +	return (struct mdesc_elem *) mdesc->data;
>>   }
>>   
>>   static void *name_block(struct mdesc_hdr *mdesc)