linux-kernel - Re: [PATCH 08/30] net: wireless: ath: carl9170: Mark 'ar9170_qmap' as __maybe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Aug 2020 19:25:58 +0200
From:   Christian Lamparter <chunkeey@...il.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     davem@...emloft.net, kuba@...nel.org, linux-kernel@...r.kernel.org,
        Christian Lamparter <chunkeey@...glemail.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        Johannes Berg <johannes@...solutions.net>,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 08/30] net: wireless: ath: carl9170: Mark 'ar9170_qmap' as
 __maybe_unused

On 2020-08-14 18:40, Lee Jones wrote:
> On Fri, 14 Aug 2020, Christian Lamparter wrote:
> 
>> On 2020-08-14 13:39, Lee Jones wrote:
>>> 'ar9170_qmap' is used in some source files which include carl9170.h,
>>> but not all of them.  Mark it as __maybe_unused to show that this is
>>> not only okay, it's expected.
>>>
>>> Fixes the following W=1 kernel build warning(s)
>>
>> Is this W=1 really a "must" requirement? I find it strange having
> 
> Clean W=1 warnings is the dream, yes.
But is it a requirement?

> 
> I would have thought most Maintainers would be on-board with this.
 From what I know: It's no changes For changes' sake. Because otherwise 
this would be pretty broken for maintainers. They could just write and 
revert the same code over and over to prob up their LOC and commit 
counter. Wouldn't you agree there?

> 
> The ones I've worked with thus far have certainly been thankful.  Many
> had this on their own TODO lists.
Question is, do you really want to be just the cleanup crew there? Since 
semantic patches came along and a lot of this has been automated.
I'm of course after something else. Like: "Isn't there a better way than 
manually slapping __maybe_unused there to suppress the warning and call 
it a day?" If you already went down these avenues and can confirm that 
there's no alternative than this, then "fine". But if there is a better
method of doing this, then "let's go with that!".

> 
>> __maybe_unused in header files as this "suggests" that the
>> definition is redundant.
> 
> Not true.
> 
> If it were redundant then we would remove the line entirely.
So, why adding __maybe_unused then? I find it not very helpful to
tell the compiler to "shut up" when you want it's opinion...
This was the vibe I got from gcc's attribute unused help text.

Cheers,
Christian

> 
>>>    from drivers/net/wireless/ath/carl9170/carl9170.h:57,
>>>    In file included from drivers/net/wireless/ath/carl9170/carl9170.h:57,
>>>    drivers/net/wireless/ath/carl9170/carl9170.h:71:17: warning: ‘ar9170_qmap’ defined but not used [-Wunused-const-variable=]
>>>
>>> Cc: Christian Lamparter <chunkeey@...glemail.com>
>>> Cc: Kalle Valo <kvalo@...eaurora.org>
>>> Cc: "David S. Miller" <davem@...emloft.net>
>>> Cc: Jakub Kicinski <kuba@...nel.org>
>>> Cc: Johannes Berg <johannes@...solutions.net>
>>> Cc: linux-wireless@...r.kernel.org
>>> Cc: netdev@...r.kernel.org
>>> Signed-off-by: Lee Jones <lee.jones@...aro.org>
>>> ---
>>>    drivers/net/wireless/ath/carl9170/carl9170.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/carl9170/carl9170.h b/drivers/net/wireless/ath/carl9170/carl9170.h
>>> index 237d0cda1bcb0..9d86253081bce 100644
>>> --- a/drivers/net/wireless/ath/carl9170/carl9170.h
>>> +++ b/drivers/net/wireless/ath/carl9170/carl9170.h
>>> @@ -68,7 +68,7 @@
>>>    #define PAYLOAD_MAX	(CARL9170_MAX_CMD_LEN / 4 - 1)
>>> -static const u8 ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 };
>>> +static const u8 __maybe_unused ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 };
>>>    #define CARL9170_MAX_RX_BUFFER_SIZE		8192
>>>
>>
>