On Wed, 29 Jan 2025 18:31:35 +0100,
John Keeping wrote:
>
> 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.
Hmm, I don't get your point. The embedded IN is paired with the
external OUT. That's the intended behavior, no?
Takashi
>
>
> Regards,
> John
>
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: c8933c3f79568 ("USB: gadget: f_midi: allow a dynamic number of input and output ports")
> > > Signed-off-by: John Keeping <jkeeping@xxxxxxxxxxxxxxxxx>
> > > ---
> > > 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
> > >
> > >
