'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)
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=]
NB: Snipped - lots of these repeat
Cc: Christian Lamparter <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Lee Jones <[email protected]>
---
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
--
2.25.1
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
__maybe_unused in header files as this "suggests" that the
definition is redundant.
> 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 <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Johannes Berg <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Lee Jones <[email protected]>
> ---
> 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
>
>
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.
I would have thought most Maintainers would be on-board with this.
The ones I've worked with thus far have certainly been thankful. Many
had this on their own TODO lists.
> __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.
> > 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 <[email protected]>
> > Cc: Kalle Valo <[email protected]>
> > Cc: "David S. Miller" <[email protected]>
> > Cc: Jakub Kicinski <[email protected]>
> > Cc: Johannes Berg <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > 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
> >
>
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>
>>> Cc: Kalle Valo <[email protected]>
>>> Cc: "David S. Miller" <[email protected]>
>>> Cc: Jakub Kicinski <[email protected]>
>>> Cc: Johannes Berg <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Signed-off-by: Lee Jones <[email protected]>
>>> ---
>>> 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
>>>
>>
>
On 2020-08-17 14:59, Kalle Valo wrote:
> Rasmus Villemoes <[email protected]> writes:
>
>> On 14/08/2020 17.14, 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
>>> __maybe_unused in header files as this "suggests" that the
>>> definition is redundant.
>>
>> In this case it seems one could replace the table lookup with a
>>
>> static inline u8 ar9170_qmap(u8 idx) { return 3 - idx; }
>>
>> gcc doesn't warn about unused static inline functions (or one would have
>> a million warnings to deal with). Just my $0.02.
>
> Yeah, this is much better.
Yes, this is much better than just adding __maybe_unused :).
To be on the safe side (and to get rid of a & in tx.c:666), the "3 -
idx" should be something like "return (3 - idx) &
CARL9170_TX_STATUS_QUEUE". I think its also possible to just use clamp_t
(or min_t since the u8 has no negative values) or make use of a switch
statement [analogues what was done in ath10k commit: 91493e8e10 "ath10k:
fix recent bandwidth conversion bug" (just to be clear. Yes this ath10k
commit has nothing to do with queues, but it is a nice, atomic
switch-case static inline function example).]
> And I think that static variables should not even be in the header
> files. Doesn't it mean that there's a local copy of the variable
> everytime the .h file is included? Sure, in this case the overhead is
> small (4 bytes per include) but still it's wrong.
Seems to be "sort of". I compiled both, the current vanilla carl9170 and
with Lee Jones' patch on Debian's current gcc 10.2.0 (Debian 10.2.0-5)
and gnu ld 2.35.
The ar9170_qmap symbol is only present in the object file if ar9170_qmap
is used by the source. In the final module file (carl9170.ko), there are
two ar9170_qmap symboles in the module's .rodata section (one is coming
from main.o code and the other from tx.o).
(The use of __maybe_unused didn't make any difference. Same overall
section and file sizes).
Cheers,
Christian
On Mon, 17 Aug 2020, Kalle Valo wrote:
> Rasmus Villemoes <[email protected]> writes:
>
> > On 14/08/2020 17.14, 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
> >> __maybe_unused in header files as this "suggests" that the
> >> definition is redundant.
> >
> > In this case it seems one could replace the table lookup with a
> >
> > static inline u8 ar9170_qmap(u8 idx) { return 3 - idx; }
> >
> > gcc doesn't warn about unused static inline functions (or one would have
> > a million warnings to deal with). Just my $0.02.
>
> Yeah, this is much better.
>
> And I think that static variables should not even be in the header
> files. Doesn't it mean that there's a local copy of the variable
> everytime the .h file is included? Sure, in this case the overhead is
> small (4 bytes per include) but still it's wrong.
It happens a lot.
As I stated before, the 2 viable options are to a) move it into the
source files; ensuring code duplication, unnecessary maintenance
burden and probably disparity over time, or b) create (or locate if
there is one already) a special header file which is only to be
included by the users.
The later option gets really complicated if there are a variety of
tables which are included by any given number of source file
permutations.
The accepted answer in all of the other subsystems I've worked with so
far, is to use __maybe_unused. It's simple, non-intrusive and doesn't
rely on any functional changes.
> Having a static inline
> function would solve that problem as well the compiler warning.
This time yes, but it's a hack that will only work with simple,
linear data. Try doing that with some of the other, more complicated
tables, like mwifiex_sdio_sd8*.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
'ar9170_qmap' is used in some source files which include carl9170.h,
but not all of them. A 'defined but not used' warning is thrown when
compiling the ones which do not use it.
Fixes the following W=1 kernel build warning(s)
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=]
NB: Snipped - lots of these repeat
Cc: Christian Lamparter <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: [email protected]
Cc: [email protected]
Suggested-by: Rasmus Villemoes <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
---
drivers/net/wireless/ath/carl9170/carl9170.h | 5 ++++-
drivers/net/wireless/ath/carl9170/main.c | 2 +-
drivers/net/wireless/ath/carl9170/tx.c | 6 +++---
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/carl9170/carl9170.h b/drivers/net/wireless/ath/carl9170/carl9170.h
index 237d0cda1bcb0..0d38100d6e4f5 100644
--- a/drivers/net/wireless/ath/carl9170/carl9170.h
+++ b/drivers/net/wireless/ath/carl9170/carl9170.h
@@ -68,7 +68,10 @@
#define PAYLOAD_MAX (CARL9170_MAX_CMD_LEN / 4 - 1)
-static const u8 ar9170_qmap[__AR9170_NUM_TXQ] = { 3, 2, 1, 0 };
+static inline u8 ar9170_qmap(u8 idx)
+{
+ return 3 - idx; /* { 3, 2, 1, 0 } */
+}
#define CARL9170_MAX_RX_BUFFER_SIZE 8192
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
index 816929fb5b143..dbef9d8fc893b 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -1374,7 +1374,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
int ret;
mutex_lock(&ar->mutex);
- memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
+ memcpy(&ar->edcf[ar9170_qmap(queue)], param, sizeof(*param));
ret = carl9170_set_qos(ar);
mutex_unlock(&ar->mutex);
return ret;
diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
index 2407931440edb..2f9e275db8ef0 100644
--- a/drivers/net/wireless/ath/carl9170/tx.c
+++ b/drivers/net/wireless/ath/carl9170/tx.c
@@ -663,7 +663,7 @@ static void __carl9170_tx_process_status(struct ar9170 *ar,
unsigned int r, t, q;
bool success = true;
- q = ar9170_qmap[info & CARL9170_TX_STATUS_QUEUE];
+ q = ar9170_qmap(info & CARL9170_TX_STATUS_QUEUE);
skb = carl9170_get_queued_skb(ar, cookie, &ar->tx_status[q]);
if (!skb) {
@@ -979,7 +979,7 @@ static int carl9170_tx_prepare(struct ar9170 *ar,
((CARL9170_TX_SUPER_MISC_VIF_ID >>
CARL9170_TX_SUPER_MISC_VIF_ID_S) + 1));
- hw_queue = ar9170_qmap[carl9170_get_queue(ar, skb)];
+ hw_queue = ar9170_qmap(carl9170_get_queue(ar, skb));
hdr = (void *)skb->data;
info = IEEE80211_SKB_CB(skb);
@@ -1279,7 +1279,7 @@ void carl9170_tx_drop(struct ar9170 *ar, struct sk_buff *skb)
super = (void *)skb->data;
SET_VAL(CARL9170_TX_SUPER_MISC_QUEUE, q,
- ar9170_qmap[carl9170_get_queue(ar, skb)]);
+ ar9170_qmap(carl9170_get_queue(ar, skb)));
__carl9170_tx_process_status(ar, super->s.cookie, q);
}
--
2.25.1
On Thursday, 27 August 2020 11:33:51 CEST Lee Jones wrote:
> 'ar9170_qmap' is used in some source files which include carl9170.h,
> but not all of them. A 'defined but not used' warning is thrown when
> compiling the ones which do not use it.
>
> Fixes the following W=1 kernel build warning(s)
>
> 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=]
>
> NB: Snipped - lots of these repeat
>
> Cc: Christian Lamparter <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Johannes Berg <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Suggested-by: Rasmus Villemoes <[email protected]>
> Signed-off-by: Lee Jones <[email protected]>
> ---
For what it's worth:
Acked-by: Christian Lamparter <[email protected]>