Question about struct snd_soc_dai() :: cpu_dai->codec

Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi ALSA SoC

My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
doesn't care about "unbind/rmmod".
For example, if someone unbinded/rmmoded "Codec", Card or other modules
doesn't know about it. Thus, user can continue to use this sound card,
and kernel will be Oops.

So, I would like to solve this issue, and for this purpose,
now I'm investigating about ALSA SoC structures.
It is now super complex, and I noticed that it is having duplicated
struct members.

If possible, I would like to cleanup this issue.

During this investigating, I noticed 1 strange operation

${LINUX}/sound/soc/soc-core.c :: snd_soc_runtime_set_dai_fmt

int snd_soc_runtime_set_dai_fmt(xxx)
{
	...
	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
	if (cpu_dai->codec) {
		...
	}
	...
}

struct snd_soc_dai {
       ...
	/* parent platform/codec */
	struct snd_soc_codec *codec;
	struct snd_soc_component *component;
	...
}

According to snd_soc_dai, *codec is its "parent".
I wonder does "cpu_dai" really can have "codec" parent ??
I think this is not correct, and we can remove this operation ?

Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi ALSA SoC

My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
doesn't care about "unbind/rmmod".
For example, if someone unbinded/rmmoded "Codec", Card or other modules
doesn't know about it. Thus, user can continue to use this sound card,
and kernel will be Oops.

So, I would like to solve this issue, and for this purpose,
now I'm investigating about ALSA SoC structures.
It is now super complex, and I noticed that it is having duplicated
struct members.

If possible, I would like to cleanup this issue.

During this investigating, I noticed 1 strange operation

${LINUX}/sound/soc/soc-core.c :: snd_soc_runtime_set_dai_fmt

int snd_soc_runtime_set_dai_fmt(xxx)
{
	...
	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
	if (cpu_dai->codec) {
		...
	}
	...
}

struct snd_soc_dai {
       ...
	/* parent platform/codec */
	struct snd_soc_codec *codec;
	struct snd_soc_component *component;
	...
}

According to snd_soc_dai, *codec is its "parent".
I wonder does "cpu_dai" really can have "codec" parent ??
I think this is not correct, and we can remove this operation ?

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Vinod Koul]

On Tue, Jul 26, 2016 at 05:41:56AM +0000, Kuninori Morimoto wrote:
> 
> Hi ALSA SoC
> 
> My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
> doesn't care about "unbind/rmmod".
> For example, if someone unbinded/rmmoded "Codec", Card or other modules
> doesn't know about it. Thus, user can continue to use this sound card,
> and kernel will be Oops.

Are you sure about this? Have you tried removing a module?

During card probe, asoc will hold a reference to the component. See the
calls to try_module_get(). This will prevent from unloading under normal
cases.

-- 
~Vinod

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Vinod

> > My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
> > doesn't care about "unbind/rmmod".
> > For example, if someone unbinded/rmmoded "Codec", Card or other modules
> > doesn't know about it. Thus, user can continue to use this sound card,
> > and kernel will be Oops.
> 
> Are you sure about this? Have you tried removing a module?
>
> During card probe, asoc will hold a reference to the component. See the
> calls to try_module_get(). This will prevent from unloading under normal
> cases.

Very easy pseudo code is like this

	rtd->card	= card;
	rtd->cpu	= cpu;
	rtd->codec	= codec;
	rtd->platform	= platform

Here, card/cpu/codec/platform are SoC device/driver,
and "ALSA sound card" is using this rtd.
Now, I can unbind/rmmod each of them (for example codec).
But rtd (= ALSA sound card) doesn't know about it.
Thus, rtd will access to invalid pointer after unbind/rmmod.

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Vinod

> > My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
> > doesn't care about "unbind/rmmod".
> > For example, if someone unbinded/rmmoded "Codec", Card or other modules
> > doesn't know about it. Thus, user can continue to use this sound card,
> > and kernel will be Oops.
> 
> Are you sure about this? Have you tried removing a module?
>
> During card probe, asoc will hold a reference to the component. See the
> calls to try_module_get(). This will prevent from unloading under normal
> cases.

Very easy pseudo code is like this

	rtd->card	= card;
	rtd->cpu	= cpu;
	rtd->codec	= codec;
	rtd->platform	= platform

Here, card/cpu/codec/platform are SoC device/driver,
and "ALSA sound card" is using this rtd.
Now, I can unbind/rmmod each of them (for example codec).
But rtd (= ALSA sound card) doesn't know about it.
Thus, rtd will access to invalid pointer after unbind/rmmod.

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Iwai]

On Wed, 27 Jul 2016 05:21:11 +0200,
Vinod Koul wrote:
> 
> On Tue, Jul 26, 2016 at 05:41:56AM +0000, Kuninori Morimoto wrote:
> > 
> > Hi ALSA SoC
> > 
> > My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
> > doesn't care about "unbind/rmmod".
> > For example, if someone unbinded/rmmoded "Codec", Card or other modules
> > doesn't know about it. Thus, user can continue to use this sound card,
> > and kernel will be Oops.
> 
> Are you sure about this? Have you tried removing a module?
> 
> During card probe, asoc will hold a reference to the component. See the
> calls to try_module_get(). This will prevent from unloading under normal
> cases.

For unloading the module, yes, it should have been prevented by
managing the module refcount.  However, unbinding can't be stopped by
that.  It's a known problem.

Morimoto-san, do you see the issue really via module unloading, or is
it only via unbinding?


Takashi

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Takashi-san

Thank you for your help

> > > My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
> > > doesn't care about "unbind/rmmod".
> > > For example, if someone unbinded/rmmoded "Codec", Card or other modules
> > > doesn't know about it. Thus, user can continue to use this sound card,
> > > and kernel will be Oops.
> > 
> > Are you sure about this? Have you tried removing a module?
> > 
> > During card probe, asoc will hold a reference to the component. See the
> > calls to try_module_get(). This will prevent from unloading under normal
> > cases.
> 
> For unloading the module, yes, it should have been prevented by
> managing the module refcount.  However, unbinding can't be stopped by
> that.  It's a known problem.
> 
> Morimoto-san, do you see the issue really via module unloading, or is
> it only via unbinding?

I tried, I thought this issue was for both unload/unbind, but it was only for unbinding.
is this correct ?

	# insmod cpu driver

	> insmod home/snd-soc-rcar.ko 
	[   94.853449] asoc-simple-graph-dpcm-card asoc-simple-graph-dpcm-card.0.auto: snd-soc-dummy-dai <-> rsnd-dai.0 mapping ok
	[   94.864466] asoc-simple-graph-dpcm-card asoc-simple-graph-dpcm-card.0.auto: snd-soc-dummy-dai <-> rsnd-dai.1 mapping ok
	[   94.876047] asoc-simple-graph-dpcm-card asoc-simple-graph-dpcm-card.0.auto: ak4642-hifi <-> snd-soc-dummy-dai mapping ok
	[   94.888157] rcar_sound ec500000.sound: probed

	# check it

	> lsmod
	Module                  Size  Used by    Not tainted
	snd_soc_rcar           47695  2 
	...

	# try rmmod

	> rmmod snd_soc_rcar
	rmmod: can't unload 'snd_soc_rcar': Resource temporarily unavailable

	# try playback, it still working

	> aplay xxxx.wav

	# try unbind

	> echo ec500000.sound > /sys/devices/platform/ec500000.sound/driver/unbind 

	# sound card will die

	> aplay xxxx.wav 
	[  230.236466] Unable to handle kernel NULL pointer dereference at virtual address 00000014
	[  230.244580] pgd = eea60000
	[  230.247320] [00000014] *pgd=6e9a3835, *pte=00000000, *ppte=00000000
	[  230.253628] Internal error: Oops: 17 [#1] SMP ARM
	[  230.258335] Modules linked in: snd_soc_rcar regmap_mmio
	...
	[  230.607462] 1fe0: 00000005 bed645b0 b6e2745d b6e28b86 60000030 bed64600 0645484d 82551041
	[  230.615643] Backtrace: 
	[  230.618105] [<c02eab58>] (pinctrl_select_state) from [<c02ead14>] (pinctrl_pm_select_state+0x28/0x58)
	[  230.627330]  r9:ee814f00 r8:ee8b8b98 r7:00000000 r6:ee927900 r5:ef2ba410 r4:ee8c9000
	[  230.635131] [<c02eacec>] (pinctrl_pm_select_state) from [<c02ead64>] (pinctrl_pm_select_default_state+0x20/0x2c)
	[  230.645311]  r7:00000000 r6:ee88b200 r5:ef378800 r4:ee8c9000
	[  230.651016] [<c02ead44>] (pinctrl_pm_select_default_state) from [<c040ab90>] (soc_pcm_open+0x34/0x798)
	[  230.660331] [<c040ab5c>] (soc_pcm_open) from [<c040c0e4>] (dpcm_fe_dai_open+0xbc/0x524)
	[  230.668340]  r10:ee8c9000 r9:ee8b8bb0 r8:ee8b8b98 r7:00000280 r6:ee8c9000 r5:00000001
	[  230.676222]  r4:ef378800
	...

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Takashi-san

Thank you for your help

> > > My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
> > > doesn't care about "unbind/rmmod".
> > > For example, if someone unbinded/rmmoded "Codec", Card or other modules
> > > doesn't know about it. Thus, user can continue to use this sound card,
> > > and kernel will be Oops.
> > 
> > Are you sure about this? Have you tried removing a module?
> > 
> > During card probe, asoc will hold a reference to the component. See the
> > calls to try_module_get(). This will prevent from unloading under normal
> > cases.
> 
> For unloading the module, yes, it should have been prevented by
> managing the module refcount.  However, unbinding can't be stopped by
> that.  It's a known problem.
> 
> Morimoto-san, do you see the issue really via module unloading, or is
> it only via unbinding?

I tried, I thought this issue was for both unload/unbind, but it was only for unbinding.
is this correct ?

	# insmod cpu driver

	> insmod home/snd-soc-rcar.ko 
	[   94.853449] asoc-simple-graph-dpcm-card asoc-simple-graph-dpcm-card.0.auto: snd-soc-dummy-dai <-> rsnd-dai.0 mapping ok
	[   94.864466] asoc-simple-graph-dpcm-card asoc-simple-graph-dpcm-card.0.auto: snd-soc-dummy-dai <-> rsnd-dai.1 mapping ok
	[   94.876047] asoc-simple-graph-dpcm-card asoc-simple-graph-dpcm-card.0.auto: ak4642-hifi <-> snd-soc-dummy-dai mapping ok
	[   94.888157] rcar_sound ec500000.sound: probed

	# check it

	> lsmod
	Module                  Size  Used by    Not tainted
	snd_soc_rcar           47695  2 
	...

	# try rmmod

	> rmmod snd_soc_rcar
	rmmod: can't unload 'snd_soc_rcar': Resource temporarily unavailable

	# try playback, it still working

	> aplay xxxx.wav

	# try unbind

	> echo ec500000.sound > /sys/devices/platform/ec500000.sound/driver/unbind 

	# sound card will die

	> aplay xxxx.wav 
	[  230.236466] Unable to handle kernel NULL pointer dereference at virtual address 00000014
	[  230.244580] pgd = eea60000
	[  230.247320] [00000014] *pgd=6e9a3835, *pte=00000000, *ppte=00000000
	[  230.253628] Internal error: Oops: 17 [#1] SMP ARM
	[  230.258335] Modules linked in: snd_soc_rcar regmap_mmio
	...
	[  230.607462] 1fe0: 00000005 bed645b0 b6e2745d b6e28b86 60000030 bed64600 0645484d 82551041
	[  230.615643] Backtrace: 
	[  230.618105] [<c02eab58>] (pinctrl_select_state) from [<c02ead14>] (pinctrl_pm_select_state+0x28/0x58)
	[  230.627330]  r9:ee814f00 r8:ee8b8b98 r7:00000000 r6:ee927900 r5:ef2ba410 r4:ee8c9000
	[  230.635131] [<c02eacec>] (pinctrl_pm_select_state) from [<c02ead64>] (pinctrl_pm_select_default_state+0x20/0x2c)
	[  230.645311]  r7:00000000 r6:ee88b200 r5:ef378800 r4:ee8c9000
	[  230.651016] [<c02ead44>] (pinctrl_pm_select_default_state) from [<c040ab90>] (soc_pcm_open+0x34/0x798)
	[  230.660331] [<c040ab5c>] (soc_pcm_open) from [<c040c0e4>] (dpcm_fe_dai_open+0xbc/0x524)
	[  230.668340]  r10:ee8c9000 r9:ee8b8bb0 r8:ee8b8b98 r7:00000280 r6:ee8c9000 r5:00000001
	[  230.676222]  r4:ef378800
	...

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Vinod Koul]

On Wed, Jul 27, 2016 at 07:57:05AM +0200, Takashi Iwai wrote:
> On Wed, 27 Jul 2016 05:21:11 +0200,
> Vinod Koul wrote:
> > 
> > On Tue, Jul 26, 2016 at 05:41:56AM +0000, Kuninori Morimoto wrote:
> > > 
> > > Hi ALSA SoC
> > > 
> > > My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
> > > doesn't care about "unbind/rmmod".
> > > For example, if someone unbinded/rmmoded "Codec", Card or other modules
> > > doesn't know about it. Thus, user can continue to use this sound card,
> > > and kernel will be Oops.
> > 
> > Are you sure about this? Have you tried removing a module?
> > 
> > During card probe, asoc will hold a reference to the component. See the
> > calls to try_module_get(). This will prevent from unloading under normal
> > cases.
> 
> For unloading the module, yes, it should have been prevented by
> managing the module refcount.  However, unbinding can't be stopped by
> that.  It's a known problem.

Oh yes, unload is an issue. Are these any solutions to prevent this?

In core, should we de-register the card if one of the components exits. The
.remove should be called for the driver, thus triggering unregister?

-- 
~Vinod

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]

