Re: [PATCH v2] wifi: mt76: mt7996: fix reading zeroed info->control.flags after mt76_tx_status_skb_add()

["lorenzo@kernel.org" <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]

On Jun 01, Roy Luo wrote:
> > > I mean the link_id is only corresponds to one specific flags bit of
> > > mac80211_tx_control_flags. But there are other bits that aren't
> > > handled. Wouldn't u32 flags make it more cleaner?
> >
> > Yes, I got your point, but my concern is if we need to sync link_id between
> > mt7996_tx_prepare_skb() and mt7996_mac_write_txwi(). If so, I guess it is
> > much better to pass link_id explicitly to mt7996_mac_write_txwi() since it
> > does not just depended on mac80211_tx_control_flags and I think we should
> > not duplicate the logic in mt7996_mac_write_txwi(). Got my point?
> > If in the future (not required now) we need to pass mac80211_tx_control_flags
> > to mt7996_mac_write_txwi(), we will do it easily.
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > Ryder
> > >
> > >
> 
> Lorenzo,

Hi Roy,

> 
> I got your point and IIUC the problem being addressed in this patch is that
> the link id assignment has unnecessary duplicated logic across different
> places. However, the commit tile "fix reading zeroed info->control.flags"
> seems a bit misleading to me - this patch does not fully address the problem
> where the info->control.flags is cleared by memset in tx path when its
> value might still be referenced, the field is still zeroed after
> mt76_tx_status_skb_add() and whoever reads it afterward would get
> incorrect value. With this patch, we avoid using the incorrect value for
> link id, but the root cause remains.

This patch is actually fixing both of the issues since now do not have any
leftover access to info->control.flags after running mt76_tx_status_skb_add()
in the mt7996 tx path and it is not required (according to the current mt7996
codebase) to pass the flags cached value to mt7996_mac_write_txwi(). However,
if in the future this will be necessary, I am completely fine with that.

Regards,
Lorenzo

> 
> The issue that Ryder tries to address in
> https://lore.kernel.org/all/5ecac6a9b7d29526e8438dea105b58f5487c93aa.1778521232.git.ryder.lee@mediatek.com/
> concerns the overlapping use of info->control and info->status in tx path,
> and it remains valid even with this link id fix applied. We have to be
> cautious when dealing with info->control in mt7996 tx path until the issue
> is fully resolved.
> 
> Regards,
> Roy

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]