On Sun, 03 Sep 2023 17:04:47 +0200,
Christophe JAILLET wrote:
>
> Le 03/09/2023 à 16:23, Takashi Iwai a écrit :
> > On Sun, 03 Sep 2023 15:06:00 +0200,
> > Christophe JAILLET wrote:
> >>
> >> If usb_alloc_coherent() or usb_urb_ep_type_check() fail, we should release
> >> the resources previously allocated.
> >
> > Those are freed in the caller side, start_input_streams() instead.
>
> Thanks for the fast review.
>
> Hmpm, If IIUC, resources allocated *before* the ending
> "ep->num_urbs++" still need to be freed here, otherwise
> free_midi_urbs() in the caller will not free them.
>
> Do you agree?
>
> If yes, I can send v2 which would look like:
> usb_alloc_urb()
> if (err)
> return -ENOMEM
>
> usb_alloc_coherent()
> if (err) {
> usb_free_urb()
> urb = NULL
> return -ENOMEM
> }
>
> usb_urb_ep_type_check()
> if (err) {
> usb_free_coherent()
> usb_free_urb()
> urb = NULL
> return -err
> }
>
> Or, if yuo prefer, with an error handling path just like below, but
> without the final free_midi_urbs() + a comment explaining that the
> caller does this part of job instead.
Indeed. The fix would be rather a oneliner like below, though:
--- a/sound/usb/midi2.c
+++ b/sound/usb/midi2.c
@@ -265,7 +265,7 @@ static void free_midi_urbs(struct snd_usb_midi2_endpoint *ep)
if (!ep)
return;
- for (i = 0; i < ep->num_urbs; ++i) {
+ for (i = 0; i < NUM_URBS; ++i) {
ctx = &ep->urbs[i];
if (!ctx->urb)
break;
That was the intended behavior of free_midi_urbs().
Takashi
