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

[Takashi Iwai <tiwai@suse.de>]

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