Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <e6c9a6fdeeaa57d73560c895046da4a58983297d.1697146767.git.yi-chia.hsieh@mediatek.com>
In-Reply-To: <e6c9a6fdeeaa57d73560c895046da4a58983297d.1697146767.git.yi-chia.hsieh@mediatek.com>
From:   James Dutton <james.dutton@gmail.com>
Date:   Sun, 15 Oct 2023 10:02:00 +0100
Message-ID: <CAAMvbhF=2kt0Ya_Oq7MkJ4fGCvbhCLn9_wTPOfdi38Vax5tO5g@mail.gmail.com>
Subject: Re: [RESEND PATCH] wifi: mt76: mt7996: fix uninitialized variable in
 parsing txfree
To:     Yi-Chia Hsieh <yi-chia.hsieh@mediatek.com>
Cc:     Felix Fietkau <nbd@nbd.name>,
        Johannes Berg <johannes.berg@intel.com>,
        Lorenzo Bianconi <lorenzo.bianconi@redhat.com>,
        Ryder Lee <ryder.lee@mediatek.com>,
        Shayne Chen <shayne.chen@mediatek.com>,
        Evelyn Tsai <evelyn.tsai@mediatek.com>,
        Money Wang <money.wang@mediatek.com>,
        Peter Chiu <chui-hao.chiu@mediatek.com>,
        Benjamin Lin <benjamin-jw.lin@mediatek.com>,
        linux-wireless@vger.kernel.org, linux-mediatek@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 12 Oct 2023 at 23:09, Yi-Chia Hsieh <yi-chia.hsieh@mediatek.com> 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 <yi-chia.hsieh@mediatek.com>
> ---
>  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                 }