On Wed, Jul 27, 2016 at 10:51:26PM +0530, Vinod Koul wrote:
> On Wed, Jul 27, 2016 at 07:57:05AM +0200, Takashi Iwai wrote:

> > For unloading the module, yes, it should have been prevented by
> > managing the module refcount.  However, unbinding can't be stopped by
> > that.  It's a known problem.

> Oh yes, unload is an issue. Are these any solutions to prevent this?

> In core, should we de-register the card if one of the components exits. The
> .remove should be called for the driver, thus triggering unregister?

That's the theory but it's full of holes at the minute.  Someone needs
to sit down and fix all the holes in there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Iwai]

On Wed, 27 Jul 2016 20:04:56 +0200,
Mark Brown wrote:
> 
> On Wed, Jul 27, 2016 at 10:51:26PM +0530, Vinod Koul wrote:
> > On Wed, Jul 27, 2016 at 07:57:05AM +0200, Takashi Iwai wrote:
> 
> > > For unloading the module, yes, it should have been prevented by
> > > managing the module refcount.  However, unbinding can't be stopped by
> > > that.  It's a known problem.
> 
> > Oh yes, unload is an issue. Are these any solutions to prevent this?
> 
> > In core, should we de-register the card if one of the components exits. The
> > .remove should be called for the driver, thus triggering unregister?
> 
> That's the theory but it's full of holes at the minute.  Someone needs
> to sit down and fix all the holes in there.

Yeah, and it's not so trivial.  The unbind can be called at any time,
even during a stream is running.  This is a kind of hot unplug.  The
whole nightmare we've faced with usb-audio and co will strike again.

I'm wondering whether it's a better option to block the unbind
behavior, either in driver base (allowing to return an error) or in
the sound side (waiting in remove() until the sane point). 


Takashi

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:

> I'm wondering whether it's a better option to block the unbind
> behavior, either in driver base (allowing to return an error) or in
> the sound side (waiting in remove() until the sane point). 

That's certainly going to be a lot easier and part of the reason it's
never been looked at much is that (unlike USB) there's very little
reason why an ASoC sound card would ever be hotplugged - even in
development these days the normal development flow involves rebooting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Iwai]

On Wed, 27 Jul 2016 20:22:33 +0200,
Mark Brown wrote:
> 
> On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
> 
> > I'm wondering whether it's a better option to block the unbind
> > behavior, either in driver base (allowing to return an error) or in
> > the sound side (waiting in remove() until the sane point). 
> 
> That's certainly going to be a lot easier and part of the reason it's
> never been looked at much is that (unlike USB) there's very little
> reason why an ASoC sound card would ever be hotplugged - even in
> development these days the normal development flow involves rebooting.

Actually there is already the suppress_bind_attr flag in struct
device_driver.  For a simple platform driver like snd-soc-rcar, it's
easy like:

diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 3351a701c60e..d019824927de 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -1251,6 +1251,7 @@ static struct platform_driver rsnd_driver = {
 	.driver	= {
 		.name	= "rcar_sound",
 		.of_match_table = rsnd_of_match,
+		.suppress_bind_attrs = true,
 	},
 	.probe		= rsnd_probe,
 	.remove		= rsnd_remove,

Then there will be no sysfs bind/unbind for this driver.
(Note: totally untested: let me know if it really works.)

The same technique is likely available for i2c and spi codec drivers.
But it's another open question whether we should suppress the sysfs
bind/unbind of these devices at all.  My gut feeling is that sysfs
bind/unbind are mostly useless for drivers like ASoC codecs.  At
least, it would be much safer to disable for now.


Takashi

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Vinod Koul]

On Wed, Jul 27, 2016 at 10:22:41PM +0200, Takashi Iwai wrote:
> On Wed, 27 Jul 2016 20:22:33 +0200,
> Mark Brown wrote:
> > 
> > On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
> > 
> > > I'm wondering whether it's a better option to block the unbind
> > > behavior, either in driver base (allowing to return an error) or in
> > > the sound side (waiting in remove() until the sane point). 
> > 
> > That's certainly going to be a lot easier and part of the reason it's
> > never been looked at much is that (unlike USB) there's very little
> > reason why an ASoC sound card would ever be hotplugged - even in
> > development these days the normal development flow involves rebooting.

I agree, it makese no sense for devices to be hotplugged. And for
developement flows people can do rmmond and insmod. That works fine!

> Actually there is already the suppress_bind_attr flag in struct
> device_driver.  For a simple platform driver like snd-soc-rcar, it's
> easy like:

I like this idea, should we do this per driver or in core? I think we should
let drivers decide..

> diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
> index 3351a701c60e..d019824927de 100644
> --- a/sound/soc/sh/rcar/core.c
> +++ b/sound/soc/sh/rcar/core.c
> @@ -1251,6 +1251,7 @@ static struct platform_driver rsnd_driver = {
>  	.driver	= {
>  		.name	= "rcar_sound",
>  		.of_match_table = rsnd_of_match,
> +		.suppress_bind_attrs = true,
>  	},
>  	.probe		= rsnd_probe,
>  	.remove		= rsnd_remove,
> 
> Then there will be no sysfs bind/unbind for this driver.
> (Note: totally untested: let me know if it really works.)
> 
> The same technique is likely available for i2c and spi codec drivers.
> But it's another open question whether we should suppress the sysfs
> bind/unbind of these devices at all.  My gut feeling is that sysfs
> bind/unbind are mostly useless for drivers like ASoC codecs.  At
> least, it would be much safer to disable for now.

-- 
~Vinod

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Lars-Peter Clausen]

On 07/26/2016 07:41 AM, Kuninori Morimoto wrote:
> 
> Hi ALSA SoC
> 
> My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform)
> doesn't care about "unbind/rmmod".
> For example, if someone unbinded/rmmoded "Codec", Card or other modules
> doesn't know about it. Thus, user can continue to use this sound card,
> and kernel will be Oops.
> 
> So, I would like to solve this issue, and for this purpose,
> now I'm investigating about ALSA SoC structures.
> It is now super complex, and I noticed that it is having duplicated
> struct members.
> 
> If possible, I would like to cleanup this issue.
> 
> During this investigating, I noticed 1 strange operation
> 
> ${LINUX}/sound/soc/soc-core.c :: snd_soc_runtime_set_dai_fmt
> 
> int snd_soc_runtime_set_dai_fmt(xxx)
> {
> 	...
> 	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
> 	if (cpu_dai->codec) {
> 		...
> 	}
> 	...
> }
> 
> struct snd_soc_dai {
>        ...
> 	/* parent platform/codec */
> 	struct snd_soc_codec *codec;
> 	struct snd_soc_component *component;
> 	...
> }
> 
> According to snd_soc_dai, *codec is its "parent".
> I wonder does "cpu_dai" really can have "codec" parent ??
> I think this is not correct, and we can remove this operation ?

This is for CODEC<->CODEC links where no CPU DAI is involved and the
"CPU" side of the DAI link is actually a CODEC.

Ideally we'd make the DAI links agnostic to what is connected, e.g. it
shouldn't matter what type is connected to what side. But that does not
work since things are anti-symmetric between CPU and CODEC DAIs. On
CODEC DAIs the capture stream is output, on CPU DAIs the capture stream
is input, similar thing for playback. So we need to know whether the DAI
is a CPU DAI or a CODEC DAI.

Fixing this at this point is near to impossible since it requires
invasive changes to all existing drivers. So we need this code and can't
drop it.

The best we can do is try to come up with a generic interface that is
DAI type independent and recommend this interface for new drivers.

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Lars-Peter Clausen]

On 07/28/2016 05:46 AM, Vinod Koul wrote:
> On Wed, Jul 27, 2016 at 10:22:41PM +0200, Takashi Iwai wrote:
>> On Wed, 27 Jul 2016 20:22:33 +0200,
>> Mark Brown wrote:
>>>
>>> On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
>>>
>>>> I'm wondering whether it's a better option to block the unbind
>>>> behavior, either in driver base (allowing to return an error) or in
>>>> the sound side (waiting in remove() until the sane point). 
>>>
>>> That's certainly going to be a lot easier and part of the reason it's
>>> never been looked at much is that (unlike USB) there's very little
>>> reason why an ASoC sound card would ever be hotplugged - even in
>>> development these days the normal development flow involves rebooting.
> 
> I agree, it makese no sense for devices to be hotplugged. And for
> developement flows people can do rmmond and insmod. That works fine!

I don't agree. In my opinion hot-plug is an essential feature of a
modern device driver framework and if ASoC wants to claim to fall in
this category we ought to support it. Hotplug is something that always
pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on
a hot-pluggable device (maybe USB) we don't want to duplicate the code,
but be able to reuse.

