RE: [PATCH rtw-next v4] wifi: rtw88: usb: Download the reserved page synchronously

[Ping-Ke Shih <pkshih@realtek.com>]

Josef Schlehofer <pepe.schlehofer@gmail.com> wrote:

[...]

Some minor comments below.

> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index 715a2ce342d9..f747400fca12 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -396,6 +396,51 @@ static int rtw_usb_write_port(struct rtw_dev *rtwdev, u8 qsel, struct sk_buff *s
>         return ret;
>  }
> 
> +static void rtw_usb_write_port_sync_complete(struct urb *urb)
> +{
> +       complete(urb->context);
> +}
> +
> +static int rtw_usb_write_port_sync(struct rtw_dev *rtwdev, u8 qsel,
> +                                  struct sk_buff *skb)
> +{
> +       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> +       struct usb_device *usbd = rtwusb->udev;
> +       DECLARE_COMPLETION_ONSTACK(done);
> +       struct urb *urb;
> +       unsigned int pipe;
> +       int ret;
> +       int ep = qsel_to_ep(rtwusb, qsel);

In reverse X'mas tree order for three non-struct declarations. 

> +
> +       if (ep < 0)
> +               return ep;
> +
> +       pipe = usb_sndbulkpipe(usbd, rtwusb->out_ep[ep]);
> +       urb = usb_alloc_urb(0, GFP_KERNEL);
> +       if (!urb)
> +               return -ENOMEM;
> +
> +       usb_fill_bulk_urb(urb, usbd, pipe, skb->data, skb->len,
> +                         rtw_usb_write_port_sync_complete, &done);
> +       urb->transfer_flags |= URB_ZERO_PACKET;
> +       ret = usb_submit_urb(urb, GFP_KERNEL);
> +       if (ret)
> +               goto out;
> +
> +       if (wait_for_completion_timeout(&done, secs_to_jiffies(5))) {
> +               ret = urb->status;
> +       } else {
> +               /* returns after the completion handler has finished with &done */

This branch is timeout case, no? This comment is confusing me.

> +               usb_kill_urb(urb);
> +               ret = urb->status == -ENOENT ? -ETIMEDOUT : urb->status;
> +       }
> +
> +out:
> +       usb_free_urb(urb);
> +
> +       return ret;
> +}
> +
>  static bool rtw_usb_tx_agg_skb(struct rtw_usb *rtwusb, struct sk_buff_head *list)
>  {
>         struct rtw_dev *rtwdev = rtwusb->rtwdev;
> @@ -508,7 +553,7 @@ static void rtw_usb_write_port_complete(struct urb *urb)
> 
>  static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>                               struct rtw_tx_pkt_info *pkt_info,
> -                             u8 *buf)
> +                             u8 *buf, bool sync)
>  {
>         const struct rtw_chip_info *chip = rtwdev->chip;
>         struct sk_buff *skb;

I found the initializer of 'int ret = 0;' can be removed.
Please keep reverse X'mas order as well.

> @@ -530,12 +575,16 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
>         rtw_tx_fill_tx_desc(rtwdev, pkt_info, skb);
>         rtw_tx_fill_txdesc_checksum(rtwdev, pkt_info, skb->data);
> 
> -       ret = rtw_usb_write_port(rtwdev, qsel, skb,
> -                                rtw_usb_write_port_complete, skb);
> -       if (unlikely(ret)) {
> +       if (sync)
> +               ret = rtw_usb_write_port_sync(rtwdev, qsel, skb);
> +       else
> +               ret = rtw_usb_write_port(rtwdev, qsel, skb,
> +                                        rtw_usb_write_port_complete, skb);
> +       if (unlikely(ret))
>                 rtw_err(rtwdev, "failed to do USB write, ret=%d\n", ret);
> +
> +       if (ret || sync)
>                 dev_kfree_skb_any(skb);
> -       }
> 
>         return ret;
>  }