On Wed, 29 Sep 2021 18:56:32 +0200, John Keeping wrote: > > On Wed, 29 Sep 2021 17:28:57 +0200 > Takashi Iwai <tiwai@xxxxxxx> wrote: > > > On Wed, 29 Sep 2021 17:17:58 +0200, > > John Keeping wrote: > > > > > > On Wed, 29 Sep 2021 16:51:47 +0200 > > > Takashi Iwai <tiwai@xxxxxxx> wrote: > > > > > > > On Wed, 29 Sep 2021 13:36:20 +0200, > > > > John Keeping wrote: > > > > > > > > > > If the sequencer device outlives the rawmidi device, then > > > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has > > > > > freed the snd_rawmidi structure. > > > > > > > > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE. > > > > > > > > > > Keep a reference to the rawmidi device until the sequencer has been > > > > > destroyed in order to avoid this. > > > > > > > > > > Signed-off-by: John Keeping <john@xxxxxxxxxxxx> > > > > > > > > Thanks for the patch. I wonder, though, how this could be triggered. > > > > Is this the case where the connected sequencer device is being used > > > > while the sound card gets released? Or is it something else? > > > > > > I'm not sure if it's possible to trigger via the ALSA API; I haven't > > > found a route that can trigger it, but that doesn't mean there isn't > > > one :-) > > > > > > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner. > > > > Hm, then could you check whether the patch below papers over it > > instead? > > No, this patch doesn't solve it. The issue is that the effect of the > final device_put() is delayed from the time it is called and there is no > way to guarantee the ordering without ensuring the sequencer has been > destroyed before the final reference to the rawmidi device is put. > > Both of the functions involved are called from the core > device::release() hook. > > I'm using the patch below to easily check that the sequencer has been > freed before the rawmidi data. This can easily be triggered by > unplugging a USB MIDI device (it's not 100% since the kobject release > delays are random). Hm, it's strange. I suppose you're *not* using the MIDI device, right? The release path for the USB-audio driver is: usb_audio_disconnect() -> snd_card_free_when_closed() -> release_card_device() (via put_device(&card->card_dev)) -> snd_card_do_free() And here in snd_card_do_free(), the snd_device free-callback chains are called at the beginning (snd_device_free_all()). As it's executed in a reverse loop, snd_rawmidi_dev_seq_free() shall be called before snd_rawmidi_dev_free(). Since the final put_device() for the rawmidi device is called in the latter function, the device release must not happen before snd_rawmidi_dev_seq_free()... So I still wonder how the problem could be triggered at all. Even if the device object release itself is delayed, it shouldn't matter in the scenario above (as the snd_device-free-chains are already called beforehand). thanks, Takashi > > -- >8 -- > --- a/sound/core/rawmidi.c > +++ b/sound/core/rawmidi.c > @@ -1571,7 +1571,10 @@ static int snd_rawmidi_alloc_substreams(struct snd_rawmidi *rmidi, > > static void release_rawmidi_device(struct device *dev) > { > - kfree(container_of(dev, struct snd_rawmidi, dev)); > + struct snd_rawmidi *rmidi = container_of(dev, struct snd_rawmidi, dev); > + > + WARN_ON(rmidi->seq_dev); > + kfree(rmidi); > } > > /** > -- 8< -- > > > --- a/sound/core/seq/seq_ports.c > > +++ b/sound/core/seq/seq_ports.c > > @@ -415,11 +415,16 @@ static int subscribe_port(struct snd_seq_client *client, > > grp->count--; > > } > > } > > - if (err >= 0 && send_ack && client->type == USER_CLIENT) > > + if (err < 0) > > + return err; > > + > > + if (send_ack && client->type == USER_CLIENT) > > snd_seq_client_notify_subscription(port->addr.client, port->addr.port, > > info, SNDRV_SEQ_EVENT_PORT_SUBSCRIBED); > > + else if (client->type == KERNEL_CLIENT) > > + get_device(&client->data.kernel.card->card_dev); > > > > - return err; > > + return 0; > > } > > > > static int unsubscribe_port(struct snd_seq_client *client, > > @@ -439,6 +444,8 @@ static int unsubscribe_port(struct snd_seq_client *client, > > snd_seq_client_notify_subscription(port->addr.client, port->addr.port, > > info, SNDRV_SEQ_EVENT_PORT_UNSUBSCRIBED); > > module_put(port->owner); > > + if (client->type == KERNEL_CLIENT) > > + snd_card_unref(client->data.kernel.card); > > return err; > > } > > >