One area that e.g. requires hot-plug/-unplug and were ASoC supported
devices are used is embedded development boards that support shields and
devicetree overlays. Like e.g. RPI and similar.

The reason why we don't support hot-unplug in ASoC at the moment is
because it is not trivial to implement and nobody has cared enough yet.

But if somebody wants to and has the resources to implement this we
should not discourage this.

I'd very much prefer to have proper hot-plug support instead of
prohibiting unbinding even on systems that do not require hot-plug
support normally. It's a much cleaner solution.

- Lars

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Iwai]

On Thu, 28 Jul 2016 22:33:31 +0200,
Lars-Peter Clausen wrote:
> 
> On 07/28/2016 05:46 AM, Vinod Koul wrote:
> > On Wed, Jul 27, 2016 at 10:22:41PM +0200, Takashi Iwai wrote:
> >> On Wed, 27 Jul 2016 20:22:33 +0200,
> >> Mark Brown wrote:
> >>>
> >>> On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
> >>>
> >>>> I'm wondering whether it's a better option to block the unbind
> >>>> behavior, either in driver base (allowing to return an error) or in
> >>>> the sound side (waiting in remove() until the sane point). 
> >>>
> >>> That's certainly going to be a lot easier and part of the reason it's
> >>> never been looked at much is that (unlike USB) there's very little
> >>> reason why an ASoC sound card would ever be hotplugged - even in
> >>> development these days the normal development flow involves rebooting.
> > 
> > I agree, it makese no sense for devices to be hotplugged. And for
> > developement flows people can do rmmond and insmod. That works fine!
> 
> I don't agree. In my opinion hot-plug is an essential feature of a
> modern device driver framework and if ASoC wants to claim to fall in
> this category we ought to support it. Hotplug is something that always
> pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on
> a hot-pluggable device (maybe USB) we don't want to duplicate the code,
> but be able to reuse.
> 
> One area that e.g. requires hot-plug/-unplug and were ASoC supported
> devices are used is embedded development boards that support shields and
> devicetree overlays. Like e.g. RPI and similar.
> 
> The reason why we don't support hot-unplug in ASoC at the moment is
> because it is not trivial to implement and nobody has cared enough yet.
> 
> But if somebody wants to and has the resources to implement this we
> should not discourage this.
> 
> I'd very much prefer to have proper hot-plug support instead of
> prohibiting unbinding even on systems that do not require hot-plug
> support normally. It's a much cleaner solution.

Well, but hot-unplugging only a component like codec would be needed
in a real scenario?  It's a different story from the hotplug in the
card level.


Takashi

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Lars-Peter Clausen]

On 07/28/2016 10:42 PM, Takashi Iwai wrote:
> On Thu, 28 Jul 2016 22:33:31 +0200,
> Lars-Peter Clausen wrote:
>>
>> On 07/28/2016 05:46 AM, Vinod Koul wrote:
>>> On Wed, Jul 27, 2016 at 10:22:41PM +0200, Takashi Iwai wrote:
>>>> On Wed, 27 Jul 2016 20:22:33 +0200,
>>>> Mark Brown wrote:
>>>>>
>>>>> On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
>>>>>
>>>>>> I'm wondering whether it's a better option to block the unbind
>>>>>> behavior, either in driver base (allowing to return an error) or in
>>>>>> the sound side (waiting in remove() until the sane point). 
>>>>>
>>>>> That's certainly going to be a lot easier and part of the reason it's
>>>>> never been looked at much is that (unlike USB) there's very little
>>>>> reason why an ASoC sound card would ever be hotplugged - even in
>>>>> development these days the normal development flow involves rebooting.
>>>
>>> I agree, it makese no sense for devices to be hotplugged. And for
>>> developement flows people can do rmmond and insmod. That works fine!
>>
>> I don't agree. In my opinion hot-plug is an essential feature of a
>> modern device driver framework and if ASoC wants to claim to fall in
>> this category we ought to support it. Hotplug is something that always
>> pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on
>> a hot-pluggable device (maybe USB) we don't want to duplicate the code,
>> but be able to reuse.
>>
>> One area that e.g. requires hot-plug/-unplug and were ASoC supported
>> devices are used is embedded development boards that support shields and
>> devicetree overlays. Like e.g. RPI and similar.
>>
>> The reason why we don't support hot-unplug in ASoC at the moment is
>> because it is not trivial to implement and nobody has cared enough yet.
>>
>> But if somebody wants to and has the resources to implement this we
>> should not discourage this.
>>
>> I'd very much prefer to have proper hot-plug support instead of
>> prohibiting unbinding even on systems that do not require hot-plug
>> support normally. It's a much cleaner solution.
> 
> Well, but hot-unplugging only a component like codec would be needed
> in a real scenario?  It's a different story from the hotplug in the
> card level.

With overlays I'm not sure if we can control the remove order. So the
component might be removed before the card complex.

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

On Thu, Jul 28, 2016 at 10:33:31PM +0200, Lars-Peter Clausen wrote:
> On 07/28/2016 05:46 AM, Vinod Koul wrote:

> > I agree, it makese no sense for devices to be hotplugged. And for
> > developement flows people can do rmmond and insmod. That works fine!

> I don't agree. In my opinion hot-plug is an essential feature of a
> modern device driver framework and if ASoC wants to claim to fall in
> this category we ought to support it. Hotplug is something that always
> pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on
> a hot-pluggable device (maybe USB) we don't want to duplicate the code,
> but be able to reuse.

Right, so there's two bits to hotplug - there's hotplugging individual
components separately to the card and there's hotplugging cards en
masse including some of their components.  The latter case definitely
does make sense and should have a reasonable chance of working already.
Hotplugging individual components is much more of a nice to have, though
as you say if someone wants to implement it that's obviously not a
problem.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Sakamoto]

Lars,

On Jul 29 2016 05:33, Lars-Peter Clausen wrote:
> Hotplug is something that always pops up sooner or later. E.g. if someone
> puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we
> don't want to duplicate the code, but be able to reuse.

(A bit to sidetrack)

To me, it's unclear for devices on USB. When ALSA SoC part supports 
these devices, what is the scenario you assumed? In short, assuming we 
put codes to ALSA SoC part, what is the shape of the corresponding 
devices and links of pairs of endpoints? Additionally, in this case, 
what codes are able to be reused?


Regards

Takashi Sakamoto

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Lars

Thank you for your feedback

> > int snd_soc_runtime_set_dai_fmt(xxx)
> > {
> > 	...
> > 	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
> > 	if (cpu_dai->codec) {
> > 		...
> > 	}
> > 	...
> > }
(snip)
> This is for CODEC<->CODEC links where no CPU DAI is involved and the
> "CPU" side of the DAI link is actually a CODEC.

Wow !!

> Ideally we'd make the DAI links agnostic to what is connected, e.g. it
> shouldn't matter what type is connected to what side. But that does not
> work since things are anti-symmetric between CPU and CODEC DAIs. On
> CODEC DAIs the capture stream is output, on CPU DAIs the capture stream
> is input, similar thing for playback. So we need to know whether the DAI
> is a CPU DAI or a CODEC DAI.
> 
> Fixing this at this point is near to impossible since it requires
> invasive changes to all existing drivers. So we need this code and can't
> drop it.
> 
> The best we can do is try to come up with a generic interface that is
> DAI type independent and recommend this interface for new drivers.

Hmm...
OK, now, I'm investigating ALSA SoC struct connection.
And yes, existing code has deep relationship each other.
Thus, fixing/droping seems very dificult...
I think each struct has random pointer connection which doesn't care
about ALSA SoC's hierarchy.

I would like to indicate my opinion here.
My strange point are...

1. snd_soc_pcm_runtime has pointer to Card/Codec/Platform, but no CPU
2. snd_soc_dai has pointer to [component] and [Codec]
   [component] is its parent, [Codec] is maybe Hack
3. [component] has pointer to [card] and [Codec]
   [card] is understandable, [Codec] is maybe Hack

It seems many struct would like to access to [Codec], thus, each struct has
codec pointer (as hack ?), but it is ignoring ALSA SoC hierarchy.

It will be more clear if...

IDEA 1

   Each component (= CPU/Codec/Platform) has pointer to Card now.
   How about Card has pointer to each component ?
   if so, we can access to every struct.
   Card -> CPU/Codec/Platform, CPU/Codec/Platform -> Card
   Then, we want to have component related macro/flag

	#define component_is_cpu(component) (component->flag & COMPONENT_CPU)
	#define component_is_codec(component) (component->flag & COMPONENT_CODEC)
	#define component_is_platform(component) (component->flag & COMPONENT_PLATFORM)

	#define component_to_cpu() container_of(xxx)
	#define component_to_codec() container_of(xxx)
	#define component_to_platorm() container_of(xxx)

IDEA 2

   if IDEA1 was OK, snd_soc_pcm_runtime already has card pointer.
   This means snd_soc_pcm_runtime can access to every component struct.

   snd_soc_dai has its parent component.
   This means snd_soc_dai can access to Card, and Card can access to every component.
   And then, cpu_dai->codec has no issue if we can use component_is_xxx() macro ?

    - if (cpu_dai->codec)
    + if (component_is_codec(cpu_dai->component))

If IDEA1 / IDEA2 are OK, we can cleanup each struct,
especially hack-able random pointer ?

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Lars

Thank you for your feedback

> > int snd_soc_runtime_set_dai_fmt(xxx)
> > {
> > 	...
> > 	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
> > 	if (cpu_dai->codec) {
> > 		...
> > 	}
> > 	...
> > }
(snip)
> This is for CODEC<->CODEC links where no CPU DAI is involved and the
> "CPU" side of the DAI link is actually a CODEC.

Wow !!

> Ideally we'd make the DAI links agnostic to what is connected, e.g. it
> shouldn't matter what type is connected to what side. But that does not
> work since things are anti-symmetric between CPU and CODEC DAIs. On
> CODEC DAIs the capture stream is output, on CPU DAIs the capture stream
> is input, similar thing for playback. So we need to know whether the DAI
> is a CPU DAI or a CODEC DAI.
> 
> Fixing this at this point is near to impossible since it requires
> invasive changes to all existing drivers. So we need this code and can't
> drop it.
> 
> The best we can do is try to come up with a generic interface that is
> DAI type independent and recommend this interface for new drivers.

Hmm...
OK, now, I'm investigating ALSA SoC struct connection.
And yes, existing code has deep relationship each other.
Thus, fixing/droping seems very dificult...
I think each struct has random pointer connection which doesn't care
about ALSA SoC's hierarchy.

I would like to indicate my opinion here.
My strange point are...

1. snd_soc_pcm_runtime has pointer to Card/Codec/Platform, but no CPU
2. snd_soc_dai has pointer to [component] and [Codec]
   [component] is its parent, [Codec] is maybe Hack
3. [component] has pointer to [card] and [Codec]
   [card] is understandable, [Codec] is maybe Hack

It seems many struct would like to access to [Codec], thus, each struct has
codec pointer (as hack ?), but it is ignoring ALSA SoC hierarchy.

It will be more clear if...

