[alsa-devel] [RFC] bind/unbind SoC devices with deferred list
Hi Takashi, Mark, Lars
I worked for bind/unbind issue which was discussed before
http://thread.gmane.org/gmane.linux.alsa.devel/134062/focus=134076 http://thread.gmane.org/gmane.linux.alsa.devel/134283/focus=134287
I would like to know your opinion before sending patch to Greg
The problem is ASoC has card/platform/cpu/codec relationship for each other, but, we can unbind one of them, and then, other related devices doesn't know about it. Thus, kernel will error if we use sound after that.
Here is example of this issue. It unbind cpu (= rcar_sound), playback sound, and, error. And, we can't re-bind after this.
// check current sound card
> aplay -l **** List of PLAYBACK Hardware Devices **** card 0: rsnddai0ak4642h [rsnd-dai.0-ak4642-hifi], device 0: rsnd-dai.0-ak4642-hifi ak4642-hifi-0 [] Subdevices: 1/1 Subdevice #0: subdevice #0
// unbind cpu (= rcar_sound)
> echo ec500000.rcar_sound > /sys/bus/platform/drivers/rcar_sound/unbind
// recheck sound card // card doesn't know cpu was removed
> aplay -l **** List of PLAYBACK Hardware Devices **** card 0: rsnddai0ak4642h [rsnd-dai.0-ak4642-hifi], device 0: rsnd-dai.0-ak4642-hifi ak4642-hifi-0 [] Subdevices: 1/1 Subdevice #0: subdevice #0
// kernel error happen if playback
> aplay xxx.wav Unable to handle kernel NULL pointer dereference at virtual address 00000100 pgd = ee97c000 ...
// we can't re-bind cpu
> echo ec500000.rcar_sound > /sys/bus/platform/drivers/rcar_sound/bind -- freeze --
I hacked this issue to using deferred list, and created attached patch. it is all-in-one patch. but it solved my issue. I would like to know your opinion about this.
// check current sound card
> aplay -l **** List of PLAYBACK Hardware Devices **** card 0: rsnddai0ak4642h [rsnd-dai.0-ak4642-hifi], device 0: rsnd-dai.0-ak4642-hifi ak4642-hifi-0 [] Subdevices: 1/1 Subdevice #0: subdevice #0
// unbind cpu (= rcar_sound)
> echo ec500000.rcar_sound > /sys/bus/platform/drivers/rcar_sound/unbind (local debug message) release ec500000.rcar_sound (local debug message) release related device - sound (local debug message) related related device - 6-0012
// recheck sound card // it knows there is no available card
> aplay -l aplay: device_list:268: no soundcards found...
// re-bind lost cpu (= rcar_sound) // it re-bind sound card
> echo ec500000.rcar_sound > /sys/bus/platform/drivers/rcar_sound/bind rcar_sound ec500000.rcar_sound: probed asoc-simple-card sound: ak4642-hifi <-> ec500000.rcar_sound mapping ok
// let's unbind codec side (= ak4642) this time
> echo 6-0012 > /sys/bus/i2c/drivers/ak4642-codec/unbind (local debug message) release 6-0012 (local debug message) release related device - ec500000.rcar_sound (local debug message) related related device - sound
// recheck sound card // it knows there is no available card
> aplay -l aplay: device_list:268: no soundcards found...
// re-bind lost codec (= ak4642) // it re-bind sound card
> echo 6-0012 > /sys/bus/i2c/drivers/ak4642-codec/bind rcar_sound ec500000.rcar_sound: probed asoc-simple-card sound: ak4642-hifi <-> ec500000.rcar_sound mapping ok
--- patch --- From 9da5324a659ff596a2733f14b768b6b8c5a04f3b Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Wed, 22 Apr 2015 16:53:49 +0900 Subject: [PATCH] no Subject at this point
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- drivers/base/base.h | 1 + drivers/base/core.c | 9 +++++++++ drivers/base/dd.c | 21 ++++++++++++++++++++- include/linux/device.h | 1 + sound/soc/soc-core.c | 4 ++++ 5 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h index 251c5d3..1a71a52 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -74,6 +74,7 @@ struct device_private { struct klist_node knode_driver; struct klist_node knode_bus; struct list_head deferred_probe; + struct list_head related_dev; struct device *device; }; #define to_device_private_parent(obj) \ diff --git a/drivers/base/core.c b/drivers/base/core.c index 21d1303..643146f 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -948,9 +948,18 @@ int device_private_init(struct device *dev) klist_init(&dev->p->klist_children, klist_children_get, klist_children_put); INIT_LIST_HEAD(&dev->p->deferred_probe); + INIT_LIST_HEAD(&dev->p->related_dev); + return 0; }
+void device_related_device(struct device *dev, struct device *node) +{ + if (list_empty(&node->p->related_dev)) + list_add(&node->p->related_dev, &dev->p->related_dev); +} +EXPORT_SYMBOL_GPL(device_related_device); + /** * device_add - add device to device hierarchy. * @dev: device. diff --git a/drivers/base/dd.c b/drivers/base/dd.c index e843fdb..3d218c6 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -516,7 +516,7 @@ EXPORT_SYMBOL_GPL(driver_attach); * __device_release_driver() must be called with @dev lock held. * When called for a USB interface, @dev->parent lock must be held as well. */ -static void __device_release_driver(struct device *dev) +static void _device_release_driver(struct device *dev) { struct device_driver *drv;
@@ -552,6 +552,25 @@ static void __device_release_driver(struct device *dev) } }
+static void __device_release_driver(struct device *dev) +{ + struct device_private *dp, *d; + + /* release dev itself */ + _device_release_driver(dev); + + list_for_each_entry_safe(dp, d, &dev->p->related_dev, related_dev) { + /* + * temporarily, release related device. + * these are push to deferred_probe_pending_list. + * It can be re-probed if it didn't call device_del() + */ + _device_release_driver(dp->device); + driver_deferred_probe_add(dp->device); + list_del_init(&dp->related_dev); + } +} + /** * device_release_driver - manually detach device from driver. * @dev: device. diff --git a/include/linux/device.h b/include/linux/device.h index 6558af9..2b81804 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -924,6 +924,7 @@ void driver_init(void); extern int __must_check device_register(struct device *dev); extern void device_unregister(struct device *dev); extern void device_initialize(struct device *dev); +extern void device_related_device(struct device *dev, struct device *node); extern int __must_check device_add(struct device *dev); extern void device_del(struct device *dev); extern int device_for_each_child(struct device *dev, void *data, diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 2373252..01c0d56 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -22,6 +22,7 @@ * o Support TDM on PCM and I2S */
+#include <linux/device.h> #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/init.h> @@ -946,6 +947,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card, int num) dai_link->cpu_dai_name); return -EPROBE_DEFER; } + device_related_device(card->dev, rtd->cpu_dai->dev);
rtd->num_codecs = dai_link->num_codecs;
@@ -962,6 +964,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card, int num) /* Single codec links expect codec and codec_dai in runtime data */ rtd->codec_dai = codec_dais[0]; rtd->codec = rtd->codec_dai->codec; + device_related_device(card->dev, rtd->codec->dev);
/* if there's no platform we match on the empty platform */ platform_name = dai_link->platform_name; @@ -986,6 +989,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card, int num) dai_link->platform_name); return -EPROBE_DEFER; } + device_related_device(card->dev, rtd->platform->dev);
card->num_rtd++;
At Wed, 22 Apr 2015 08:51:21 +0000, Kuninori Morimoto wrote:
Hi Takashi, Mark, Lars
I worked for bind/unbind issue which was discussed before
http://thread.gmane.org/gmane.linux.alsa.devel/134062/focus=134076 http://thread.gmane.org/gmane.linux.alsa.devel/134283/focus=134287
I would like to know your opinion before sending patch to Greg
The problem is ASoC has card/platform/cpu/codec relationship for each other, but, we can unbind one of them, and then, other related devices doesn't know about it. Thus, kernel will error if we use sound after that.
Wouldn't the standard component (linux/component.h) be able to solve such a problem?
Takashi
Here is example of this issue. It unbind cpu (= rcar_sound), playback sound, and, error. And, we can't re-bind after this.
// check current sound card
aplay -l
**** List of PLAYBACK Hardware Devices **** card 0: rsnddai0ak4642h [rsnd-dai.0-ak4642-hifi], device 0: rsnd-dai.0-ak4642-hifi ak4642-hifi-0 [] Subdevices: 1/1 Subdevice #0: subdevice #0
// unbind cpu (= rcar_sound)
echo ec500000.rcar_sound > /sys/bus/platform/drivers/rcar_sound/unbind
// recheck sound card // card doesn't know cpu was removed
aplay -l
**** List of PLAYBACK Hardware Devices **** card 0: rsnddai0ak4642h [rsnd-dai.0-ak4642-hifi], device 0: rsnd-dai.0-ak4642-hifi ak4642-hifi-0 [] Subdevices: 1/1 Subdevice #0: subdevice #0
// kernel error happen if playback
aplay xxx.wav
Unable to handle kernel NULL pointer dereference at virtual address 00000100 pgd = ee97c000 ...
// we can't re-bind cpu
echo ec500000.rcar_sound > /sys/bus/platform/drivers/rcar_sound/bind
-- freeze --I hacked this issue to using deferred list, and created attached patch. it is all-in-one patch. but it solved my issue. I would like to know your opinion about this.
// check current sound card
aplay -l
**** List of PLAYBACK Hardware Devices **** card 0: rsnddai0ak4642h [rsnd-dai.0-ak4642-hifi], device 0: rsnd-dai.0-ak4642-hifi ak4642-hifi-0 [] Subdevices: 1/1 Subdevice #0: subdevice #0
// unbind cpu (= rcar_sound)
echo ec500000.rcar_sound > /sys/bus/platform/drivers/rcar_sound/unbind
(local debug message) release ec500000.rcar_sound (local debug message) release related device - sound (local debug message) related related device - 6-0012// recheck sound card // it knows there is no available card
aplay -l
aplay: device_list:268: no soundcards found...
// re-bind lost cpu (= rcar_sound) // it re-bind sound card
echo ec500000.rcar_sound > /sys/bus/platform/drivers/rcar_sound/bind
rcar_sound ec500000.rcar_sound: probed asoc-simple-card sound: ak4642-hifi <-> ec500000.rcar_sound mapping ok
// let's unbind codec side (= ak4642) this time
echo 6-0012 > /sys/bus/i2c/drivers/ak4642-codec/unbind
(local debug message) release 6-0012 (local debug message) release related device - ec500000.rcar_sound (local debug message) related related device - sound
// recheck sound card // it knows there is no available card
aplay -l
aplay: device_list:268: no soundcards found...
// re-bind lost codec (= ak4642) // it re-bind sound card
echo 6-0012 > /sys/bus/i2c/drivers/ak4642-codec/bind
rcar_sound ec500000.rcar_sound: probed asoc-simple-card sound: ak4642-hifi <-> ec500000.rcar_sound mapping ok
--- patch --- From 9da5324a659ff596a2733f14b768b6b8c5a04f3b Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Wed, 22 Apr 2015 16:53:49 +0900 Subject: [PATCH] no Subject at this point
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
drivers/base/base.h | 1 + drivers/base/core.c | 9 +++++++++ drivers/base/dd.c | 21 ++++++++++++++++++++- include/linux/device.h | 1 + sound/soc/soc-core.c | 4 ++++ 5 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h index 251c5d3..1a71a52 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -74,6 +74,7 @@ struct device_private { struct klist_node knode_driver; struct klist_node knode_bus; struct list_head deferred_probe;
- struct list_head related_dev; struct device *device;
}; #define to_device_private_parent(obj) \ diff --git a/drivers/base/core.c b/drivers/base/core.c index 21d1303..643146f 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -948,9 +948,18 @@ int device_private_init(struct device *dev) klist_init(&dev->p->klist_children, klist_children_get, klist_children_put); INIT_LIST_HEAD(&dev->p->deferred_probe);
- INIT_LIST_HEAD(&dev->p->related_dev);
- return 0;
}
+void device_related_device(struct device *dev, struct device *node) +{
- if (list_empty(&node->p->related_dev))
list_add(&node->p->related_dev, &dev->p->related_dev);+} +EXPORT_SYMBOL_GPL(device_related_device);
/**
- device_add - add device to device hierarchy.
- @dev: device.
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index e843fdb..3d218c6 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -516,7 +516,7 @@ EXPORT_SYMBOL_GPL(driver_attach);
- __device_release_driver() must be called with @dev lock held.
- When called for a USB interface, @dev->parent lock must be held as well.
*/ -static void __device_release_driver(struct device *dev) +static void _device_release_driver(struct device *dev) { struct device_driver *drv;
@@ -552,6 +552,25 @@ static void __device_release_driver(struct device *dev) } }
+static void __device_release_driver(struct device *dev) +{
- struct device_private *dp, *d;
- /* release dev itself */
- _device_release_driver(dev);
- list_for_each_entry_safe(dp, d, &dev->p->related_dev, related_dev) {
/** temporarily, release related device.* these are push to deferred_probe_pending_list.* It can be re-probed if it didn't call device_del()*/_device_release_driver(dp->device);driver_deferred_probe_add(dp->device);list_del_init(&dp->related_dev);- }
+}
/**
- device_release_driver - manually detach device from driver.
- @dev: device.
diff --git a/include/linux/device.h b/include/linux/device.h index 6558af9..2b81804 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -924,6 +924,7 @@ void driver_init(void); extern int __must_check device_register(struct device *dev); extern void device_unregister(struct device *dev); extern void device_initialize(struct device *dev); +extern void device_related_device(struct device *dev, struct device *node); extern int __must_check device_add(struct device *dev); extern void device_del(struct device *dev); extern int device_for_each_child(struct device *dev, void *data, diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 2373252..01c0d56 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -22,6 +22,7 @@
- o Support TDM on PCM and I2S
*/
+#include <linux/device.h> #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/init.h> @@ -946,6 +947,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card, int num) dai_link->cpu_dai_name); return -EPROBE_DEFER; }
device_related_device(card->dev, rtd->cpu_dai->dev);
rtd->num_codecs = dai_link->num_codecs;
@@ -962,6 +964,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card, int num) /* Single codec links expect codec and codec_dai in runtime data */ rtd->codec_dai = codec_dais[0]; rtd->codec = rtd->codec_dai->codec;
device_related_device(card->dev, rtd->codec->dev);
/* if there's no platform we match on the empty platform */ platform_name = dai_link->platform_name;
@@ -986,6 +989,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card, int num) dai_link->platform_name); return -EPROBE_DEFER; }
device_related_device(card->dev, rtd->platform->dev);
card->num_rtd++;
-- 1.9.1
Best regards
Kuninori Morimoto
On 04/22/2015 02:02 PM, Takashi Iwai wrote:
At Wed, 22 Apr 2015 08:51:21 +0000, Kuninori Morimoto wrote:
Hi Takashi, Mark, Lars
I worked for bind/unbind issue which was discussed before
http://thread.gmane.org/gmane.linux.alsa.devel/134062/focus=134076 http://thread.gmane.org/gmane.linux.alsa.devel/134283/focus=134287
I would like to know your opinion before sending patch to Greg
The problem is ASoC has card/platform/cpu/codec relationship for each other, but, we can unbind one of them, and then, other related devices doesn't know about it. Thus, kernel will error if we use sound after that.
Wouldn't the standard component (linux/component.h) be able to solve such a problem?
At least that is the idea. The long term plan for ASoC is to switch over to the component framework.
- Lars
Hi
I worked for bind/unbind issue which was discussed before
http://thread.gmane.org/gmane.linux.alsa.devel/134062/focus=134076 http://thread.gmane.org/gmane.linux.alsa.devel/134283/focus=134287
I would like to know your opinion before sending patch to Greg
The problem is ASoC has card/platform/cpu/codec relationship for each other, but, we can unbind one of them, and then, other related devices doesn't know about it. Thus, kernel will error if we use sound after that.
Wouldn't the standard component (linux/component.h) be able to solve such a problem?
At least that is the idea. The long term plan for ASoC is to switch over to the component framework.
Wow ! I didn't know this.
Best regards --- Kuninori Morimoto
participants (3)
-
Kuninori Morimoto -
Lars-Peter Clausen -
Takashi Iwai