Received-SPF: pass (google.com: domain of linux-wireless+bounces-1703-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <a89202a3-0dc8-4206-82eb-eddb8b59e729@linutronix.de>
Date: Thu, 11 Jan 2024 10:23:31 +0100
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH v2] wifi: rtl8xxxu: enable channel switch support
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: <20240110103909.240514-1-martin.kaistra@linutronix.de>
 <cf31e4c5e1a04230891e5bbbff5d24ff@realtek.com>
From: Martin Kaistra <martin.kaistra@linutronix.de>
In-Reply-To: <cf31e4c5e1a04230891e5bbbff5d24ff@realtek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Am 11.01.24 um 02:21 schrieb Ping-Ke Shih:
> 
> 
>> -----Original Message-----
>> From: Martin Kaistra <martin.kaistra@linutronix.de>
>> Sent: Wednesday, January 10, 2024 6:39 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] wifi: rtl8xxxu: enable channel switch support
>>
>> The CSA countdown in the beacon frames, which are sent out by firmware,
>> needs to get updated by the driver. To achieve this, convert
>> update_beacon_work to delayed_work and schedule it with the beacon
>> interval in case CSA is active and the countdown is not complete.
>>
>> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
>> ---
>> changes v1->v2: use delayed_work instead of usleep_range
>> v1: https://lore.kernel.org/linux-wireless/20240108111103.121378-1-martin.kaistra@linutronix.de/
>>   .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h   |  2 +-
>>   .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c  | 18 ++++++++++++++----
>>   2 files changed, 15 insertions(+), 5 deletions(-)
>>
> 
> [...]
> 
>> @@ -7764,7 +7772,7 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
>>          spin_lock_init(&priv->rx_urb_lock);
>>          INIT_WORK(&priv->rx_urb_wq, rtl8xxxu_rx_urb_work);
>>          INIT_DELAYED_WORK(&priv->ra_watchdog, rtl8xxxu_watchdog_callback);
>> -       INIT_WORK(&priv->update_beacon_work, rtl8xxxu_update_beacon_work_callback);
>> +       INIT_DELAYED_WORK(&priv->update_beacon_work, rtl8xxxu_update_beacon_work_callback);
> 
> It seems like we missed cancel priv->update_beacon_work work at rtl8xxxu_stop().
> Because that isn't introduced by this patch, you can decide to do it in this
> patch or not. Also, 'struct work_struct c2hcmd_work;' has similar problem.

Makes sense to me, I will add cancel_delayed_work_sync() for a v3 of this patch. 
Should I send a second patch for cancelling c2hcmd_work?


> 
> This could be a problem theoretically, but not easy to see the problem though.
> 
> 
>>          skb_queue_head_init(&priv->c2hcmd_queue);
>>
>>          usb_set_intfdata(interface, hw);
> 
> [...]
>