Re: [PATCH v2 0/2] Add snd_card_disconnect_sync() helper

[Takashi Iwai <tiwai@suse.de>]

On Tue, 17 Oct 2017 17:16:51 +0200,
Takashi Sakamoto wrote:
> 
> On Oct 17 2017 23:34, Takashi Iwai wrote:
> > Well, as I mentioned, a potential breakage would appear in the legacy
> > AC97 codec binding -- if it would do any hot-unplug.  But most of
> > legacy drivers work fine without my patches just because it has only
> > top-level unbind.
> 
> Yep. Recent drivers get no suffers from the issue because AC'97 is
> enough ancient, at least for rsnd.

Erm, no, what I meant is the legacy AC97 codec driver implementation,
i.e. the code in sound/pci/ac97/ac97_codec.c.  The driver using this
codec implementations are all non-ASoC.  But AFAIK most of its usage
is always with the hard-dependency, not via bus binding, so most
likely we don't see the issue in the legacy drivers, luckily or
unluckily.

> > So, the top-level hot-unplug as you tested with the dummy driver works
> > as is.  The problem is only about unbinding the middle-layer
> > component, as mentioned in the patch.  It's found mostly in ASoC, but
> > not necessarily tied only with ASoC.
> 
> Are there any driver outside of ALSA SoC part to use the feature?

If a middle-layer driver can unbind over bus or class, yes, any driver
code may suffer from the very same problem.  The soundbus of AOA
drivers likely has the same problem, for example, and I can imagine
some of media stuff has a similar problem, too.

> Actually, not. It's quite natural to assume that the feature is somehow
> specialized for ALSA SoC part even it it's in ALSA PCM core.

Please stop blaming as if it were only ASoC-specific.  It's the nature
of Linux driver model.  As long as you can unbind some partial
component, it has to sync with the rest for a proper removal.
The lack of such mechanism in ASoC is partly because ALSA core doesn't
provide the capability, and it's the very reason we're discussing
here.

> And today I have little time to review the patch. Please check
> in-reply-to field of my previous message. This is not a reply to yours.
>
> >> But I note that even if unbind works fine to shift state of sound devices
> >> into disconnected, poll(2) call to ALSA control character devices does not
> >> return (e.g. run 'amixer events'). I don't know exactly the cause yet...
> > 
> > The disconnection doesn't close the device by itself (we can't), but
> > it replaces with the dummy ops so that it never touches the driver
> > except for closing.  That is, the device is in the idle state and just
> > accepts closing.
> 
> You did misread. I just said that a call of poll(2) doesn't return.
> 
> 1599 static unsigned int snd_ctl_poll(struct file *file, poll_table * wait)
> 1600 {
> 1601         unsigned int mask;
> 1602         struct snd_ctl_file *ctl;
> 1603
> 1604         ctl = file->private_data;
> 1605         if (!ctl->subscribed)
> 1606                 return 0;
> 1607         poll_wait(file, &ctl->change_sleep, wait);
> 
> This should be awakened by below lines:
> 
> 1775 static int snd_ctl_dev_disconnect(struct snd_device *device)
> 1776 {
> 1777         struct snd_card *card = device->device_data;
> 1778         struct snd_ctl_file *ctl;
> 1779
> 1780         read_lock(&card->ctl_files_rwlock);
> 1781         list_for_each_entry(ctl, &card->ctl_files, list) {
> 1782                 wake_up(&ctl->change_sleep);
> 
> As long as I observed, it doesn't even if the above line is executed.
> (how odd...)

Hm, then it's worth to check, but it's irrelevant from the issue we're
chasing.

> I suspect to hit some other bugs such as buffer-overrun
> depending on my environment.

How can such a thing happen...?

> But it's out of my current plan and I
> didn't investigate further. That's all of what I experience.

OK, thanks for reporting.


Takashi