IDEA 1

   Each component (= CPU/Codec/Platform) has pointer to Card now.
   How about Card has pointer to each component ?
   if so, we can access to every struct.
   Card -> CPU/Codec/Platform, CPU/Codec/Platform -> Card
   Then, we want to have component related macro/flag

	#define component_is_cpu(component) (component->flag & COMPONENT_CPU)
	#define component_is_codec(component) (component->flag & COMPONENT_CODEC)
	#define component_is_platform(component) (component->flag & COMPONENT_PLATFORM)

	#define component_to_cpu() container_of(xxx)
	#define component_to_codec() container_of(xxx)
	#define component_to_platorm() container_of(xxx)

IDEA 2

   if IDEA1 was OK, snd_soc_pcm_runtime already has card pointer.
   This means snd_soc_pcm_runtime can access to every component struct.

   snd_soc_dai has its parent component.
   This means snd_soc_dai can access to Card, and Card can access to every component.
   And then, cpu_dai->codec has no issue if we can use component_is_xxx() macro ?

    - if (cpu_dai->codec)
    + if (component_is_codec(cpu_dai->component))

If IDEA1 / IDEA2 are OK, we can cleanup each struct,
especially hack-able random pointer ?

Best regards
---
Kuninori Morimoto

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Lars-Peter Clausen]

On 07/29/2016 04:24 AM, Kuninori Morimoto wrote:
> 
> Hi Lars
> 
> Thank you for your feedback
> 
>>> int snd_soc_runtime_set_dai_fmt(xxx)
>>> {
>>> 	...
>>> 	/* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */
>>> 	if (cpu_dai->codec) {
>>> 		...
>>> 	}
>>> 	...
>>> }
> (snip)
>> This is for CODEC<->CODEC links where no CPU DAI is involved and the
>> "CPU" side of the DAI link is actually a CODEC.
> 
> Wow !!
> 
>> Ideally we'd make the DAI links agnostic to what is connected, e.g. it
>> shouldn't matter what type is connected to what side. But that does not
>> work since things are anti-symmetric between CPU and CODEC DAIs. On
>> CODEC DAIs the capture stream is output, on CPU DAIs the capture stream
>> is input, similar thing for playback. So we need to know whether the DAI
>> is a CPU DAI or a CODEC DAI.
>>
>> Fixing this at this point is near to impossible since it requires
>> invasive changes to all existing drivers. So we need this code and can't
>> drop it.
>>
>> The best we can do is try to come up with a generic interface that is
>> DAI type independent and recommend this interface for new drivers.
> 
> Hmm...
> OK, now, I'm investigating ALSA SoC struct connection.
> And yes, existing code has deep relationship each other.
> Thus, fixing/droping seems very dificult...
> I think each struct has random pointer connection which doesn't care
> about ALSA SoC's hierarchy.
> 
> I would like to indicate my opinion here.
> My strange point are...
> 
> 1. snd_soc_pcm_runtime has pointer to Card/Codec/Platform, but no CPU
> 2. snd_soc_dai has pointer to [component] and [Codec]
>    [component] is its parent, [Codec] is maybe Hack

dai->codec is something we could get rid of since it is the same as
dai->component->codec. And for drivers were we know that it is a CODEC
driver you can use snd_soc_component_to_codec(dai->component). For those
drivers we know that this returns a valid pointer.

But there are a lot of users which all need to be updated. If you want to do
it go ahead. If you do the conversion this should be done by the use of
helper functions, so we can change the implementation if necessary.

> 3. [component] has pointer to [card] and [Codec]
>    [card] is understandable, [Codec] is maybe Hack
> 
> It seems many struct would like to access to [Codec], thus, each struct has
> codec pointer (as hack ?), but it is ignoring ALSA SoC hierarchy.
> 

This is a leftover of the old hierarchy where snd_soc_codec was not a
sub-struct of snd_soc_component.

> It will be more clear if...
> 
> IDEA 1
> 
>    Each component (= CPU/Codec/Platform) has pointer to Card now.
>    How about Card has pointer to each component ?
>    if so, we can access to every struct.
>    Card -> CPU/Codec/Platform, CPU/Codec/Platform -> Card
>    Then, we want to have component related macro/flag
> 
> 	#define component_is_cpu(component) (component->flag & COMPONENT_CPU)
> 	#define component_is_codec(component) (component->flag & COMPONENT_CODEC)
> 	#define component_is_platform(component) (component->flag & COMPONENT_PLATFORM)
> 
> 	#define component_to_cpu() container_of(xxx)
> 	#define component_to_codec() container_of(xxx)
> 	#define component_to_platorm() container_of(xxx)

I though about this when doing the refactoring of the ASoC core. But in the
end it does not make too much of a difference whether we have a flag field
or a CODEC pointer field. The former would have just caused more code churn
so I went with the later.

You wouldn't be able to simplify the code by this. You need to replace all
component->codec checks by component_is_codec(component). So the overall
code complexity stays the same.

The helper macros might still make sense to hide the actual implementation.

> 
> IDEA 2
> 
>    if IDEA1 was OK, snd_soc_pcm_runtime already has card pointer.
>    This means snd_soc_pcm_runtime can access to every component struct.
> 
>    snd_soc_dai has its parent component.
>    This means snd_soc_dai can access to Card, and Card can access to every component.
>    And then, cpu_dai->codec has no issue if we can use component_is_xxx() macro ?
> 
>     - if (cpu_dai->codec)
>     + if (component_is_codec(cpu_dai->component))
> 
> If IDEA1 / IDEA2 are OK, we can cleanup each struct,
> especially hack-able random pointer ?

In my opinion the flags are just as much a hack as the pointer. In an ideal
setup the component does not need to know what type it is. The reason why we
need this in ASoC is because the framework has grown over time and we need
to support legacy code.

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Lars-Peter Clausen]

On 07/29/2016 02:30 AM, Takashi Sakamoto wrote:
> Lars,
> 
> On Jul 29 2016 05:33, Lars-Peter Clausen wrote:
>> Hotplug is something that always pops up sooner or later. E.g. if someone
>> puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we
>> don't want to duplicate the code, but be able to reuse.
> 
> (A bit to sidetrack)
> 
> To me, it's unclear for devices on USB. When ALSA SoC part supports these
> devices, what is the scenario you assumed? In short, assuming we put codes
> to ALSA SoC part, what is the shape of the corresponding devices and links
> of pairs of endpoints? Additionally, in this case, what codes are able to be
> reused?

Lets say you have USB stick with a small micro controller or FPGA which has
a USB interface on one side and a I2S and I2C interface on the other side.
The I2S and I2C are connected to a CODEC. I2S for data, I2C for control. If
the interface is implemented in a way so that it is just a simple USB to I2C
bridge, this means the raw I2C commands are send over the USB interface you
can implement a I2C adapter driver for this bridge. If you have that you can
instantiate the existing ASoC CODEC driver, which is a I2C device driver, on
the bus registered by the adapter.

- Lars

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

On Fri, Jul 29, 2016 at 11:01:27AM +0200, Lars-Peter Clausen wrote:

> In my opinion the flags are just as much a hack as the pointer. In an ideal
> setup the component does not need to know what type it is. The reason why we
> need this in ASoC is because the framework has grown over time and we need
> to support legacy code.

Yes, the pointer is essentially already a flag - having a separate flag
really doesn't add a huge amount here.  What we need to do is get rid of
the need for the flag entirely.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Vinod Koul]

On Fri, Jul 29, 2016 at 11:07:49AM +0200, Lars-Peter Clausen wrote:
> On 07/29/2016 02:30 AM, Takashi Sakamoto wrote:
> > Lars,
> > 
> > On Jul 29 2016 05:33, Lars-Peter Clausen wrote:
> >> Hotplug is something that always pops up sooner or later. E.g. if someone
> >> puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we
> >> don't want to duplicate the code, but be able to reuse.
> > 
> > (A bit to sidetrack)
> > 
> > To me, it's unclear for devices on USB. When ALSA SoC part supports these
> > devices, what is the scenario you assumed? In short, assuming we put codes
> > to ALSA SoC part, what is the shape of the corresponding devices and links
> > of pairs of endpoints? Additionally, in this case, what codes are able to be
> > reused?
> 
> Lets say you have USB stick with a small micro controller or FPGA which has
> a USB interface on one side and a I2S and I2C interface on the other side.
> The I2S and I2C are connected to a CODEC. I2S for data, I2C for control. If
> the interface is implemented in a way so that it is just a simple USB to I2C
> bridge, this means the raw I2C commands are send over the USB interface you
> can implement a I2C adapter driver for this bridge. If you have that you can
> instantiate the existing ASoC CODEC driver, which is a I2C device driver, on
> the bus registered by the adapter.

That still seems a bit fancy hardware :)

If we can reasonably support this, I am for it. But not making stuff
overtly complex...

-- 
~Vinod

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Iwai]

On Fri, 29 Jul 2016 18:07:02 +0200,
Vinod Koul wrote:
> 
> On Fri, Jul 29, 2016 at 11:07:49AM +0200, Lars-Peter Clausen wrote:
> > On 07/29/2016 02:30 AM, Takashi Sakamoto wrote:
> > > Lars,
> > > 
> > > On Jul 29 2016 05:33, Lars-Peter Clausen wrote:
> > >> Hotplug is something that always pops up sooner or later. E.g. if someone
> > >> puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we
> > >> don't want to duplicate the code, but be able to reuse.
> > > 
> > > (A bit to sidetrack)
> > > 
> > > To me, it's unclear for devices on USB. When ALSA SoC part supports these
> > > devices, what is the scenario you assumed? In short, assuming we put codes
> > > to ALSA SoC part, what is the shape of the corresponding devices and links
> > > of pairs of endpoints? Additionally, in this case, what codes are able to be
> > > reused?
> > 
> > Lets say you have USB stick with a small micro controller or FPGA which has
> > a USB interface on one side and a I2S and I2C interface on the other side.
> > The I2S and I2C are connected to a CODEC. I2S for data, I2C for control. If
> > the interface is implemented in a way so that it is just a simple USB to I2C
> > bridge, this means the raw I2C commands are send over the USB interface you
> > can implement a I2C adapter driver for this bridge. If you have that you can
> > instantiate the existing ASoC CODEC driver, which is a I2C device driver, on
> > the bus registered by the adapter.
> 
> That still seems a bit fancy hardware :)
> 
> If we can reasonably support this, I am for it. But not making stuff
> overtly complex...

Yes, we don't have to overreact to a dream immediately now.
We should consider whether it can be shifted to the card-level instead
before worrying too much about hot-unplug of an ASoC component.  Then
the whole story becomes far easier.

And, the suppress_bind_attrs flag I suggested is only to suppress the
sysfs bind/unbind, and doesn't mean to prohibit hotplug itself via the
bus driver.


Takashi

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Sakamoto]

On Jul 30 2016 05:41, Takashi Iwai wrote:
>> That still seems a bit fancy hardware :)
>>
>> If we can reasonably support this, I am for it. But not making stuff
>> overtly complex...
> 
> Yes, we don't have to overreact to a dream immediately now.

As I wrote at first, my question is a sidetrack. So you can still
continue the discussion for loadable ALSA SoC part.

In my understanding, one of the aim of ALSA SoC part is to reuse drivers
of audio IPs, typically hardware ADC/DAC (so-called 'codec'). To achieve
this, the ideas of 'DAI' and 'link' are introduced for better
abstraction, additionally to historical 'card' and 'components'.

