Re: [alsa-devel] [PATCH v6 3/3] sound/usb: Use Media Controller API to share media resources

On Tue, 06 Dec 2016 19:41:37 +0100, Shuah Khan wrote:

Hi Takashi,

On 12/05/2016 11:50 PM, Takashi Iwai wrote:

...

On Wed, 30 Nov 2016 23:01:16 +0100, Shuah Khan wrote:

...

--- a/sound/usb/card.c +++ b/sound/usb/card.c

(snip)

...

@@ -616,6 +617,11 @@ static int usb_audio_probe(struct usb_interface *intf, if (err < 0) goto __error;

• if (quirk && quirk->media_device) {

• /* don't want to fail when media_snd_device_create() fails */
  

• media_snd_device_create(chip, intf);
  

Note that the usb-audio driver is probed for each usb interface, and there are multiple interfaces per device. That is, usb_audio_probe() may be called multiple times per device, and at the second or the later calls, it appends the stuff onto the previously created card object.

So, you'd have to make this call also conditional (e.g. check chip->num_interfaces == 0, indicating the very first one), or allow to be called multiple times.

In the former case, it'd be better to split media_snd_device_create() and media_snd_mixer_init(). The *_device_create() needs to be called only once, while *_mixer_init() still has to be called for each time because the new mixer element may be added for each interface.

Thanks for the catch. I am able to fix this in media_snd_device_create() I made a change to do media_dev allocate only once. media_snd_mixer_init() will get called every time media_snd_device_create() is called. This way new mixers can be handled. media_snd_mixer_init() has logic to deal with mixers that are already initialized. We are good here with the following change:

@@ -272,6 +258,14 @@ int media_snd_device_create(struct snd_usb_audio *chip, struct usb_device *usbdev = interface_to_usbdev(iface); int ret;

•   /* usb-audio driver is probed for each usb interface, and
  

•    * there are multiple interfaces per device. Avoid calling
  

•    * media_device_usb_allocate() each time usb_audio_probe()
  

•    * is called. Do it only once.
  

•    */
  

•   if (chip->media_dev)
  

•           goto snd_mixer_init;
  

•   mdev = media_device_usb_allocate(usbdev, KBUILD_MODNAME);
    if (!mdev)
            return -ENOMEM;
  

@@ -291,6 +285,7 @@ int media_snd_device_create(struct snd_usb_audio *chip, /* save media device - avoid lookups */ chip->media_dev = mdev;

+snd_mixer_init: /* Create media entities for mixer and control dev */ ret = media_snd_mixer_init(chip); if (ret) {

This looks good enough, yes.

...

...

• }
• usb_chip[chip->index] = chip; chip->num_interfaces++; usb_set_intfdata(intf, chip);

@@ -672,6 +678,14 @@ static void usb_audio_disconnect(struct usb_interface *intf) list_for_each(p, &chip->midi_list) { snd_usbmidi_disconnect(p); }

• /*
  

•  * Nice to check quirk && quirk->media_device and
  

•  * then call media_snd_device_delete(). Don't have
  

•  * access to quirk here. media_snd_device_delete()
  

•  * acceses mixer_list
  

•  */
  

• media_snd_device_delete(chip);
  

... meanwhile this is OK, as it's called only once.

(BTW, is it OK to call media_* stuff while the device is still in use? The disconnect callback gets called for hot-unplug.)

Yes. All of the media_* functions that get called during run-time check for chip->media_dev or media_ctl and do nothing when these are null.

media_device itself will not be free'd until all reference are gone. When usb_audio_disconnect() happens via unbind snd_usb_audio or physical remove, media_dev sticks around until au0828 (media driver) goes away. There is handling for any user apps. that have /dev/mediaX open.

Does this sound correct? Did I miss any of your concerns?

That sounds all good, so it's safe to call there.

thanks,

Takashi