Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     sean.wang@mediatek.com, lorenzo.bianconi@redhat.com
Cc:     Soul.Huang@mediatek.com, YN.Chen@mediatek.com,
        Leon.Yen@mediatek.com, Eric-SY.Chang@mediatek.com,
        Deren.Wu@mediatek.com, km.lin@mediatek.com,
        robin.chiu@mediatek.com, ch.yeh@mediatek.com,
        posh.sun@mediatek.com, Eric.Liang@mediatek.com,
        Stella.Chang@mediatek.com, jemele@google.com, yenlinlai@google.com,
        linux-wireless@vger.kernel.org, linux-mediatek@lists.infradead.org
References: <1620659697-12048-1-git-send-email-sean.wang@mediatek.com>
 <1620659697-12048-4-git-send-email-sean.wang@mediatek.com>
From:   Felix Fietkau <nbd@nbd.name>
Subject: Re: [PATCH 4/9] mt76: mt7921: enable deep sleep at runtime
Message-ID: <6020930d-14dd-4150-3d91-454128bb9fc7@nbd.name>
Date:   Fri, 28 May 2021 14:51:21 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <1620659697-12048-4-git-send-email-sean.wang@mediatek.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


Hi Sean,

On 2021-05-10 17:14, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> Enable the deep sleep mode with that firmware is able to trap into
> the doze state at runtime to reduce the power consumption further.
> 
> The deep sleep mode is not allowed in the STA state transition with
> the firmware to have the fast connection experience as we've done in
> the full power mode
> 
> Reviewed-by: Lorenzo Bianconi <lorenzo@kernel.org>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../wireless/mediatek/mt76/mt76_connac_mcu.c  | 20 +++++++++++++
>  .../wireless/mediatek/mt76/mt76_connac_mcu.h  |  3 ++
>  .../net/wireless/mediatek/mt76/mt7921/init.c  |  6 +++-
>  .../net/wireless/mediatek/mt76/mt7921/main.c  | 28 +++++++++++++++++--
>  4 files changed, 54 insertions(+), 3 deletions(-)
> 
>  void mt7921_unregister_device(struct mt7921_dev *dev)
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> index 39b3e769925e..5dbccbefe047 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> @@ -848,6 +848,31 @@ mt7921_sta_remove(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>  			      IEEE80211_STA_NOTEXIST);
>  }
>  
> +static int mt7921_sta_state(struct ieee80211_hw *hw,
> +			    struct ieee80211_vif *vif,
> +			    struct ieee80211_sta *sta,
> +			    enum ieee80211_sta_state old_state,
> +			    enum ieee80211_sta_state new_state)
> +{
> +	struct mt7921_dev *dev = mt7921_hw_dev(hw);
> +
> +	if (dev->pm.enable) {
> +		mt7921_mutex_acquire(dev);
> +		mt76_connac_sta_state_dp(&dev->mt76, old_state, new_state);
> +		mt7921_mutex_release(dev);
> +	}
> +
> +	if (old_state == IEEE80211_STA_AUTH &&
> +	    new_state == IEEE80211_STA_ASSOC) {
> +		return mt7921_sta_add(hw, vif, sta);
> +	} else if (old_state == IEEE80211_STA_ASSOC &&
> +		   new_state == IEEE80211_STA_AUTH) {
> +		return mt7921_sta_remove(hw, vif, sta);
> +	}
> +
> +	return 0;
> +}
> +
>  static int
>  mt7921_get_stats(struct ieee80211_hw *hw,
>  		 struct ieee80211_low_level_stats *stats)
> @@ -1191,8 +1216,7 @@ const struct ieee80211_ops mt7921_ops = {
>  	.conf_tx = mt7921_conf_tx,
>  	.configure_filter = mt7921_configure_filter,
>  	.bss_info_changed = mt7921_bss_info_changed,
> -	.sta_add = mt7921_sta_add,
> -	.sta_remove = mt7921_sta_remove,
> +	.sta_state = mt7921_sta_state,
>  	.sta_pre_rcu_remove = mt76_sta_pre_rcu_remove,
>  	.set_key = mt7921_set_key,
>  	.sta_set_decap_offload = mt7921_sta_set_decap_offload,
Unfortunately, we can't switch from sta_add/remove to .sta_state here.
When .sta_state is implemented, mac80211 assumes that the station entry
is usable after the NOTEXIST->NONE transition, while the driver state is
only created after the AUTH->ASSOC transition.

Can you please send a fix, or a replacement patch?

Thanks,

- Felix