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) {
- }
- 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?
--- /dev/null +++ b/sound/usb/media.c
(snip)
+void media_snd_stream_delete(struct snd_usb_substream *subs) +{
- struct media_ctl *mctl = subs->media_ctl;
- if (mctl && mctl->media_dev) {
mctl->media_dev NULL check here is superfluous, as it's checked mctl->below.
Done.
struct media_device *mdev;mdev = mctl->media_dev;if (mdev && media_devnode_is_registered(mdev->devnode)) {media_devnode_remove(mctl->intf_devnode);media_device_unregister_entity(&mctl->media_entity);media_entity_cleanup(&mctl->media_entity);}kfree(mctl);subs->media_ctl = NULL;- }
+}
+int media_snd_start_pipeline(struct snd_usb_substream *subs) +{
- struct media_ctl *mctl = subs->media_ctl;
- if (mctl)
return media_snd_enable_source(mctl);- return 0;
+}
It's merely a wrapper, and media_snd_enable_source() itself checks NULL mctl. So we can replace media_snd_enable_source() with media_snd_start_pipeline().
Done.
+void media_snd_stop_pipeline(struct snd_usb_substream *subs) +{
- struct media_ctl *mctl = subs->media_ctl;
- if (mctl)
media_snd_disable_source(mctl);+}
Ditto.
Done.
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 44d178e..0e4e0640 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c
(snip)
@@ -717,10 +718,14 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, struct audioformat *fmt; int ret;
- ret = media_snd_start_pipeline(subs);
- if (ret)
return ret;It's an open question at which timing we should call media_snd_start_pipeline(). The hw_params is mostly OK, while the real timing where the stream might be started is the prepare callback. I guess we can keep as is for now.
Okay.
Also, one more thing to be considered is whether media_snd_start_pipeline() can be called multiple times. hw_params and prepare callbacks may be called multiple times. I suppose it's OK, but just to be sure.
Yes. media_snd_start_pipeline() can be called multiple times. v4l2 apps. also call start_pipeline multiple times. So we are good here.
@@ -1234,7 +1246,12 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction) subs->dsd_dop.channel = 0; subs->dsd_dop.marker = 1;
- return setup_hw_info(runtime, subs);
- ret = setup_hw_info(runtime, subs);
- if (ret == 0)
ret = media_snd_stream_init(subs, as->pcm, direction);- if (ret)
snd_usb_autosuspend(subs->stream->chip);- return ret;
This leads to the PM refcount unbalance. The call of snd_usb_autosuspend() must be in the former if block,
ret = setup_hw_info(runtime, subs); if (ret == 0) { ret = media_snd_stream_init(subs, as->pcm, direction); if (ret) snd_usb_autosuspend(subs->stream->chip); } return ret;
Done.
thanks,
Takashi
Mauro,
I will send patch v7 and include the others in the series that don't need changes to keep them grouped.
thanks, -- Shuah