Re: [PATCH v2] mt76: refactor cc_lock locking scheme

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> On Tue, Oct 15, 2019 at 05:16:43PM +0200, Lorenzo Bianconi wrote:
> > Read busy counters not holding cc_lock spinlock since usb read can't be
> > performed in interrupt context. Move cc_active and cc_rx counters out of
> > cc_lock since they are not modified in interrupt context.
> > Grab cc_lock updating cur_cc_bss_rx in mt76_airtime_report and do not
> > hold rx_lock in mt76_update_survey.
> <snip>
> > Fixes: 168aea24f4bb ("mt76: mt76x02u: enable survey support")
> 
> I think problem was introduced currently in mt76 driver version
> that is not yet in mainline tree, so this is not right commit.
> On Linus' tree we still read registers outside of cc_lock section.

Hi Stanislaw,

yes, you are right. We should drop the Fixes tag. Thx

> 
> void mt76x02_update_channel(struct mt76_dev *mdev)
> {
> 	...
> 
>         busy = mt76_rr(dev, MT_CH_BUSY);
>         active = busy + mt76_rr(dev, MT_CH_IDLE);
> 
>         spin_lock_bh(&dev->mt76.cc_lock);
>         state->cc_busy += busy;
>         state->cc_active += active;
>         spin_unlock_bh(&dev->mt76.cc_lock);
> }
> 
> >  	if (dev->drv->drv_flags & MT_DRV_SW_RX_AIRTIME) {
> > -		spin_lock_bh(&dev->rx_lock);
> > -		spin_lock(&dev->cc_lock);
> > +		spin_lock_bh(&dev->cc_lock);
> >  		state->cc_bss_rx += dev->cur_cc_bss_rx;
> >  		dev->cur_cc_bss_rx = 0;
> > -		spin_unlock(&dev->cc_lock);
> > -		spin_unlock_bh(&dev->rx_lock);
> > +		spin_unlock_bh(&dev->cc_lock);
> 
> Why dev->rx_lock was needed before and is not needed now ?

Looking at the code I think rx_lock is not needed here since cur_cc_bss_rx is
updated without holding rx_lock in mt76_airtime_report() (we need cc_lock
there).

Regards,
Lorenzo

> 
> Stanislaw

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