Re: [PATCH 1/7] mt76x02: use mask for vifs

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Fri, Jan 25, 2019 at 11:25:46AM +0100, Lorenzo Bianconi wrote:
> > On Fri, Jan 25, 2019 at 10:02:38AM +0100, Lorenzo Bianconi wrote:
> > > > On Thu, Jan 24, 2019 at 11:20:42PM +0100, Lorenzo Bianconi wrote:
> > > > > > > > 
> > > > > > > > I guess this does not work if you add 2 vifs and then you remove the first one
> > > > > > > > (you will end up with a wrong configuration in MT_MAC_ADDR_DW{0,1}). I guess
> > > > > > > > the hw will not work well if MT_MAC_ADDR_DW{0,1} is not properly configured
> > > > > > 
> > > > > > Maybe I am missing something, but let's assume you add the interface vif0 with address
> > > > > > 00:11:22:33:44:55 (MT_MAC_ADDR_DW{0,1} will be set to 00:11:22:33:44:55) and
> > > > > > then you add vif1 with address 00:aa:bb:cc:dd:ee. If at some point you remove
> > > > > > vif0 MT_MAC_ADDR_DW{0,1} will not be properly reconfigured. The problem will
> > > > > > be more complex if you have more interfaces
> > > > 
> > > > Ok, so in remove_interface extra code can be added to implement that.
> > 
> > Something like this should address the issue you raised:
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> > index 3880caa0c64a..44b4af928a4e 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> > @@ -320,6 +320,15 @@ void mt76x02_sta_remove(struct mt76_dev *mdev, struct ieee80211_vif *vif,
> >  }
> >  EXPORT_SYMBOL_GPL(mt76x02_add_interface);
> >  
> > +static void mt76x02_setaddr_iterator(void *data, u8 *mac,
> > +                                     struct ieee80211_vif *vif)
> > +{
> > +	struct mt76x02_dev *dev = data;
> > +
> > +	if (!ether_addr_equal(dev->mt76.macaddr, vif->addr))
> > +                mt76x02_mac_setaddr(dev, vif->addr);
> 
> This will end-up with multiple configurations if we have more than two
> interfaces, right?

No. It is called only when number of vifs change from 2 to 1 . But there
should be additional check 'dev->vif_mask & BIT(mvif->idx)' since we can
have removing interface still present in mac80211.

> >  void mt76x02_remove_interface(struct ieee80211_hw *hw,
> >  			      struct ieee80211_vif *vif)
> >  {
> > @@ -328,6 +337,10 @@ void mt76x02_remove_interface(struct ieee80211_hw *hw,
> >  
> >  	mt76_txq_remove(&dev->mt76, vif->txq);
> >  	dev->vif_mask &= ~BIT(mvif->idx);
> > +
> > +	if (hweight16(dev->vif_mask) == 1)
> > +		ieee80211_iterate_interfaces(hw, 0, mt76x02_setaddr_iterator,
> > +					     dev);
> 
> I guess we have the same issue if we have more than two interfaces and we
> remove the first one that has been configured.

We can not configure more than one MAC address without using MT_MAC_ADDR_EXT
registers. If there are more than one vif, there is no point to set MT_MAC_ADDR.

> Moreover I am a little worried about tpt regressions with this patch.
> Are you sure that if you use complete different mac addresses on a multivif scenario
> you can get the same tpt on all the interfaces? Could you please provide some
> tpt results?

How exactly posted patch can cause tpt regression ?

Posted patch just add possibility to configure HW MAC address
by this:

iw dev wlan0 del
iw phy phy0 interface add wlan0 type managed addr 00:11:22:33:44:55

what is feature of mt76x2u. Patch just extend that possibly to other
mt76x02 devices and allow to remove custom mt76x2u add_interfacea
callback.

Thanks
Stanislaw