diff mbox

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

Message ID s5hpopy6dbi.wl-tiwai@suse.de (mailing list archive)
State RFC
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Takashi Iwai July 27, 2016, 8:22 p.m. UTC
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:


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

Comments

Vinod Koul July 28, 2016, 3:46 a.m. UTC | #1
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.
Lars-Peter Clausen July 28, 2016, 8:33 p.m. UTC | #2
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
Takashi Iwai July 28, 2016, 8:42 p.m. UTC | #3
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
Lars-Peter Clausen July 28, 2016, 8:43 p.m. UTC | #4
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.
Mark Brown July 28, 2016, 8:44 p.m. UTC | #5
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.
Takashi Sakamoto July 29, 2016, 12:30 a.m. UTC | #6
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
Lars-Peter Clausen July 29, 2016, 9:07 a.m. UTC | #7
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
Vinod Koul July 29, 2016, 4:07 p.m. UTC | #8
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...
Takashi Iwai July 29, 2016, 8:41 p.m. UTC | #9
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
Takashi Sakamoto July 29, 2016, 9:45 p.m. UTC | #10
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
Mark Brown July 29, 2016, 10:08 p.m. UTC | #11
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.
Takashi Sakamoto Aug. 4, 2016, 3:17 a.m. UTC | #12
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
Mark Brown Aug. 4, 2016, 10:28 a.m. UTC | #13
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.
Takashi Sakamoto Aug. 4, 2016, 12:12 p.m. UTC | #14
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
Takashi Iwai Aug. 4, 2016, 12:27 p.m. UTC | #15
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
Takashi Sakamoto Aug. 4, 2016, 1:39 p.m. UTC | #16
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
Takashi Iwai Aug. 4, 2016, 1:52 p.m. UTC | #17
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
diff mbox

Patch

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,