Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211109164113.65981-1-benjamin@sipsolutions.net> <20211109164113.65981-5-benjamin@sipsolutions.net>
In-Reply-To: <20211109164113.65981-5-benjamin@sipsolutions.net>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 9 Nov 2021 15:25:07 -0800
Message-ID: <CABBYNZ+W61=tPNrPp2zEcFrGFimf-djPG9HgJ-ZMOtBkHge9KA@mail.gmail.com>
Subject: Re: [PATCH 4/4] Bluetooth: btusb: Signal URB errors as TX failure
To:     Benjamin Berg <benjamin@sipsolutions.net>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Benjamin Berg <bberg@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Benjamin,

On Tue, Nov 9, 2021 at 2:35 PM Benjamin Berg <benjamin@sipsolutions.net> wrote:
>
> From: Benjamin Berg <bberg@redhat.com>
>
> Call the TX failure handler when transmission of URBs fail. This is done
> both for failures to send an URB and also when the interrupt URB used to
> retrieve a response fails.
>
> This approach is sufficient to quickly deal with certain errors such as
> a device being disconnected while synchronous commands are done during
> initialization.
>
> Signed-off-by: Benjamin Berg <bberg@redhat.com>
> ---
>  drivers/bluetooth/btusb.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 75c83768c257..0c4fe89c6573 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -924,6 +924,8 @@ static void btusb_intr_complete(struct urb *urb)
>                 if (err != -EPERM && err != -ENODEV)
>                         bt_dev_err(hdev, "urb %p failed to resubmit (%d)",
>                                    urb, -err);
> +               if (err != -EPERM)
> +                       hci_tx_error(hdev, -err);
>                 usb_unanchor_urb(urb);
>         }
>  }
> @@ -967,6 +969,8 @@ static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
>                 if (err != -EPERM && err != -ENODEV)
>                         bt_dev_err(hdev, "urb %p submission failed (%d)",
>                                    urb, -err);
> +               if (err != -EPERM)
> +                       hci_tx_error(hdev, -err);
>                 usb_unanchor_urb(urb);
>         }
>
> @@ -1322,10 +1326,12 @@ static void btusb_tx_complete(struct urb *urb)
>         if (!test_bit(HCI_RUNNING, &hdev->flags))
>                 goto done;
>
> -       if (!urb->status)
> +       if (!urb->status) {
>                 hdev->stat.byte_tx += urb->transfer_buffer_length;
> -       else
> +       } else {
> +               hci_tx_error(hdev, -urb->status);
>                 hdev->stat.err_tx++;
> +       }

Looks like we are reusing the btusb_tx_complete for all endpoints but
the likes of hci_tx_error/hci_cmd_sync_cancel only applies to commands
(e.g:  alloc_ctrl_urb), perhaps there is a way to detect if this is
actually a control urb or not so we can skip this for bulk transfers,
or actually if a bulk transfer fails we may actually need to resend
depending if the error is recoverable since the bulk transfers can
actually contain fragments rather than the entire packet, but I'd
leave that for another patch since it is probably not what you are
trying to fix in this set.

>  done:
>         spin_lock_irqsave(&data->txlock, flags);
> @@ -1348,10 +1354,12 @@ static void btusb_isoc_tx_complete(struct urb *urb)
>         if (!test_bit(HCI_RUNNING, &hdev->flags))
>                 goto done;
>
> -       if (!urb->status)
> +       if (!urb->status) {
>                 hdev->stat.byte_tx += urb->transfer_buffer_length;
> -       else
> +       } else {
> +               hci_tx_error(hdev, -urb->status);
>                 hdev->stat.err_tx++;
> +       }
>
>  done:
>         kfree(urb->setup_packet);
> --
> 2.31.1
>


-- 
Luiz Augusto von Dentz