Re: Applied "ASoC: core: ensure component names are unique" to the asoc tree

[Marek Szyprowski]

Hi Jerome,

On 17.02.2020 14:18, Jerome Brunet wrote:
> On Mon 17 Feb 2020 at 13:13, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote:
>> On 14.02.2020 21:56, Mark Brown wrote:
>>> The patch
>>>
>>>      ASoC: core: ensure component names are unique
>>>
>>> has been applied to the asoc tree at
>>>
>>>      https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
>>>
>>> All being well this means that it will be integrated into the linux-next
>>> tree (usually sometime in the next 24 hours) and sent to Linus during
>>> the next merge window (or sooner if it is a bug fix), however if
>>> problems are discovered then the patch may be dropped or reverted.
>>>
>>> You may get further e-mails resulting from automated or manual testing
>>> and review of the tree, please engage with people reporting problems and
>>> send followup patches addressing any issues that are reported if needed.
>>>
>>> If any updates are required or you are submitting further changes they
>>> should be sent as incremental updates against current git, existing
>>> patches will not be replaced.
>>>
>>> Please add any relevant lists and maintainers to the CCs when replying
>>> to this mail.
>>>
>>> Thanks,
>>> Mark
>>>
>>>   From b2354e4009a773c00054b964d937e1b81cb92078 Mon Sep 17 00:00:00 2001
>>> From: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>>> Date: Fri, 14 Feb 2020 14:47:04 +0100
>>> Subject: [PATCH] ASoC: core: ensure component names are unique
>>>
>>> Make sure each ASoC component is registered with a unique name.
>>> The component is derived from the device name. If a device registers more
>>> than one component, the component names will be the same.
>>>
>>> This usually brings up a warning about the debugfs directory creation of
>>> the component since directory already exists.
>>>
>>> In such case, start numbering the component of the device so the names
>>> don't collide anymore.
>>>
>>> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>>> Link: https://lore.kernel.org/r/20200214134704.342501-1-jbrunet@xxxxxxxxxxxx
>>> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
>> This patch landed in today's linux-next and I've noticed that it breaks
>> registration of VC4 DRM driver on Raspberry Pi3 boards (I've compiled
>> kernel from bcm2835_defconfig):
> I think I have an idea about what is going on. (good catch in the defer BTW)
>
> 1) Funny to see that the vc4-hdmi already registers several (3) ASoC
> components. It must have had warning about the debugfs entry before this patch.

Okay, right, before this patch there is a warning from debugfs I've 
missed (debugfs was not enabled in bcm2835_defconfig):

raspberrypi-firmware soc:firmware: Attached to firmware from 2019-07-09 
14:40
raspberrypi-clk raspberrypi-clk: CPU frequency range: min 600000000, max 
1200000000
debugfs: Directory '3f902000.hdmi' with parent 'vc4-hdmi' already present!
vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi mapping ok
vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops)
vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops)
vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops)
vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops)
vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops)
vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops)
vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops)
vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops)

> 2) Let say that vc4 hdmi driver defers after the registration of the 2nd
> component (/drivers/gpu/drm/vc4/vc4-hdmi.c:1082). The devm callback of
> the 2nd component will run first and call
> snd_soc_unregister_component(). The catch is that this function
> unregisters all the components registered by the linux device so far.
>
> 3) Then the devm callback of the dma_engine will run
> snd_dmaengine_pcm_unregister()
> (sound/soc/soc-generic-dmaengine-pcm.c:466) and the lookup will not find
> the component dmaengine component since it is already unregistered. The
> function bails out and dmaengine_pcm_release_chan() is not run.
>
> 4) Next time the hdmi driver tries to probe, the it will fail to
> allocate the DMA channel because the sysfs entry already exists.
>
> I would suspect that any driver doing:
> 1) devm_snd_dmaengine_pcm_register()
> 2) devm_snd_soc_register_component()
> could be exposed to this issue, even before this patch.
> I don't really get is triggering the issue in this change.
>
> Maybe it would be better if the devm callback of
> devm_snd_soc_register_component() unregistered only the related component.

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland