RE: [PATCH v4 07/11] wifi: rtw88: Add common USB chip support

[Ping-Ke Shih]

> -----Original Message-----
> From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Sent: Wednesday, November 30, 2022 4:13 PM
> To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> Cc: linux-wireless@xxxxxxxxxxxxxxx; Neo Jou <neojou@xxxxxxxxx>; Hans Ulli Kroll <linux@xxxxxxxxxxxxx>;
> Yan-Hsuan Chuang <tony0620emma@xxxxxxxxx>; Kalle Valo <kvalo@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>;
> kernel@xxxxxxxxxxxxxx; Johannes Berg <johannes@xxxxxxxxxxxxxxxx>; Alexander Hochbaum <alex@xxxxxxxxxx>;
> Da Xue <da@libre.computer>; Bernie Huang <phhuang@xxxxxxxxxxx>; Viktor Petrenko <g0000ga@xxxxxxxxx>;
> neo_jou <neo_jou@xxxxxxxxxxx>
> Subject: Re: [PATCH v4 07/11] wifi: rtw88: Add common USB chip support
> 
> On Wed, Nov 30, 2022 at 01:40:36AM +0000, Ping-Ke Shih wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > > Sent: Tuesday, November 29, 2022 6:08 PM
> > > To: linux-wireless@xxxxxxxxxxxxxxx
> > > Cc: Neo Jou <neojou@xxxxxxxxx>; Hans Ulli Kroll <linux@xxxxxxxxxxxxx>; Ping-Ke Shih
> <pkshih@xxxxxxxxxxx>;
> > > Yan-Hsuan Chuang <tony0620emma@xxxxxxxxx>; Kalle Valo <kvalo@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>;
> > > kernel@xxxxxxxxxxxxxx; Johannes Berg <johannes@xxxxxxxxxxxxxxxx>; Alexander Hochbaum
> <alex@xxxxxxxxxx>;
> > > Da Xue <da@libre.computer>; Bernie Huang <phhuang@xxxxxxxxxxx>; Viktor Petrenko <g0000ga@xxxxxxxxx>;
> > > Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; neo_jou <neo_jou@xxxxxxxxxxx>
> > > Subject: [PATCH v4 07/11] wifi: rtw88: Add common USB chip support
> > >
> > > Add the common bits and pieces to add USB support to the RTW88 driver.
> > > This is based on https://github.com/ulli-kroll/rtw88-usb.git which
> > > itself is first written by Neo Jou.
> > >
> > > Signed-off-by: neo_jou <neo_jou@xxxxxxxxxxx>
> > > Signed-off-by: Hans Ulli Kroll <linux@xxxxxxxxxxxxx>
> > > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > > ---
> >
> > > +static void rtw_usb_write_port_tx_complete(struct urb *urb)
> > > +{
> > > +	struct rtw_usb_txcb *txcb = urb->context;
> > > +	struct rtw_dev *rtwdev = txcb->rtwdev;
> > > +	struct ieee80211_hw *hw = rtwdev->hw;
> > > +	int max_iter = RTW_USB_MAX_XMITBUF_SZ;
> > > +
> > > +	while (true) {
> > > +		struct sk_buff *skb = skb_dequeue(&txcb->tx_ack_queue);
> > > +		struct ieee80211_tx_info *info;
> > > +		struct rtw_usb_tx_data *tx_data;
> > > +
> > > +		if (!skb)
> > > +			break;
> > > +
> > > +		if (!--max_iter) {
> >
> > Don't you need to free 'skb'? or you should not dequeue skb in this situation?
> 
> My first reaction here was to call skb_queue_purge(), but that is
> implemented as:
> 
> 	while ((skb = skb_dequeue(list)) != NULL)
> 		kfree_skb(skb);
> 
> So basically it brings us into the same endless loop we are trying to
> break out here.
> 
> If it was me I would just remove this check. *txcb is allocated once
> in rtw_usb_tx_agg_skb(), &txcb->tx_ack_queue is added the number of skbs
> that fit into RTW_USB_MAX_XMITBUF_SZ and here we dequeue these skbs
> again. No other code even has the pointer to add skbs to this queue
> concurrently.

Agree with you after I trace the flow again. The number of skb is limited
and all skb(s) belong to this urb must be freed in one go, or we don't
have another chance to free them. Therefore, I think we can use this
chunk of v3.

Ping-Ke