Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f1e3e2ef-c91e-bfcd-657e-d124e8cc593e@linutronix.de>
Date:   Mon, 24 Apr 2023 11:17:46 +0200
MIME-Version: 1.0
Subject: Re: [PATCH v2 03/18] wifi: rtl8xxxu: Add beacon functions
Content-Language: de-DE
To:     Ping-Ke Shih <pkshih@realtek.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc:     Jes Sorensen <Jes.Sorensen@gmail.com>,
        Kalle Valo <kvalo@kernel.org>,
        Bitterblue Smith <rtl8821cerfe2@gmail.com>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
References: <20230419100145.159191-1-martin.kaistra@linutronix.de>
 <20230419100145.159191-4-martin.kaistra@linutronix.de>
 <9849321bafb64c4c941955e27f85fa13@realtek.com>
From:   Martin Kaistra <martin.kaistra@linutronix.de>
In-Reply-To: <9849321bafb64c4c941955e27f85fa13@realtek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Am 24.04.23 um 04:29 schrieb Ping-Ke Shih:
> 
> 
>> -----Original Message-----
>> From: Martin Kaistra <martin.kaistra@linutronix.de>
>> Sent: Wednesday, April 19, 2023 6:02 PM
>> To: linux-wireless@vger.kernel.org
>> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Kalle Valo <kvalo@kernel.org>; Ping-Ke Shih
>> <pkshih@realtek.com>; Bitterblue Smith <rtl8821cerfe2@gmail.com>; Sebastian Andrzej Siewior
>> <bigeasy@linutronix.de>
>> Subject: [PATCH v2 03/18] wifi: rtl8xxxu: Add beacon functions
>>
>> Add a workqueue to update the beacon contents asynchronously and
>> implement downloading the beacon to the HW and starting beacon tx like
>> the vendor driver.
>>
>> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
> 
> [...]
> 
>> @@ -4885,6 +4903,22 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>>                  dev_dbg(dev, "Changed BASIC_RATES!\n");
>>                  rtl8xxxu_set_basic_rates(priv, bss_conf->basic_rates);
>>          }
>> +
>> +       if (changed & BSS_CHANGED_BEACON ||
>> +           (changed & BSS_CHANGED_BEACON_ENABLED &&
>> +            bss_conf->enable_beacon)) {
>> +               if (!priv->beacon_enabled) {
> 
> Is it possible to rely on bss_conf->enable_beacon only? Then, we don't need
> priv->beacon_enabled. Like
> 
> if (changed & BSS_CHANGED_BEACON_ENABLED && bss_conf->enable_beacon)
>        rtl8xxxu_start_tx_beacon(priv);
> 
> if (changed & BSS_CHANGED_BEACON)
>        schedule_work(&priv->update_beacon_work);
> 
> if (changed & BSS_CHANGED_BEACON_ENABLED && !bss_conf->enable_beacon)
>        rtl8xxxu_stop_tx_beacon(priv);

Looking at the the mac80211 code which calls ops->bss_info_changed(), this seems 
fine to me. I will implement your suggestion and do some testing.

> 
>> +                       dev_dbg(dev, "BSS_CHANGED_BEACON_ENABLED\n");
>> +                       rtl8xxxu_start_tx_beacon(priv);
>> +               }
>> +               schedule_work(&priv->update_beacon_work);
>> +       }
>> +
>> +       if (changed & BSS_CHANGED_BEACON_ENABLED && !bss_conf->enable_beacon) {
>> +               if (priv->beacon_enabled)
>> +                       rtl8xxxu_stop_tx_beacon(priv);
>> +       }
>> +
>>   error:
>>          return;
>>   }
> 
> [...]
>