In a point of 'reuse of codes', I cannot imagine what Lars said for USB
devices, then post the questions.

> We should consider whether it can be shifted to the card-level instead
> before worrying too much about hot-unplug of an ASoC component.  Then
> the whole story becomes far easier.
>
> And, the suppress_bind_attrs flag I suggested is only to suppress the
> sysfs bind/unbind, and doesn't mean to prohibit hotplug itself via the
> bus driver.

In my opinion, the relationship between DAI driver and codec or
component driver is not described as relationship between kernel
modules. The issue cannot be resolved by dependency of loadable modules
and we need to manage the relationship of loaded modules somewhere. In
this meaning, I can understand the idea which Mr.Morimoto described.

But I think it's logically difficult to manage state of sound card; e.g.
disconnect. When one sound card instance consists of instances of
several 'DAI', 'Codecs' and 'Components' (this 'component' is not in
ALSA core contexts[1]) and we try to unload one of them, then which
state the card should be assigned to? Or no 'Codecs' drivers are loaded,
then which state should be assigned to the card?

Additionally, when old Codec driver is unloaded and new Codec driver is
loaded, then what should we do for corresponding PCM character devices
are? Currently, once snd_card_regsiter() is called, we cannot
insert/delete ALSA components such like PCM.


[1] For the context, please refer to 'Writing an ALSA driver'.
http://www.alsa-project.org/~tiwai/writing-an-alsa-driver/ch02s03.html#basic-flow-constructor-create-other


Regards

Takashi Sakamoto

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On Sat, Jul 30, 2016 at 06:45:04AM +0900, Takashi Sakamoto wrote:

> In a point of 'reuse of codes', I cannot imagine what Lars said for USB
> devices, then post the questions.

Someone might make a fancy device connected via USB which doesn't
conform to the USB specs.

> But I think it's logically difficult to manage state of sound card; e.g.
> disconnect. When one sound card instance consists of instances of
> several 'DAI', 'Codecs' and 'Components' (this 'component' is not in
> ALSA core contexts[1]) and we try to unload one of them, then which
> state the card should be assigned to? Or no 'Codecs' drivers are loaded,
> then which state should be assigned to the card?

The card only instantiates when all the components of the card are
present, until then it defers probe.

> Additionally, when old Codec driver is unloaded and new Codec driver is
> loaded, then what should we do for corresponding PCM character devices
> are? Currently, once snd_card_regsiter() is called, we cannot
> insert/delete ALSA components such like PCM.

The card should be deinstantiated and reinstantiated whenever a
component driver unbinds and rebinds (respectively).  You'd need to
completely deregister the card to change the list of things it's
expecting currently.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Lars, Mark

> > In my opinion the flags are just as much a hack as the pointer. In an ideal
> > setup the component does not need to know what type it is. The reason why we
> > need this in ASoC is because the framework has grown over time and we need
> > to support legacy code.
> 
> Yes, the pointer is essentially already a flag - having a separate flag
> really doesn't add a huge amount here.  What we need to do is get rid of
> the need for the flag entirely.

I can agree to remove flag entirely, and actually it is my final target.
But we need many steps for it IMO.

I would like to cleanup (= remove random pointer) as 1st step.
My 1st concern is that each struct which want to have codec pointer has
codec pointer randomly.
And it makes component cleanup difficult IMO.

2nd concern is rtd has *codec, *platform, and *dai (for cpu/codec).
These are different type of pointer.
If platform can have dummy *dai, we can do like below ?

	struct snd_soc_pcm_runtime {
		...
		struct snd_soc_dai *cpu;
		struct snd_soc_dai *codec;
		struct snd_soc_dai *platform;
		...
	}

Here, we can get original *codec or *platform by
using component_to_xxx, because dai already have *component;
It can be more cleanup ?

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Lars, Mark

> > In my opinion the flags are just as much a hack as the pointer. In an ideal
> > setup the component does not need to know what type it is. The reason why we
> > need this in ASoC is because the framework has grown over time and we need
> > to support legacy code.
> 
> Yes, the pointer is essentially already a flag - having a separate flag
> really doesn't add a huge amount here.  What we need to do is get rid of
> the need for the flag entirely.

I can agree to remove flag entirely, and actually it is my final target.
But we need many steps for it IMO.

I would like to cleanup (= remove random pointer) as 1st step.
My 1st concern is that each struct which want to have codec pointer has
codec pointer randomly.
And it makes component cleanup difficult IMO.

2nd concern is rtd has *codec, *platform, and *dai (for cpu/codec).
These are different type of pointer.
If platform can have dummy *dai, we can do like below ?

	struct snd_soc_pcm_runtime {
		...
		struct snd_soc_dai *cpu;
		struct snd_soc_dai *codec;
		struct snd_soc_dai *platform;
		...
	}

Here, we can get original *codec or *platform by
using component_to_xxx, because dai already have *component;
It can be more cleanup ?

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Lars, Mark

My previous mail was missing point...

> > > In my opinion the flags are just as much a hack as the pointer. In an ideal
> > > setup the component does not need to know what type it is. The reason why we
> > > need this in ASoC is because the framework has grown over time and we need
> > > to support legacy code.
> > 
> > Yes, the pointer is essentially already a flag - having a separate flag
> > really doesn't add a huge amount here.  What we need to do is get rid of
> > the need for the flag entirely.
> 
> I can agree to remove flag entirely, and actually it is my final target.
> But we need many steps for it IMO.
> 
> I would like to cleanup (= remove random pointer) as 1st step.
> My 1st concern is that each struct which want to have codec pointer has
> codec pointer randomly.
> And it makes component cleanup difficult IMO.
> 
> 2nd concern is rtd has *codec, *platform, and *dai (for cpu/codec).
> These are different type of pointer.
> If platform can have dummy *dai, we can do like below ?
> 
> 	struct snd_soc_pcm_runtime {
> 		...
> 		struct snd_soc_dai *cpu;
> 		struct snd_soc_dai *codec;
> 		struct snd_soc_dai *platform;
> 		...
> 	}
> 
> Here, we can get original *codec or *platform by
> using component_to_xxx, because dai already have *component;
> It can be more cleanup ?

I mean current ALSA SoC has duplicate pointer,
and using different type of pointer for CPU/Codec/Platform/Card.
I agree that we would like to have flag-less style.

If my understanding was correct, we can more cleanup struct
relationship if we can exchange pointer style.
I think this is related to flag-less style.

For example, current rtd <-> CPU/Codec/Platform relationship
is using different type of pointer.
It makes flag-less style difficult.
But we can use same type of pointer if we can have dummy dai on platform,
like above idea. Similar idea is for component.

I think this kind of cleanup is needed, and it makes hotplug support easier.
Otherwise, hotplug support will add more random pointer on each struct,
and it makes cleanup more difficult.

# Here, I said "flag-less" style, but maybe we need flag somehow to keep
# current style ? For example I don't think we can have flag-less cristal-clear
# style for CODEC-CODEC support or DPCM etc ...

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Lars, Mark

My previous mail was missing point...

> > > In my opinion the flags are just as much a hack as the pointer. In an ideal
> > > setup the component does not need to know what type it is. The reason why we
> > > need this in ASoC is because the framework has grown over time and we need
> > > to support legacy code.
> > 
> > Yes, the pointer is essentially already a flag - having a separate flag
> > really doesn't add a huge amount here.  What we need to do is get rid of
> > the need for the flag entirely.
> 
> I can agree to remove flag entirely, and actually it is my final target.
> But we need many steps for it IMO.
> 
> I would like to cleanup (= remove random pointer) as 1st step.
> My 1st concern is that each struct which want to have codec pointer has
> codec pointer randomly.
> And it makes component cleanup difficult IMO.
> 
> 2nd concern is rtd has *codec, *platform, and *dai (for cpu/codec).
> These are different type of pointer.
> If platform can have dummy *dai, we can do like below ?
> 
> 	struct snd_soc_pcm_runtime {
> 		...
> 		struct snd_soc_dai *cpu;
> 		struct snd_soc_dai *codec;
> 		struct snd_soc_dai *platform;
> 		...
> 	}
> 
> Here, we can get original *codec or *platform by
> using component_to_xxx, because dai already have *component;
> It can be more cleanup ?

I mean current ALSA SoC has duplicate pointer,
and using different type of pointer for CPU/Codec/Platform/Card.
I agree that we would like to have flag-less style.

If my understanding was correct, we can more cleanup struct
relationship if we can exchange pointer style.
I think this is related to flag-less style.

For example, current rtd <-> CPU/Codec/Platform relationship
is using different type of pointer.
It makes flag-less style difficult.
But we can use same type of pointer if we can have dummy dai on platform,
like above idea. Similar idea is for component.

I think this kind of cleanup is needed, and it makes hotplug support easier.
Otherwise, hotplug support will add more random pointer on each struct,
and it makes cleanup more difficult.

# Here, I said "flag-less" style, but maybe we need flag somehow to keep
# current style ? For example I don't think we can have flag-less cristal-clear
# style for CODEC-CODEC support or DPCM etc ...

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Lars-Peter Clausen]

On 08/02/2016 08:47 AM, Kuninori Morimoto wrote:
> 
> Hi Lars, Mark
> 
> My previous mail was missing point...
> 
>>>> In my opinion the flags are just as much a hack as the pointer. In an ideal
>>>> setup the component does not need to know what type it is. The reason why we
>>>> need this in ASoC is because the framework has grown over time and we need
>>>> to support legacy code.
>>>
>>> Yes, the pointer is essentially already a flag - having a separate flag
>>> really doesn't add a huge amount here.  What we need to do is get rid of
>>> the need for the flag entirely.
>>
>> I can agree to remove flag entirely, and actually it is my final target.
>> But we need many steps for it IMO.
>>
>> I would like to cleanup (= remove random pointer) as 1st step.
>> My 1st concern is that each struct which want to have codec pointer has
>> codec pointer randomly.
>> And it makes component cleanup difficult IMO.
>>
>> 2nd concern is rtd has *codec, *platform, and *dai (for cpu/codec).
>> These are different type of pointer.
>> If platform can have dummy *dai, we can do like below ?
>>
>> 	struct snd_soc_pcm_runtime {
>> 		...
>> 		struct snd_soc_dai *cpu;
>> 		struct snd_soc_dai *codec;
>> 		struct snd_soc_dai *platform;
>> 		...
>> 	}
>>
>> Here, we can get original *codec or *platform by
>> using component_to_xxx, because dai already have *component;
>> It can be more cleanup ?
> 
> I mean current ALSA SoC has duplicate pointer,
> and using different type of pointer for CPU/Codec/Platform/Card.
> I agree that we would like to have flag-less style.
> 
> If my understanding was correct, we can more cleanup struct
> relationship if we can exchange pointer style.
> I think this is related to flag-less style.
> 
> For example, current rtd <-> CPU/Codec/Platform relationship
> is using different type of pointer.
> It makes flag-less style difficult.
> But we can use same type of pointer if we can have dummy dai on platform,
> like above idea. Similar idea is for component.
> 
> I think this kind of cleanup is needed, and it makes hotplug support easier.
> Otherwise, hotplug support will add more random pointer on each struct,
> and it makes cleanup more difficult.
> 
> # Here, I said "flag-less" style, but maybe we need flag somehow to keep
> # current style ? For example I don't think we can have flag-less cristal-clear
> # style for CODEC-CODEC support or DPCM etc ...

