Re: ALSA USB MIDI port names are poorly constructed and truncate iJACK names

[Takashi Iwai <tiwai@suse.de>]

On Mon, 28 Apr 2025 22:36:30 +0200,
Paul Davis wrote:
> 
> I already included the examples from before and after upthread.

You're right, I didn't check from the beginning of the thread
carefully enough.

> The tabs/spaces is my editor, not my mailer. I'm not I can be bothered
> to fix it at this point (I'm set up to edit Ardour, not the kernel).
> especially since you want the substreams renamed too. I should leave
> it with you.

OK, let me write up and submit the patch.


thanks,

Takashi

> 
> On Fri, Apr 25, 2025 at 9:10 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Fri, 25 Apr 2025 16:39:59 +0200,
> > Paul Davis wrote:
> > >
> > > No, this is not correct.
> > >
> > > The number is only used when constructing a "default jack name" as in
> > > "SHORT_DEVICE_NAME MIDI NUMBER". It is never used when the jack name
> > > is given by the manufacturer.
> > >
> > > In the case of the Midisport 2x2, for example, the manufacturer does
> > > not provide jack names, and so we create "Midisport 2x2 MIDI 1" and
> > > "Midisport 2x2 MIDI 2". You can see this in the example output I
> > > provided.
> > >
> > > If the manufacturer has provided the jack name, the code already
> > > assumed that the jack names will be unique (because otherwise things
> > > would just break on Windows and macOS, and because they bothered to
> > > provide names). And if the jack name includes the product name, even
> > > more reason to continue with this assumption.
> >
> > Ah OK, now it's clearer.  In anyway, if you give the result after your
> > patch for comparison, it'll give a better demonstration.
> >
> > But another point about the predefined port_info is still valid, I
> > suppose.  snd_usbmidi_port_info[] provides the own format strings, and
> > they are all like "%s Tone Generator", where "%s" should be filled
> > with the shortname, and not jack_name.
> >
> > So, when port_info is defined, we should skip the duplicated word
> > check.  That being said, the change can be something like:
> >
> > -- 8< --
> > --- a/sound/usb/midi.c
> > +++ b/sound/usb/midi.c
> > @@ -1885,10 +1885,18 @@ static void snd_usbmidi_init_substream(struct snd_usb_midi *umidi,
> >         }
> >
> >         port_info = find_port_info(umidi, number);
> > -       name_format = port_info ? port_info->name :
> > -               (jack_name != default_jack_name  ? "%s %s" : "%s %s %d");
> > -       snprintf(substream->name, sizeof(substream->name),
> > -                name_format, umidi->card->shortname, jack_name, number + 1);
> > +       if (port_info || jack_name == default_jack_name ||
> > +           strncmp(umidi->card->shortname, jack_name, strlen(umidi->card->shortname)) != 0) {
> > +               name_format = port_info ? port_info->name :
> > +                       (jack_name != default_jack_name  ? "%s %s" : "%s %s %d");
> > +               snprintf(substream->name, sizeof(substream->name),
> > +                        name_format, umidi->card->shortname, jack_name, number + 1);
> > +       } else {
> > +               /* The manufacturer included the iProduct name in the jack
> > +                * name, do not use both
> > +                */
> > +               strscpy(substream->name, jack_name);
> > +       }
> >
> >         *rsubstream = substream;
> >  }
> > -- 8< --
> >
> > > I realized after I sent the patch that I had run diff -uRN at the
> > > wrong level of the source tree, but I did read the doc and other than
> > > that, I think there were no other errors? I run a standard debian
> > > kernel and so am not working with git.
> >
> > Indeed, the patch level isn't right, but the biggest problem is that
> > tabs/spaces are broken (possibly due to your mailer), so it can't be
> > applied as is, unfortunately.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > > On Wed, Apr 23, 2025 at 12:45 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > >
> > > > On Tue, 22 Apr 2025 19:31:59 +0200,
> > > > Paul Davis wrote:
> > > > >
> > > > > This patch was developed using the Debian 6.12.12 kernel source, but I
> > > > > believe it works without changes for current git.
> > > > >
> > > > > The approach taken is to always look for the iProduct name
> > > > > (umidi->card->shortname) in the iJACK name (jack_name). If not found,
> > > > > the code path is unchanged. If found, a different string format is
> > > > > used to create the port name, using only the jack name. Note that
> > > > > since finding the product name in the jack name implies the jack name
> > > > > is not default_jack_name, there is never any reason to use "number" in
> > > > > constructing the port name.
> > > > >
> > > > > This generates the expected port names for several USB MIDI devices
> > > > > here, some of which have "product name in jack name" and some which do
> > > > > not:
> > > > >
> > > > > % aplaymidi -l
> > > > >  Port    Client name                      Port name
> > > > >  14:0    Midi Through                     Midi Through Port-0
> > > > >  16:0    Scarlett 18i20 USB               Scarlett 18i20 USB MIDI 1
> > > > >  36:0    Keystation 88                    Keystation 88 MIDI 1
> > > > >  40:0    Launchkey MK4 49                 Launchkey MK4 49 MIDI In
> > > > >  40:1    Launchkey MK4 49                 Launchkey MK4 49 DAW In
> > > > >  44:0    MidiSport 2x2                    MidiSport 2x2 MIDI 1
> > > > >  44:1    MidiSport 2x2                    MidiSport 2x2 MIDI 2
> > > > >
> > > > >
> > > > > Signed-off-by: Paul Davis <paul@linuxaudiosystems.com>
> > > > >
> > > > > --- midi.c.orig    2025-04-22 11:20:30.192338737 -0600
> > > > > +++ midi.c    2025-04-22 11:20:47.373155820 -0600
> > > > > @@ -1817,10 +1817,18 @@ static void snd_usbmidi_init_substream(s
> > > > >      }
> > > > >
> > > > >      port_info = find_port_info(umidi, number);
> > > > > -    name_format = port_info ? port_info->name :
> > > > > -        (jack_name != default_jack_name  ? "%s %s" : "%s %s %d");
> > > > > -    snprintf(substream->name, sizeof(substream->name),
> > > > > -         name_format, umidi->card->shortname, jack_name, number + 1);
> > > > > +
> > > > > +    if (strncmp (umidi->card->shortname, jack_name, strlen
> > > > > (umidi->card->shortname)) != 0) {
> > > > > +        name_format = port_info ? port_info->name :
> > > > > +            (jack_name != default_jack_name  ? "%s %s" : "%s %s %d");
> > > > > +        snprintf(substream->name, sizeof(substream->name),
> > > > > +                 name_format, umidi->card->shortname, jack_name, number + 1);
> > > > > +    } else {
> > > > > +        /* The manufacturer included the iProduct name in the jack
> > > > > name, do not use both */
> > > > > +        name_format = port_info ? port_info->name : "%s";
> > > > > +        snprintf(substream->name, sizeof(substream->name),
> > > > > name_format, jack_name);
> > > > > +    }
> > > >
> > > > Thanks for the patch.
> > > >
> > > > However, dropping the number suffix will break the uniqueness.
> > > > For example, in your case above, MidiSport 2x2 will have two identical
> > > > port names, I suppose?
> > > >
> > > > IMO, we should keep adding the suffix number unless the uniqueness is
> > > > guaranteed instead of blindly dropping.  Admittedly the current code
> > > > isn't in the best form and this also needs more modification, too.
> > > >
> > > > Also, the pre-defined port names defined in snd_usb_midi_port_info[]
> > > > have the format like "%s Synth", too, and those should be adjusted, as
> > > > well.
> > > >
> > > > Last but not least, after we get a final form, could you try to submit
> > > > a patch in the proper format as defined in
> > > >   Documentation/process/submitting-patches.rst?
> > > >
> > > >
> > > > thanks,
> > > >
> > > > Takashi
>