Re: [PATCH] usb: gadget: f_midi: fix MIDI Streaming descriptor lengths

[John Keeping <jkeeping@inmusicbrands.com>]

On Wed, Jan 29, 2025 at 05:40:04PM +0100, Takashi Iwai wrote:
> On Wed, 29 Jan 2025 17:05:19 +0100,
> John Keeping wrote:
> > 
> > In the two loops before setting the MIDIStreaming descriptors,
> > ms_in_desc.baAssocJackID[] has entries written for "in_ports" values and
> > ms_out_desc.baAssocJackID[] has entries written for "out_ports" values.
> > But the counts and lengths are set the other way round in the
> > descriptors.
> > 
> > Fix the descriptors so that the bNumEmbMIDIJack values and the
> > descriptor lengths match the number of entries populated in the trailing
> > arrays.
> 
> Are you sure that it's a correct change?
> 
> IIUC, the in_ports and out_ports parameters are for external IN and
> OUT jacks, where an external OUT jack is connected to an embedded IN
> jack, and an external IN jack is connected to an embedded OUT jack.

I think it depends how the in_ports and out_ports values in configfs are
interpreted.  However, the case where in_ports != out_ports has been
broken since these files were added!

Without this change, setting in_ports=4 out_ports=2 we end up with:

      Endpoint Descriptor:
        [...]
        bEndpointAddress     0x01  EP 1 OUT
        [...]
        MIDIStreaming Endpoint Descriptor:
          bLength                 8
          bDescriptorType        37
          bDescriptorSubtype      1 (Invalid)
          bNumEmbMIDIJack         4
          baAssocJackID( 0)       9
          baAssocJackID( 1)      11
          baAssocJackID( 2)       9
          baAssocJackID( 3)       0
      Endpoint Descriptor:
        [...]
        bEndpointAddress     0x81  EP 1 IN
        [...]
        MIDIStreaming Endpoint Descriptor:
          bLength                 6
          bDescriptorType        37
          bDescriptorSubtype      1 (Invalid)
          bNumEmbMIDIJack         2
          baAssocJackID( 0)       2
          baAssocJackID( 1)       4

Note that baAssocJackID values 2 and 3 on the OUT endpoint are wrong.

From the same config, the jack definitions are:

	1:  IN  External
	2:  OUT Embedded, source 1
	3:  IN  External
	4:  OUT Embedded, source 3
	5:  IN  External
	6:  OUT Embedded, source 5
	7:  IN  External
	8:  OUT Embedded, source 7

	9:  IN  Embedded
	10: OUT External, source 9
	11: IN  Embedded
	12: OUT External, source 11

So it seems that the first 2 entries in each endpoint list are correct.
For the OUT endpoint, jacks 9 and 11 are embedded IN jacks and for the
IN endpoint, jacks 2 and 4 are embedded OUT jacks.

The problem is that the OUT endpoint lists two extra invalid jack IDs
and the IN endpoint should list jacks 6 and 8 but does not.

After applying this patch, the endpoint descriptors for the same config
are:

      Endpoint Descriptor:
        [...]
        bEndpointAddress     0x01  EP 1 OUT
        [...]
        MIDIStreaming Endpoint Descriptor:
          bLength                 6
          bDescriptorType        37
          bDescriptorSubtype      1 (Invalid)
          bNumEmbMIDIJack         2
          baAssocJackID( 0)       9
          baAssocJackID( 1)      11
      Endpoint Descriptor:
        [...]
        bEndpointAddress     0x81  EP 1 IN
        [...]
        MIDIStreaming Endpoint Descriptor:
          bLength                 8
          bDescriptorType        37
          bDescriptorSubtype      1 (Invalid)
          bNumEmbMIDIJack         4
          baAssocJackID( 0)       2
          baAssocJackID( 1)       4
          baAssocJackID( 2)       6
          baAssocJackID( 3)       8

Which lists all the jack IDs where they should be.


Regards,
John

> > Cc: stable@vger.kernel.org
> > Fixes: c8933c3f79568 ("USB: gadget: f_midi: allow a dynamic number of input and output ports")
> > Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
> > ---
> >  drivers/usb/gadget/function/f_midi.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> > index 837fcdfa3840f..6cc3d86cb4774 100644
> > --- a/drivers/usb/gadget/function/f_midi.c
> > +++ b/drivers/usb/gadget/function/f_midi.c
> > @@ -1000,11 +1000,11 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
> >  	}
> >  
> >  	/* configure the endpoint descriptors ... */
> > -	ms_out_desc.bLength = USB_DT_MS_ENDPOINT_SIZE(midi->in_ports);
> > -	ms_out_desc.bNumEmbMIDIJack = midi->in_ports;
> > +	ms_out_desc.bLength = USB_DT_MS_ENDPOINT_SIZE(midi->out_ports);
> > +	ms_out_desc.bNumEmbMIDIJack = midi->out_ports;
> >  
> > -	ms_in_desc.bLength = USB_DT_MS_ENDPOINT_SIZE(midi->out_ports);
> > -	ms_in_desc.bNumEmbMIDIJack = midi->out_ports;
> > +	ms_in_desc.bLength = USB_DT_MS_ENDPOINT_SIZE(midi->in_ports);
> > +	ms_in_desc.bNumEmbMIDIJack = midi->in_ports;
> >  
> >  	/* ... and add them to the list */
> >  	endpoint_descriptor_index = i;
> > -- 
> > 2.48.1
> > 
> >