I think moving forward we should get rid of the whole CPU/CODEC/platform
concept. This is an outdated view of how the hardware looks like. When ASoC
was initially introduce all hardware basically had a CPU side DAI, a CODEC
side DAI and a DMA. The DMA was connected to the CPU DAI and the CPU DAI was
connected to the CODEC DAI and that was it. The CPU side was also really
simple, no signal processing, no signal routing, just the raw audio data
directly transferred via DMA (or PIO sometimes) to the CPU DAI controller.
And all digital audio was assumed to be in a single digital domain running
at them same clock rate.

This no longer reflects todays typical systems. Sometimes you have more than
those three components (e.g. additional amplifier IC, BT chip ...),
sometimes you less (just a DAC or ADC directly connected to a DMA in the
SoC). You often have complex routing and processing capabilities on the host
side. Also you have often multiple digital domains with sample-rate
converters between them.

Yet still at the core ASoC keeps the CPU/CODEC/platform concept. DPCM e.g.
tries to work with these concepts by introducing frontend and backend DAIs
and use dummy components to compensate for when the software model does not
match the hardware model. This makes it code more complicated than it has to
be and less efficient than it can be.

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Lars

> I think moving forward we should get rid of the whole CPU/CODEC/platform
> concept. This is an outdated view of how the hardware looks like. When ASoC
> was initially introduce all hardware basically had a CPU side DAI, a CODEC
> side DAI and a DMA. The DMA was connected to the CPU DAI and the CPU DAI was
> connected to the CODEC DAI and that was it. The CPU side was also really
> simple, no signal processing, no signal routing, just the raw audio data
> directly transferred via DMA (or PIO sometimes) to the CPU DAI controller.
> And all digital audio was assumed to be in a single digital domain running
> at them same clock rate.
> 
> This no longer reflects todays typical systems. Sometimes you have more than
> those three components (e.g. additional amplifier IC, BT chip ...),
> sometimes you less (just a DAC or ADC directly connected to a DMA in the
> SoC). You often have complex routing and processing capabilities on the host
> side. Also you have often multiple digital domains with sample-rate
> converters between them.
> 
> Yet still at the core ASoC keeps the CPU/CODEC/platform concept. DPCM e.g.
> tries to work with these concepts by introducing frontend and backend DAIs
> and use dummy components to compensate for when the software model does not
> match the hardware model. This makes it code more complicated than it has to
> be and less efficient than it can be.

I agree to your opinion.
OTOH, we would like to use/keep existing current all drivers.
Thus, I think we need super many small and slow steps.
Or, we need new ASoC2 ?

I can agree that we should get rid of current CPU/Codec/Platform,
but, removing all of them is a little bit over-kill for me.
I think ALSA SoC "framework" should care "component" only,
and it doesn't care what it was.
OTOH "driver" side can use existing CPU/Codec/Platform and/or AUX/compr
etc as helper ? (I'm not sure detail of AUX/compr actually...)
And each "component" has "dai".

My image is like this.
This allows many / few components, and many / few DAIs.
Current DPCM style is automatically supported,
and it is easy to add new style device.
What do you think ?

/*
 * DAI has parent (= component) pointer
 * and rtd interface list_head
 */
struct snd_soc_dai {
	...
	struct snd_soc_component *component; /* parent */
	struct list_head *dai_node; /* struct snd_soc_pcm_runtime */
};

/*
 * Each component has at least 1 DAI
 *
 * CPU/Codec will have many DAIs
 * Card/Platform and other will have dummy DAI as interface
 */
struct snd_soc_component {
	...
	struct snd_soc_dai *dais;
};

/*
 * We can use Card/CPU/Codec/Platform/AUX/COMPR helper,
 * all includes "component".
 * Of course each driver can create original one,
 * or don't use helper is no problem
 */
struct snd_soc_card {
	...
	struct snd_soc_component component;
};

struct snd_soc_cpu {
	...
	struct snd_soc_component component;
};

struct snd_soc_codec {
	...
	struct snd_soc_component component;
};

struct snd_soc_platform {
	...
	struct snd_soc_component component;
};

struct snd_soc_aux {
	...
	struct snd_soc_component component;
};
struct snd_soc_compr {
	...
	struct snd_soc_component component;
};

...

/*
 * rtd has each DAI interface list.
 * But it doesn't care what it was
 */
struct snd_soc_pcm_runtime {
	...
	struct list_head *dai_node_head; /* struct snd_soc_dai */
}

/*
 * each function just call dai or component function
 * it doesn't care what it was
 */
static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
{
	struct snd_soc_pcm_runtime *rtd = substream->private_data;
	struct snd_soc_component *component;
	struct snd_soc_dai *dai;
	int ret = 0;

	/*
	 * call each component's ops
	 */
	list_for_each_entry(dai, rtd->xxx) {
		component = snd_soc_dai_to_component(dai);
		ret = component->ops->trigger(dai, xxx);
		if (ret < 0)
			goto trigger_end;
	}

trigger_end:
	return ret;
}

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Lars

> I think moving forward we should get rid of the whole CPU/CODEC/platform
> concept. This is an outdated view of how the hardware looks like. When ASoC
> was initially introduce all hardware basically had a CPU side DAI, a CODEC
> side DAI and a DMA. The DMA was connected to the CPU DAI and the CPU DAI was
> connected to the CODEC DAI and that was it. The CPU side was also really
> simple, no signal processing, no signal routing, just the raw audio data
> directly transferred via DMA (or PIO sometimes) to the CPU DAI controller.
> And all digital audio was assumed to be in a single digital domain running
> at them same clock rate.
> 
> This no longer reflects todays typical systems. Sometimes you have more than
> those three components (e.g. additional amplifier IC, BT chip ...),
> sometimes you less (just a DAC or ADC directly connected to a DMA in the
> SoC). You often have complex routing and processing capabilities on the host
> side. Also you have often multiple digital domains with sample-rate
> converters between them.
> 
> Yet still at the core ASoC keeps the CPU/CODEC/platform concept. DPCM e.g.
> tries to work with these concepts by introducing frontend and backend DAIs
> and use dummy components to compensate for when the software model does not
> match the hardware model. This makes it code more complicated than it has to
> be and less efficient than it can be.

I agree to your opinion.
OTOH, we would like to use/keep existing current all drivers.
Thus, I think we need super many small and slow steps.
Or, we need new ASoC2 ?

I can agree that we should get rid of current CPU/Codec/Platform,
but, removing all of them is a little bit over-kill for me.
I think ALSA SoC "framework" should care "component" only,
and it doesn't care what it was.
OTOH "driver" side can use existing CPU/Codec/Platform and/or AUX/compr
etc as helper ? (I'm not sure detail of AUX/compr actually...)
And each "component" has "dai".

My image is like this.
This allows many / few components, and many / few DAIs.
Current DPCM style is automatically supported,
and it is easy to add new style device.
What do you think ?

/*
 * DAI has parent (= component) pointer
 * and rtd interface list_head
 */
struct snd_soc_dai {
	...
	struct snd_soc_component *component; /* parent */
	struct list_head *dai_node; /* struct snd_soc_pcm_runtime */
};

/*
 * Each component has at least 1 DAI
 *
 * CPU/Codec will have many DAIs
 * Card/Platform and other will have dummy DAI as interface
 */
struct snd_soc_component {
	...
	struct snd_soc_dai *dais;
};

/*
 * We can use Card/CPU/Codec/Platform/AUX/COMPR helper,
 * all includes "component".
 * Of course each driver can create original one,
 * or don't use helper is no problem
 */
struct snd_soc_card {
	...
	struct snd_soc_component component;
};

struct snd_soc_cpu {
	...
	struct snd_soc_component component;
};

struct snd_soc_codec {
	...
	struct snd_soc_component component;
};

struct snd_soc_platform {
	...
	struct snd_soc_component component;
};

struct snd_soc_aux {
	...
	struct snd_soc_component component;
};
struct snd_soc_compr {
	...
	struct snd_soc_component component;
};

...

/*
 * rtd has each DAI interface list.
 * But it doesn't care what it was
 */
struct snd_soc_pcm_runtime {
	...
	struct list_head *dai_node_head; /* struct snd_soc_dai */
}

/*
 * each function just call dai or component function
 * it doesn't care what it was
 */
static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
{
	struct snd_soc_pcm_runtime *rtd = substream->private_data;
	struct snd_soc_component *component;
	struct snd_soc_dai *dai;
	int ret = 0;

	/*
	 * call each component's ops
	 */
	list_for_each_entry(dai, rtd->xxx) {
		component = snd_soc_dai_to_component(dai);
		ret = component->ops->trigger(dai, xxx);
		if (ret < 0)
			goto trigger_end;
	}

trigger_end:
	return ret;
}

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Sakamoto]

On Jul 30 2016 07:08, Mark Brown wrote:
>> But I think it's logically difficult to manage state of sound card; e.g.
>> disconnect. When one sound card instance consists of instances of
>> several 'DAI', 'Codecs' and 'Components' (this 'component' is not in
>> ALSA core contexts[1]) and we try to unload one of them, then which
>> state the card should be assigned to? Or no 'Codecs' drivers are loaded,
>> then which state should be assigned to the card?
>
> The card only instantiates when all the components of the card are
> present, until then it defers probe.

Oops. I forgot ALSA soc part utilizes EPROBE_DEFER. Thanks for your 
correction.

>> Additionally, when old Codec driver is unloaded and new Codec driver is
>> loaded, then what should we do for corresponding PCM character devices
>> are? Currently, once snd_card_regsiter() is called, we cannot
>> insert/delete ALSA components such like PCM.
>
> The card should be deinstantiated and reinstantiated whenever a
> component driver unbinds and rebinds (respectively).  You'd need to
> completely deregister the card to change the list of things it's
> expecting currently.

In a point of application interfaces, I guess that current 
implementation of ALSA soc part includes a bug that it's possible to 
unload codec or component modules when any ALSA character devices are 
opened. The framework has no codes to manage reference counting of 
character devices or loaded codecs, components.

Here, any of suggestions comes from my code reading. I apologize if 
they're wrong.


Regards

Takashi Sakamoto

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Lars-Peter Clausen]

