2021-12-06 10:02:42

by Kory Maincent

[permalink] [raw]
Subject: [PATCH] ASoC: soc-core: add the driver component name to the component struc

The function fmt_single_name is using to sanitize namings, and it return
the name set to component->name.
This function is returning either the "device_name" or the
"device_name.driver_name" for i2c devices. There is no use of the component
driver name.
If a non i2c driver register two components the function will return the
same "device_name" for both components. This could cause unexpected issue,
in my case it is a debugfs error which tries to create two directory with
the same component name.

I have fixed it by prefixing the component name with the driver component
name.

Signed-off-by: Kory Maincent <[email protected]>
---
sound/soc/soc-core.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index dcf6be4c4aaa..21ff77b231b8 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2342,10 +2342,10 @@ EXPORT_SYMBOL_GPL(snd_soc_unregister_card);
* Simplify DAI link configuration by removing ".-1" from device names
* and sanitizing names.
*/
-static char *fmt_single_name(struct device *dev, int *id)
+static char *fmt_single_name(struct device *dev, const char *snd_drv_name, int *id)
{
const char *devname = dev_name(dev);
- char *found, *name;
+ char *found, *name, *tmp;
unsigned int id1, id2;

if (devname == NULL)
@@ -2380,6 +2380,14 @@ static char *fmt_single_name(struct device *dev, int *id)
*id = 0;
}

+ if (snd_drv_name != NULL) {
+ /* Add driver component name if present */
+ tmp = devm_kasprintf(dev, GFP_KERNEL, "%s.%s", snd_drv_name, name);
+ devm_kfree(dev, name);
+ name = devm_kstrdup(dev, tmp, GFP_KERNEL);
+ devm_kfree(dev, tmp);
+ }
+
return name;
}

@@ -2444,7 +2452,7 @@ struct snd_soc_dai *snd_soc_register_dai(struct snd_soc_component *component,
*/
if (legacy_dai_naming &&
(dai_drv->id == 0 || dai_drv->name == NULL)) {
- dai->name = fmt_single_name(dev, &dai->id);
+ dai->name = fmt_single_name(dev, dai_drv->name, &dai->id);
} else {
dai->name = fmt_multiple_name(dev, dai_drv);
if (dai_drv->id)
@@ -2578,7 +2586,7 @@ int snd_soc_component_initialize(struct snd_soc_component *component,
INIT_LIST_HEAD(&component->list);
mutex_init(&component->io_mutex);

- component->name = fmt_single_name(dev, &component->id);
+ component->name = fmt_single_name(dev, driver->name, &component->id);
if (!component->name) {
dev_err(dev, "ASoC: Failed to allocate name\n");
return -ENOMEM;
--
2.25.1



2021-12-06 19:34:08

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] ASoC: soc-core: add the driver component name to the component struc

On Mon, Dec 06, 2021 at 10:59:20AM +0100, Kory Maincent wrote:

> If a non i2c driver register two components the function will return the
> same "device_name" for both components. This could cause unexpected issue,
> in my case it is a debugfs error which tries to create two directory with
> the same component name.

Why is one device registering multiple components in the first place?


Attachments:
(No filename) (390.00 B)
signature.asc (488.00 B)
Download all attachments

2021-12-07 08:47:39

by Kory Maincent

[permalink] [raw]
Subject: Re: [PATCH] ASoC: soc-core: add the driver component name to the component struc

Hello Mark,

On Mon, 6 Dec 2021 19:33:58 +0000
Mark Brown <[email protected]> wrote:

> On Mon, Dec 06, 2021 at 10:59:20AM +0100, Kory Maincent wrote:
>
> > If a non i2c driver register two components the function will return the
> > same "device_name" for both components. This could cause unexpected issue,
> > in my case it is a debugfs error which tries to create two directory with
> > the same component name.
>
> Why is one device registering multiple components in the first place?

Because the sound components are more and more complex. Why they shouldn't?

It seems to be already the case:
sound/soc/codecs/cros_ec_codec.c
sound/soc/fsl/fsl_easrc.c
sound/soc/mediatek/mt*/mt*-afe-pcm.c
sound/soc/sunxi/sun4i-codec.c
sound/soc/soc-utils.c

Regards,

Köry

2021-12-07 13:08:45

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] ASoC: soc-core: add the driver component name to the component struc

On Tue, Dec 07, 2021 at 09:47:32AM +0100, K?ry Maincent wrote:
> Mark Brown <[email protected]> wrote:

> > Why is one device registering multiple components in the first place?

> Because the sound components are more and more complex. Why they shouldn't?

In what way are they more complex?

> It seems to be already the case:
> sound/soc/codecs/cros_ec_codec.c
> sound/soc/fsl/fsl_easrc.c
> sound/soc/mediatek/mt*/mt*-afe-pcm.c
> sound/soc/sunxi/sun4i-codec.c
> sound/soc/soc-utils.c

Quite a few (I think all?) of these are quite old and and are the result
of refactoring from pre-component code rather than modern drivers, it's
likely there is no concrete reason for them to behave as they do.


Attachments:
(No filename) (702.00 B)
signature.asc (488.00 B)
Download all attachments

2021-12-07 15:16:46

by Kory Maincent

[permalink] [raw]
Subject: Re: [PATCH] ASoC: soc-core: add the driver component name to the component struc

Mark,

On Tue, 7 Dec 2021 13:08:33 +0000
Mark Brown <[email protected]> wrote:

> On Tue, Dec 07, 2021 at 09:47:32AM +0100, Köry Maincent wrote:
> > Mark Brown <[email protected]> wrote:
>
> > > Why is one device registering multiple components in the first place?
>
> > Because the sound components are more and more complex. Why they shouldn't?
> >
>
> In what way are they more complex?

The sound hardware components add more and more features.

>
> > It seems to be already the case:
> > sound/soc/codecs/cros_ec_codec.c
> > sound/soc/fsl/fsl_easrc.c
> > sound/soc/mediatek/mt*/mt*-afe-pcm.c
> > sound/soc/sunxi/sun4i-codec.c
> > sound/soc/soc-utils.c
>
> Quite a few (I think all?) of these are quite old and and are the result
> of refactoring from pre-component code rather than modern drivers, it's
> likely there is no concrete reason for them to behave as they do.

I am a beginner in the kernel sound stack, alright then, the issue comes from
the drivers.

Thanks,

Regards