This patch introduces improved error handling for the usb_submit_urb call
in the usbtv_audio_start function. Prior to this change, the function did
not handle the scenario where usb_submit_urb could fail, potentially
leading to inconsistent state and unreliable audio streaming.
Although the error addressed by this patch may not occur in the current
environment, I still suggest implementing these error handling routines
if the function is not highly time-sensitive. As the environment evolves
or the code gets reused in different contexts, there's a possibility that
these errors might occur. Addressing them now can prevent potential
debugging efforts in the future, which could be quite resource-intensive.
Signed-off-by: Haoran Liu <[email protected]>
---
drivers/media/usb/usbtv/usbtv-audio.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/usbtv/usbtv-audio.c b/drivers/media/usb/usbtv/usbtv-audio.c
index 333bd305a4f9..81d6d54fd12c 100644
--- a/drivers/media/usb/usbtv/usbtv-audio.c
+++ b/drivers/media/usb/usbtv/usbtv-audio.c
@@ -172,6 +172,7 @@ static void usbtv_audio_urb_received(struct urb *urb)
static int usbtv_audio_start(struct usbtv *chip)
{
unsigned int pipe;
+ int err;
static const u16 setup[][2] = {
/* These seem to enable the device. */
{ USBTV_BASE + 0x0008, 0x0001 },
@@ -216,7 +217,15 @@ static int usbtv_audio_start(struct usbtv *chip)
usbtv_set_regs(chip, setup, ARRAY_SIZE(setup));
usb_clear_halt(chip->udev, pipe);
- usb_submit_urb(chip->snd_bulk_urb, GFP_ATOMIC);
+ err = usb_submit_urb(chip->snd_bulk_urb, GFP_ATOMIC);
+ if (err) {
+ dev_err(&chip->udev->dev,
+ "usb_submit_urb failed: %d\n", err);
+ kfree(chip->snd_bulk_urb->transfer_buffer);
+ usb_free_urb(chip->snd_bulk_urb);
+ chip->snd_bulk_urb = NULL;
+ return err;
+ }
return 0;
--
2.17.1
Hi Haoran,
On 30/11/2023 04:37, Haoran Liu wrote:
> This patch introduces improved error handling for the usb_submit_urb call
> in the usbtv_audio_start function. Prior to this change, the function did
> not handle the scenario where usb_submit_urb could fail, potentially
> leading to inconsistent state and unreliable audio streaming.
>
> Although the error addressed by this patch may not occur in the current
> environment, I still suggest implementing these error handling routines
> if the function is not highly time-sensitive. As the environment evolves
> or the code gets reused in different contexts, there's a possibility that
> these errors might occur. Addressing them now can prevent potential
> debugging efforts in the future, which could be quite resource-intensive.
>
> Signed-off-by: Haoran Liu <[email protected]>
> ---
> drivers/media/usb/usbtv/usbtv-audio.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/usbtv/usbtv-audio.c b/drivers/media/usb/usbtv/usbtv-audio.c
> index 333bd305a4f9..81d6d54fd12c 100644
> --- a/drivers/media/usb/usbtv/usbtv-audio.c
> +++ b/drivers/media/usb/usbtv/usbtv-audio.c
> @@ -172,6 +172,7 @@ static void usbtv_audio_urb_received(struct urb *urb)
> static int usbtv_audio_start(struct usbtv *chip)
> {
> unsigned int pipe;
> + int err;
> static const u16 setup[][2] = {
> /* These seem to enable the device. */
> { USBTV_BASE + 0x0008, 0x0001 },
> @@ -216,7 +217,15 @@ static int usbtv_audio_start(struct usbtv *chip)
> usbtv_set_regs(chip, setup, ARRAY_SIZE(setup));
>
> usb_clear_halt(chip->udev, pipe);
> - usb_submit_urb(chip->snd_bulk_urb, GFP_ATOMIC);
> + err = usb_submit_urb(chip->snd_bulk_urb, GFP_ATOMIC);
> + if (err) {
> + dev_err(&chip->udev->dev,
> + "usb_submit_urb failed: %d\n", err);
> + kfree(chip->snd_bulk_urb->transfer_buffer);
> + usb_free_urb(chip->snd_bulk_urb);
> + chip->snd_bulk_urb = NULL;
> + return err;
> + }
This can be improved a bit. Add a new label in the clean up path to
do the kfree, and just go to to that label.
In the error clean up part you now need to return 'err' at the end,
so make sure you also initialize 'err' to -ENOMEM to ensure the
correct error value is returned if usb_alloc_urb fails.
It's a bit cleaner that way and avoids duplicate cleanup paths.
Regards,
Hans
>
> return 0;
>