On 08/04/2016 04:38 AM, Kuninori Morimoto wrote:
> 
> Hi Lars
> 
>> I think moving forward we should get rid of the whole CPU/CODEC/platform
>> concept. This is an outdated view of how the hardware looks like. When ASoC
>> was initially introduce all hardware basically had a CPU side DAI, a CODEC
>> side DAI and a DMA. The DMA was connected to the CPU DAI and the CPU DAI was
>> connected to the CODEC DAI and that was it. The CPU side was also really
>> simple, no signal processing, no signal routing, just the raw audio data
>> directly transferred via DMA (or PIO sometimes) to the CPU DAI controller.
>> And all digital audio was assumed to be in a single digital domain running
>> at them same clock rate.
>>
>> This no longer reflects todays typical systems. Sometimes you have more than
>> those three components (e.g. additional amplifier IC, BT chip ...),
>> sometimes you less (just a DAC or ADC directly connected to a DMA in the
>> SoC). You often have complex routing and processing capabilities on the host
>> side. Also you have often multiple digital domains with sample-rate
>> converters between them.
>>
>> Yet still at the core ASoC keeps the CPU/CODEC/platform concept. DPCM e.g.
>> tries to work with these concepts by introducing frontend and backend DAIs
>> and use dummy components to compensate for when the software model does not
>> match the hardware model. This makes it code more complicated than it has to
>> be and less efficient than it can be.
> 
> I agree to your opinion.
> OTOH, we would like to use/keep existing current all drivers.
> Thus, I think we need super many small and slow steps.
> Or, we need new ASoC2 ?
> 
> I can agree that we should get rid of current CPU/Codec/Platform,
> but, removing all of them is a little bit over-kill for me.
> I think ALSA SoC "framework" should care "component" only,
> and it doesn't care what it was.
> OTOH "driver" side can use existing CPU/Codec/Platform and/or AUX/compr
> etc as helper ? (I'm not sure detail of AUX/compr actually...)
> And each "component" has "dai".
> 
> My image is like this.
> This allows many / few components, and many / few DAIs.
> Current DPCM style is automatically supported,
> and it is easy to add new style device.
> What do you think ?

To be honest I'd also get rid of DAIs has a top level concept. This image
(http://metafoo.de/the_new_asoc.svg) is something I've put together awhile
ago how I think we should lay things out if we do a major refactoring of the
ASoC core.

The big green and blue boxes are components. Green are On-SoC components,
blue are external components.

The light grey boxes are domains. Domains have certain properties, like e.g.
samplerates. There are different types of domains and different types have
different kinds of properties. E.g. in the PCM domain we care about the
memory layout of the samples in addition to the samplerate and the number of
channels, while in the digital domain however we do no longer care about the
memory layout since here data is processed one sample at a time. In the I2S
link domain we care about which I2S mode is used (I2S, LJ, ...), while in a
SPDIF link domain we don't care about such properties.

The dark grey boxes are bridges between domains and they translate
properties from one domain to another. This can either be straight forward
propagation like a PCM device where the samplerate in the source and target
domain are the same, or it can modify the properties, e.g. a samplerate
converter between two digital domains will change the samplerate according
to the interpolation/decimation factor of the SRC.

In addition to just translating the properties a bridge can also translate
the constraints on properties from one domain to another or even add its own
constraints. E.g. if a CODEC has a constraint that it can run either with
48kHz or 96kHz this constraint is propagated through the bridges to the PCM
devices so that the userspace application using the PCM is aware of this.
Bridges can also add their own constraints e.g. a SRC might have multiple
interpolation/decimation rates available, so it might say the samplerate in
the target domain must be 1x or 2x that of the source domain. Going back to
the example that means for a CODEC that supports either 48kHz or 96kHz and
there is a SRC with interpolation factors of 1 or 2 then the PCM device will
present the list of either 24kHz, 48kHz or 96kHz to the userspace application.

A bridge is also not limited to one source and one sink. It can have
multiple sources and multiple sinks e.g. for a crossbar with a complex
routing matrix.

Obviously we can't convert everything at the same time and it will take a
lot of time and effort to update all drivers to this new model. This is
where the legacy bridge kicks in which still keeps the concept of 1 CPU, 1
CODEC, 1 platform. If you want to use the advanced features of the new
framework you have to update your driver, if you are OK with the current set
of features just keep the drivers the way they are and use the legacy bridge
that is automatically managed by the ASoC core.

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Lars-Peter Clausen]

On 08/04/2016 04:38 AM, Kuninori Morimoto wrote:
> 
> Hi Lars
> 
>> I think moving forward we should get rid of the whole CPU/CODEC/platform
>> concept. This is an outdated view of how the hardware looks like. When ASoC
>> was initially introduce all hardware basically had a CPU side DAI, a CODEC
>> side DAI and a DMA. The DMA was connected to the CPU DAI and the CPU DAI was
>> connected to the CODEC DAI and that was it. The CPU side was also really
>> simple, no signal processing, no signal routing, just the raw audio data
>> directly transferred via DMA (or PIO sometimes) to the CPU DAI controller.
>> And all digital audio was assumed to be in a single digital domain running
>> at them same clock rate.
>>
>> This no longer reflects todays typical systems. Sometimes you have more than
>> those three components (e.g. additional amplifier IC, BT chip ...),
>> sometimes you less (just a DAC or ADC directly connected to a DMA in the
>> SoC). You often have complex routing and processing capabilities on the host
>> side. Also you have often multiple digital domains with sample-rate
>> converters between them.
>>
>> Yet still at the core ASoC keeps the CPU/CODEC/platform concept. DPCM e.g.
>> tries to work with these concepts by introducing frontend and backend DAIs
>> and use dummy components to compensate for when the software model does not
>> match the hardware model. This makes it code more complicated than it has to
>> be and less efficient than it can be.
> 
> I agree to your opinion.
> OTOH, we would like to use/keep existing current all drivers.
> Thus, I think we need super many small and slow steps.
> Or, we need new ASoC2 ?
> 
> I can agree that we should get rid of current CPU/Codec/Platform,
> but, removing all of them is a little bit over-kill for me.
> I think ALSA SoC "framework" should care "component" only,
> and it doesn't care what it was.
> OTOH "driver" side can use existing CPU/Codec/Platform and/or AUX/compr
> etc as helper ? (I'm not sure detail of AUX/compr actually...)
> And each "component" has "dai".
> 
> My image is like this.
> This allows many / few components, and many / few DAIs.
> Current DPCM style is automatically supported,
> and it is easy to add new style device.
> What do you think ?

To be honest I'd also get rid of DAIs has a top level concept. This image
(http://metafoo.de/the_new_asoc.svg) is something I've put together awhile
ago how I think we should lay things out if we do a major refactoring of the
ASoC core.

The big green and blue boxes are components. Green are On-SoC components,
blue are external components.

The light grey boxes are domains. Domains have certain properties, like e.g.
samplerates. There are different types of domains and different types have
different kinds of properties. E.g. in the PCM domain we care about the
memory layout of the samples in addition to the samplerate and the number of
channels, while in the digital domain however we do no longer care about the
memory layout since here data is processed one sample at a time. In the I2S
link domain we care about which I2S mode is used (I2S, LJ, ...), while in a
SPDIF link domain we don't care about such properties.

The dark grey boxes are bridges between domains and they translate
properties from one domain to another. This can either be straight forward
propagation like a PCM device where the samplerate in the source and target
domain are the same, or it can modify the properties, e.g. a samplerate
converter between two digital domains will change the samplerate according
to the interpolation/decimation factor of the SRC.

In addition to just translating the properties a bridge can also translate
the constraints on properties from one domain to another or even add its own
constraints. E.g. if a CODEC has a constraint that it can run either with
48kHz or 96kHz this constraint is propagated through the bridges to the PCM
devices so that the userspace application using the PCM is aware of this.
Bridges can also add their own constraints e.g. a SRC might have multiple
interpolation/decimation rates available, so it might say the samplerate in
the target domain must be 1x or 2x that of the source domain. Going back to
the example that means for a CODEC that supports either 48kHz or 96kHz and
there is a SRC with interpolation factors of 1 or 2 then the PCM device will
present the list of either 24kHz, 48kHz or 96kHz to the userspace application.

A bridge is also not limited to one source and one sink. It can have
multiple sources and multiple sinks e.g. for a crossbar with a complex
routing matrix.

Obviously we can't convert everything at the same time and it will take a
lot of time and effort to update all drivers to this new model. This is
where the legacy bridge kicks in which still keeps the concept of 1 CPU, 1
CODEC, 1 platform. If you want to use the advanced features of the new
framework you have to update your driver, if you are OK with the current set
of features just keep the drivers the way they are and use the legacy bridge
that is automatically managed by the ASoC core.

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
> On Jul 30 2016 07:08, Mark Brown wrote:

> > The card should be deinstantiated and reinstantiated whenever a
> > component driver unbinds and rebinds (respectively).  You'd need to
> > completely deregister the card to change the list of things it's
> > expecting currently.

> In a point of application interfaces, I guess that current implementation of
> ALSA soc part includes a bug that it's possible to unload codec or component
> modules when any ALSA character devices are opened. The framework has no
> codes to manage reference counting of character devices or loaded codecs,
> components.

Yes, exactly - we don't cope very well with that situation and we really
ought to but since it's hard to trigger without trying in practice it's
never been a priority.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Sakamoto]

[-- Attachment #1.1: Type: text/plain, Size: 1809 bytes --]

On Aug 4 2016 19:28, Mark Brown wrote:
> On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
>> On Jul 30 2016 07:08, Mark Brown wrote:
> 
>>> The card should be deinstantiated and reinstantiated whenever a
>>> component driver unbinds and rebinds (respectively).  You'd need to
>>> completely deregister the card to change the list of things it's
>>> expecting currently.
> 
>> In a point of application interfaces, I guess that current implementation of
>> ALSA soc part includes a bug that it's possible to unload codec or component
>> modules when any ALSA character devices are opened. The framework has no
>> codes to manage reference counting of character devices or loaded codecs,
>> components.
> 
> Yes, exactly - we don't cope very well with that situation and we really
> ought to but since it's hard to trigger without trying in practice it's
> never been a priority.

Ugly... completely ugly idea for user space applications and operating
system... It's better for developers for ALSA soc part to pay enough
attention not only to their hardwares but also to application interfaces.

Please assume that a loaded module for SoC's sound interface which
supports Jack detection, and pulseaudio runs on the system. Then,
typically, the process listen to ALSA ctrl character device for Jack
detection.

In this case, when modules for codec or component are unloaded, what
happends? In worst case, pulseaudio process can kill the system in a
system call path or something else, because the modules for SoC's sound
interface is still loaded and userspace applications can execute system
calls via ALSA character devices.

It should be forbidden to build ALSA soc part as loadable kernel
modules. It's really danger...


Regards

Takashi Sakamoto


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Iwai]

On Thu, 04 Aug 2016 14:12:09 +0200,
Takashi Sakamoto wrote:
> 
> On Aug 4 2016 19:28, Mark Brown wrote:
> > On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
> >> On Jul 30 2016 07:08, Mark Brown wrote:
> > 
> >>> The card should be deinstantiated and reinstantiated whenever a
> >>> component driver unbinds and rebinds (respectively).  You'd need to
> >>> completely deregister the card to change the list of things it's
> >>> expecting currently.
> > 
> >> In a point of application interfaces, I guess that current implementation of
> >> ALSA soc part includes a bug that it's possible to unload codec or component
> >> modules when any ALSA character devices are opened. The framework has no
> >> codes to manage reference counting of character devices or loaded codecs,
> >> components.
> > 
> > Yes, exactly - we don't cope very well with that situation and we really
> > ought to but since it's hard to trigger without trying in practice it's
> > never been a priority.
> 
> Ugly... completely ugly idea for user space applications and operating
> system... It's better for developers for ALSA soc part to pay enough
> attention not only to their hardwares but also to application interfaces.
> 
> Please assume that a loaded module for SoC's sound interface which
> supports Jack detection, and pulseaudio runs on the system. Then,
> typically, the process listen to ALSA ctrl character device for Jack
> detection.
> 
> In this case, when modules for codec or component are unloaded, what
> happends?

You can't unload.  The module unload is already protected by the
proper module refcounting.

> In worst case, pulseaudio process can kill the system in a
> system call path or something else, because the modules for SoC's sound
> interface is still loaded and userspace applications can execute system
> calls via ALSA character devices.
> 
> It should be forbidden to build ALSA soc part as loadable kernel
> modules. It's really danger...

The only danger is about the dynamic unbinding, and it has nothing to
do with the module.  And, protecting the sysfs unbind itself is
trivial, too: as I already suggested, we can put the flags in
appropriate places.


Takashi

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Sakamoto]

On Aug 4 2016 21:27, Takashi Iwai wrote:
> On Thu, 04 Aug 2016 14:12:09 +0200,
> Takashi Sakamoto wrote:
>>
>> On Aug 4 2016 19:28, Mark Brown wrote:
>>> On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
>>>> On Jul 30 2016 07:08, Mark Brown wrote:
>>>
>>>>> The card should be deinstantiated and reinstantiated whenever a
>>>>> component driver unbinds and rebinds (respectively).  You'd need to
>>>>> completely deregister the card to change the list of things it's
>>>>> expecting currently.
>>>
>>>> In a point of application interfaces, I guess that current implementation of
>>>> ALSA soc part includes a bug that it's possible to unload codec or component
>>>> modules when any ALSA character devices are opened. The framework has no
>>>> codes to manage reference counting of character devices or loaded codecs,
>>>> components.
>>>
>>> Yes, exactly - we don't cope very well with that situation and we really
>>> ought to but since it's hard to trigger without trying in practice it's
>>> never been a priority.
>>
>> Ugly... completely ugly idea for user space applications and operating
>> system... It's better for developers for ALSA soc part to pay enough
>> attention not only to their hardwares but also to application interfaces.
>>
>> Please assume that a loaded module for SoC's sound interface which
>> supports Jack detection, and pulseaudio runs on the system. Then,
>> typically, the process listen to ALSA ctrl character device for Jack
>> detection.
>>
>> In this case, when modules for codec or component are unloaded, what
>> happends?
> 
> You can't unload.  The module unload is already protected by the
> proper module refcounting.

Hm. For my information, could you please show call graph to increment
the reference counter of codec/component modules when modules for SoC's
sound interfaces refer to them?


Regards

Takashi Sakamoto

Re: Question about struct snd_soc_dai() :: cpu_dai->codec

[Takashi Iwai]

On Thu, 04 Aug 2016 15:39:40 +0200,
Takashi Sakamoto wrote:
> 
> On Aug 4 2016 21:27, Takashi Iwai wrote:
> > On Thu, 04 Aug 2016 14:12:09 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> On Aug 4 2016 19:28, Mark Brown wrote:
> >>> On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
> >>>> On Jul 30 2016 07:08, Mark Brown wrote:
> >>>
> >>>>> The card should be deinstantiated and reinstantiated whenever a
> >>>>> component driver unbinds and rebinds (respectively).  You'd need to
> >>>>> completely deregister the card to change the list of things it's
> >>>>> expecting currently.
> >>>
> >>>> In a point of application interfaces, I guess that current implementation of
> >>>> ALSA soc part includes a bug that it's possible to unload codec or component
> >>>> modules when any ALSA character devices are opened. The framework has no
> >>>> codes to manage reference counting of character devices or loaded codecs,
> >>>> components.
> >>>
> >>> Yes, exactly - we don't cope very well with that situation and we really
> >>> ought to but since it's hard to trigger without trying in practice it's
> >>> never been a priority.
> >>
> >> Ugly... completely ugly idea for user space applications and operating
> >> system... It's better for developers for ALSA soc part to pay enough
> >> attention not only to their hardwares but also to application interfaces.
> >>
> >> Please assume that a loaded module for SoC's sound interface which
> >> supports Jack detection, and pulseaudio runs on the system. Then,
> >> typically, the process listen to ALSA ctrl character device for Jack
> >> detection.
> >>
> >> In this case, when modules for codec or component are unloaded, what
> >> happends?
> > 
> > You can't unload.  The module unload is already protected by the
> > proper module refcounting.
> 
> Hm. For my information, could you please show call graph to increment
> the reference counter of codec/component modules when modules for SoC's
> sound interfaces refer to them?

Just grep try_module_get() and module_put() calls.


Takashi

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

On Thu, Aug 04, 2016 at 02:38:33AM +0000, Kuninori Morimoto wrote:

> I agree to your opinion.
> OTOH, we would like to use/keep existing current all drivers.
> Thus, I think we need super many small and slow steps.
> Or, we need new ASoC2 ?

Small steps is how we do things in the kernel.  

> OTOH "driver" side can use existing CPU/Codec/Platform and/or AUX/compr
> etc as helper ? (I'm not sure detail of AUX/compr actually...)
> And each "component" has "dai".

Auxiliary devices are just CODECs that don't have DAIs hooked up.
Compressed DAIs are just DAIs for compressed audio.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

On Thu, Aug 04, 2016 at 10:21:10AM +0200, Lars-Peter Clausen wrote:

> To be honest I'd also get rid of DAIs has a top level concept. This image
> (http://metafoo.de/the_new_asoc.svg) is something I've put together awhile
> ago how I think we should lay things out if we do a major refactoring of the
> ASoC core.

I do think they have a useful purpose in representing the edges of chips
in a convenient bundle and inject configuration.  I too would like to
see them less prominent in the code internals but they are useful as an
interface.

> Obviously we can't convert everything at the same time and it will take a
> lot of time and effort to update all drivers to this new model. This is
> where the legacy bridge kicks in which still keeps the concept of 1 CPU, 1
> CODEC, 1 platform. If you want to use the advanced features of the new
> framework you have to update your driver, if you are OK with the current set
> of features just keep the drivers the way they are and use the legacy bridge
> that is automatically managed by the ASoC core.

I actually keep thinking that it might be easier to refactor the simple
CPU plus platform SoCs into components than anything else - there's
going to be gotchas in there but it seems like a useful string to pull
at.  Though realistically if we're going to get companies working on
this it's probably easier to get DPCM people started off.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec

[Kuninori Morimoto]

Hi Lars

Thank you for your idea

> To be honest I'd also get rid of DAIs has a top level concept. This image
> (http://metafoo.de/the_new_asoc.svg) is something I've put together awhile
> ago how I think we should lay things out if we do a major refactoring of the
> ASoC core.

I think most important ideas on this image is ALSA SoC new style has
 - "component" can have many "domain"
 - "domain" can connect to other "domain" by using "bridge"
 - "bridge" can connect to many "domain"s
 - "domain" has certain properties, and "bridge" translate it to other "domain"

Here, my opinions are...
 - Actually, I want to have more and more simple style.
   If "domain" can have its format / frequency etc, "domain" list/array
   without "bridge" is very enough ?
   or no distinction in "domain" and "bridge" ?
   because we can have many "domain" on new style ?
   something like
	struct snd_soc_node;
	struct snd_soc_domain {
		struct snd_soc_node;
	};
	struct snd_soc_bridge {
		struct snd_soc_node;
	};
   This case ALSA SoC cares snd_soc_node only;
 - If we have "bridge", I think "component property" can be
   "component needs to setup connected bridge's parameter in init time", and
   "component should follow bridge's parameter when playback time" is nicer ?
   It is easy to care about "crossbar" case ?

On your image,
DMA / Audio controller / CODEC are component inheritance struct,
Playback / Capture / Digital / Link / Analog are domain inheritance struct,
PCM / DAI / DAC / ADC / crossbar are bridge inheritance struct.
ALSA SoC framework cares "component", "domain", "bridge" only.
Each driver can create original structure if it was based on
"component" or "domain" or "bridge", or it can use soc-core's
basic helper (= CPU / Codec / Plaform etc).
Then, it is easy to add new device / connection in the future ?
And it is easy to use OF graph style on DT ?

struct snd_soc_component {

       /* we want to have rtd list here for hotplug purpose ? */

	struct list_head component_domain_head; 
};

struct snd_soc_domain {

	struct snd_soc_component *component; /* parent */

	struct list_head component_domain;
};

struct snd_soc_bridge {

	/* connected domain will sets available parameter */
	unsigned int fmt, ...;

	struct list_head runtime_bridge;
	struct snd_soc_domain *domains[0] /* domain array */
};

struct snd_soc_pcm_runtime {

	struct list_head runtime_bridge_head;
};

static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
{
	struct snd_soc_pcm_runtime *rtd = substream->private_data;
	struct snd_soc_bridge *bridge;
	struct snd_soc_domain *domain;

	for_each_bridge(bridge, rtd) {
		for_each_domain(domain, bridge) {
			domain->trigger(domain, bridge, rtd);
		}
	}
	...
}

> The dark grey boxes are bridges between domains and they translate
> properties from one domain to another. This can either be straight forward
> propagation like a PCM device where the samplerate in the source and target
> domain are the same, or it can modify the properties, e.g. a samplerate
> converter between two digital domains will change the samplerate according
> to the interpolation/decimation factor of the SRC.
> 
> In addition to just translating the properties a bridge can also translate
> the constraints on properties from one domain to another or even add its own
> constraints. E.g. if a CODEC has a constraint that it can run either with
> 48kHz or 96kHz this constraint is propagated through the bridges to the PCM
> devices so that the userspace application using the PCM is aware of this.
> Bridges can also add their own constraints e.g. a SRC might have multiple
> interpolation/decimation rates available, so it might say the samplerate in
> the target domain must be 1x or 2x that of the source domain. Going back to
> the example that means for a CODEC that supports either 48kHz or 96kHz and
> there is a SRC with interpolation factors of 1 or 2 then the PCM device will
> present the list of either 24kHz, 48kHz or 96kHz to the userspace application.

I think this is current DPCM's dai_link->be_hw_params_fixup portion ?
I think "bridge" can/should handle it ?


> A bridge is also not limited to one source and one sink. It can have
> multiple sources and multiple sinks e.g. for a crossbar with a complex
> routing matrix.

Do you mean runtime-path-change by this "crossbar" ?

--A1-+-B1--
     |
--A2-+-B2--

For example, like this ??

	> amixer set "A1-B1" ... // connect
	> aplay xxx
	> amixer set "A1 B1" ... // disconnect
	> amixer set "A1-B2" ... // connect
	> aplay xxx

Or having fixed path card ?

	> aplay -D plughw:0,0 xxx  // A1-B1 path
	> aplay -D plughw:0,1 xxx  // A1-B2 path
	> aplay -D plughw:0,2 xxx  // B1-A1 path
	> aplay -D plughw:0,3 xxx  // B1-B2 path
	...

> Obviously we can't convert everything at the same time and it will take a
> lot of time and effort to update all drivers to this new model. This is
> where the legacy bridge kicks in which still keeps the concept of 1 CPU, 1
> CODEC, 1 platform. If you want to use the advanced features of the new
> framework you have to update your driver, if you are OK with the current set
> of features just keep the drivers the way they are and use the legacy bridge
> that is automatically managed by the ASoC core.

I think it will be super x 10 long term project
Because of it, we need to consider about
 - "Big picture" and "details" of final style
 - consider how to goto there from current style

Best regards
---
Kuninori Morimoto