2023-10-12 22:01:13

by Yi-Chia Hsieh

Subject: [PATCH] wifi: mt76: mt7996: fix uninitialized variable in parsing txfree

Fix the uninitialized variable warning in mt7996_mac_tx_free.

Fixes: 2461599f835e: "wifi: mt76: mt7996: get tx_retries and tx_failed from txfree"
Signed-off-by: Yi-Chia Hsieh <[email protected]>
---
drivers/net/wireless/mediatek/mt76/mt7996/mac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
index 04540833485f..59ab07b89087 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
@@ -1074,7 +1074,7 @@ mt7996_mac_tx_free(struct mt7996_dev *dev, void *data, int len)
struct mt76_phy *phy3 = mdev->phys[MT_BAND2];
struct mt76_txwi_cache *txwi;
struct ieee80211_sta *sta = NULL;
- struct mt76_wcid *wcid;
+ struct mt76_wcid *wcid = NULL;
LIST_HEAD(free_list);
struct sk_buff *skb, *tmp;
void *end = data + len;
--
2.39.0

2023-10-15 09:03:06

by James Dutton

Subject: Re: [RESEND PATCH] wifi: mt76: mt7996: fix uninitialized variable in parsing txfree

On Thu, 12 Oct 2023 at 23:09, Yi-Chia Hsieh <[email protected]> wrote:
>
> Fix the uninitialized variable warning in mt7996_mac_tx_free.
>
> Fixes: 2461599f835e ("wifi: mt76: mt7996: get tx_retries and tx_failed from txfree")
> Signed-off-by: Yi-Chia Hsieh <[email protected]>
> ---
> drivers/net/wireless/mediatek/mt76/mt7996/mac.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> index 04540833485f..59ab07b89087 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c
> @@ -1074,7 +1074,7 @@ mt7996_mac_tx_free(struct mt7996_dev *dev, void *data, int len)
> struct mt76_phy *phy3 = mdev->phys[MT_BAND2];
> struct mt76_txwi_cache *txwi;
> struct ieee80211_sta *sta = NULL;
> - struct mt76_wcid *wcid;
> + struct mt76_wcid *wcid = NULL;
> LIST_HEAD(free_list);
> struct sk_buff *skb, *tmp;
> void *end = data + len;
> --
> 2.39.0
>

I am curious. Setting "struct mt76_wcid *wcid=NULL;" at the top of the
function will remove this warning, but is this really the intended
behaviour?
I am thinking about what situations will wcid now be non zero at this
(HERE below) position in the code.
It will be non-zero as a result of a previous round of the loop,
instead of given a value on this round of the loop.
There are no comments in the code, so I don't know if the intention is
to use the wcid from previous rounds of the loop or not.
My guess is we should raise the initialisation of wcid =
rcu_dereference(dev->mt76.wcid
[idx]); to somewhere higher up, before the previous if...else scope.

1127 } else if (info & MT_TXFREE_INFO_HEADER) {
1128 u32 tx_retries = 0, tx_failed = 0;
1129
--> 1130 if (!wcid) <--- HERE

Uninitialized on first iteration

1131 continue;
1132
1133 tx_retries =
1134 FIELD_GET(MT_TXFREE_INFO_COUN
T, info) - 1;
1135 tx_failed = tx_retries +
1136 !!FIELD_GET(MT_TXFREE_INFO_STAT, info);
1137
1138 wcid->stats.tx_retries += tx_retries;
1139 wcid->stats.tx_failed += tx_failed;
1140 continue;
1141 }