On Tue, 15 Aug 2023 23:32:31 +0200, Curtis Malainey wrote:
On Tue, Aug 15, 2023 at 9:07 AM Takashi Iwai tiwai@suse.de wrote:
On Mon, 14 Aug 2023 22:20:29 +0200, Curtis Malainey wrote:
On Sun, Aug 13, 2023 at 1:08 AM Takashi Iwai tiwai@suse.de wrote:
On Wed, 09 Aug 2023 23:11:45 +0200, Curtis Malainey wrote:
And now looking back at kobj code and device code, they do refcount parent objects. Maybe the problem is in our side -- the all devices are created with the original real device as the parent, including the card_dev, while there are some dependencies among children. So, if we build up a proper tree, pci_dev -> card_dev -> ctl_dev, pcm_dev, etc, one of the problems could be solved. It's more or less similar as what I suggested initially (referring card_dev at pcm), while changing the parent would make it implicitly.
Yes I think this would be the long term proper way to go, that way parents just put children and remove their reference, then they cleanup on their own time making everyone happy. My first patch was a very lazy attempt that, if we wanted to do the right thing we would obviously have to split the structs and free functions to operate in their own release. If you have time feel free to take another swing at the patches, otherwise I won't be able to start until next week.
Now looking back at the problem again, I noticed that actually my previous comment was wrong: as default, the device dependencies aren't kept at the release time, but it's already cleared at device_del() call. The device_del() calls kobject_del() and put_device(parent). So after this moment, both device releases become independent, and it'll hit a problem if the released object has still some dependency (such as the case of card vs ctl_dev in our case).
An extra dependency to card_dev as I put in my early patch would "fix" it. But, there is yet another problem: the call of dev_free call for snd_device object with SNDRV_DEV_LOWLEVEL can happen before releasing PCM and other devices when the delayed kobj release is enabled. And, usually this callback does release the top-level resources, which might be still accessed during the other releases.
I was doing some testing late last week that confirms these fears actually. Basically the rig was opening a non-blocking PCM and just never sending data, removing the hw sound device (in this case a USB peripheral) but not closing the userspace app. As a result kobjects were released out of order with the top level "sound" being released at disconnect of the USB device, but the actual card not being released until the app closed. See annotated logs below
[ 690.528577] kobject: 'sound' (00000000266cd308): kobject_add_internal: parent: '3-9:1.0', set: '(null)' <----- plug in device [ 690.528607] kobject: 'card1' (00000000f7fa0903): kobject_add_internal: parent: 'sound', set: 'devices' [ 690.528732] kobject: 'card1' (00000000f7fa0903): kobject_uevent_env [ 690.528756] kobject: 'card1' (00000000f7fa0903): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1' [ 690.528988] kobject: 'pcmC1D0p' (0000000095ff4473): kobject_add_internal: parent: 'card1', set: 'devices' [ 690.529640] kobject: 'pcmC1D0p' (0000000095ff4473): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1/pcmC1D0p' [ 690.529778] kobject: 'pcmC1D0c' (00000000c4879d24): kobject_add_internal: parent: 'card1', set: 'devices' [ 690.530006] kobject: 'pcmC1D0c' (00000000c4879d24): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1/pcmC1D0c' [ 690.530108] kobject: 'controlC1' (00000000a0a25449): kobject_add_internal: parent: 'card1', set: 'devices' [ 690.530373] kobject: 'controlC1' (00000000a0a25449): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1/controlC1' [ 690.671889] kobject: 'card1' (00000000f7fa0903): kobject_uevent_env [ 690.671906] kobject: 'card1' (00000000f7fa0903): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1' [ 700.009244] kobject: 'controlC1' (00000000a0a25449): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1/controlC1' [ 700.010131] kobject: 'pcmC1D0p' (0000000095ff4473): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1/pcmC1D0p' [ 700.011344] kobject: 'pcmC1D0c' (00000000c4879d24): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1/pcmC1D0c' [ 700.012916] kobject: 'card1' (00000000f7fa0903): kobject_uevent_env [ 700.012951] kobject: 'card1' (00000000f7fa0903): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1' <---- start blocked playback here [ 700.013057] kobject: 'sound' (00000000266cd308): kobject_release, parent 0000000000000000 (delayed 1000) <--- unplug usb device [ 701.054221] kobject: 'sound' (00000000266cd308): kobject_cleanup, parent 0000000000000000 [ 701.054236] kobject: 'sound' (00000000266cd308): calling ktype release [ 701.054257] kobject: 'sound': free name [ 713.639843] kobject: 'card1' (00000000f7fa0903): kobject_release, parent 0000000000000000 (delayed 3000) <--- Send EOF to playback stream [ 716.669776] kobject: 'card1' (00000000f7fa0903): kobject_cleanup, parent 0000000000000000 [ 716.669810] kobject: 'card1' (00000000f7fa0903): calling ktype release [ 716.670834] kobject: 'card1': free name
So, if we tie the object resource with each struct device release, we have a lot of works:
- Add extra dependencies among device hierarchy
- Don't use card_dev refcount for managing the sync to device closes, introduce another kref instead; otherwise card_dev refcount would never reach to zero
- Fix race of devres vs card_dev release
- Move the second half part of snd_card_do_free() to the release callback of card_dev itself to sync with the top-level release
- Rewrite all SNDRV_DEV_LOWLEVEL usages to be called via card->private_free or such; maybe the only problem is hda_intel.c and hda_tegra.c that need some work at the disconnection time, and we may introduce another hook in the card object to replace that
And, at this moment, I feel that it'd be easier to go back to the early way of device management, i.e. it'll be just like your patch, managing the device object independently from the rest resources. (This means also that the way freeing the resource for hwdep and rawmidi will go back again without the embedded device, too; they also suffer from the same problem of .)
I agree, I think as a simple stopgap, my earlier patch would at least appease the test until we can figure out the best way to do some heavier work on the kobj. I think the proxy pointer for devres would also be the best short term for 3.
The change 2 and 3 above can be still applied with your change, which will fix the remaining devres-vs-card_dev problem.
I am not sure I follow the need for 2. If we broke ctl_dev out into its own memory region and structured everything as a proper tree we would have a proper cleanup and be able to use the refcounting properly.
My thought was about the devres release that does kfree() of the card while the card's card_dev release itself is still delayed. This might be a needless fear, though, as snd_card_free() should sync with the actual card_dev release.
But, splitting the release-trigger and the actual memory release could be still worth.
Once after fixing the current problem, we may work further on other stuff (e.g. item 5), so that we can switch again to the device-release model eventually later, too.
Agreed, I don't have any experience with SNDRV_DEV_LOWLEVEL but I am happy to help out here where I can.
I am going to see if I can split the release card as mentioned but also have refcount work as expected and have the release calls roll up the tree.
I quickly worked on and made a patch series. It's put in topic/dev-split branch of sound git tree https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=topic...
It'd be appreciated if you can review / test it.
Took a look and ran it through the tests
You need to apply this diff
Thanks, I'll fix it up.
diff --git a/sound/usb/media.c b/sound/usb/media.c index 6d11fedb46326..d48db6f3ae659 100644 --- a/sound/usb/media.c +++ b/sound/usb/media.c @@ -35,7 +35,7 @@ int snd_media_stream_init(struct snd_usb_substream *subs, struct snd_pcm *pcm, { struct media_device *mdev; struct media_ctl *mctl;
struct device *pcm_dev = &pcm->streams[stream].dev;
struct device *pcm_dev = pcm->streams[stream].dev; u32 intf_type; int ret = 0; u16 mixer_pad;Hammering probe and remove appears to be fine. Went 45min without issue.
Userspace holding references past hw removal appears to still be broken as sound is released while the app is still running.
-- remove usb device -- [ 4819.827476] kobject: 'controlC1' (00000000255a51c8): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1/controlC1' [ 4819.828114] kobject: 'pcmC1D0p' (00000000f0627532): kobject_uevent_env [ 4819.828145] kobject: 'pcmC1D0p' (00000000f0627532): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1/pcmC1D0p' [ 4819.828822] kobject: 'pcmC1D0c' (000000001b707a15): kobject_uevent_env [ 4819.828850] kobject: 'pcmC1D0c' (000000001b707a15): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1/pcmC1D0c' [ 4819.829405] kobject: 'card1' (000000005bce975e): kobject_uevent_env [ 4819.829428] kobject: 'card1' (000000005bce975e): fill_kobj_path: path = '/devices/pci0000:00/0000:00:14.0/usb3/3-9/3-9:1.0/sound/card1' [ 4819.829516] kobject: 'sound' (000000000bb52434): kobject_release, parent 0000000000000000 (delayed 4000) [ 4823.873625] kobject: 'sound' (000000000bb52434): kobject_cleanup, parent 0000000000000000 [ 4823.873645] kobject: 'sound' (000000000bb52434): calling ktype release [ 4823.873654] kobject: 'sound': free name
-- end app -- [ 4849.581815] kobject: 'pcmC1D0p' (00000000f0627532): kobject_release, parent 0000000000000000 (delayed 2000) [ 4849.581960] kobject: 'pcmC1D0c' (000000001b707a15): kobject_release, parent 0000000000000000 (delayed 2000) [ 4849.582626] kobject: 'card1' (000000005bce975e): kobject_release, parent 0000000000000000 (delayed 1000) [ 4850.625615] kobject: 'card1' (000000005bce975e): kobject_cleanup, parent 0000000000000000 [ 4850.625638] kobject: 'card1' (000000005bce975e): calling ktype release [ 4850.625663] kobject: 'card1': free name [ 4851.585647] kobject: 'pcmC1D0c' (000000001b707a15): kobject_cleanup, parent 0000000000000000 [ 4851.585672] kobject: 'pcmC1D0c' (000000001b707a15): calling ktype release [ 4851.585708] kobject: 'pcmC1D0c': free name [ 4851.585727] kobject: 'pcmC1D0p' (00000000f0627532): kobject_cleanup, parent 0000000000000000 [ 4851.585737] kobject: 'pcmC1D0p' (00000000f0627532): calling ktype release [ 4851.585752] kobject: 'pcmC1D0p': free name
It's the designed behavior. Those are device *files* that are deleted immediately at the disconnection while the application is still running. It's for avoiding a new application to be started after the disconnect. That is, only the device files in /dev/snd/* become invisible. Meanwhile, the already opened objects are still handled internally.
I still don't understand why you need the kref. The devices are already reference counting, why not use them? If we split them up into their own structs we could then just device_put everything on removal and let it roll up the tree with releases automatically, blocking where userspace is still holding references. I will share a patches sometime this week of what I mean. They will probably be a bit bigger blast radius but I think its what is needed here.
We want to trigger the top-level release free procedure once when all files are closed. This top-level release does put_device() of all belonging devices.
The card_dev device refcount was used for this purpose. OTOH, if we want to construct the topology of the devices until the actual deletion (i.e. keep card_dev until pcm and others are really released/deleted), the card_dev refcount will be used for managing the topology, too. So, it'll get a side-effect side-effect that the card_dev refcount won't be zero even after all files are closed (it's referred from the children).
So, it's a kind of preparation for the future.
Takashi