| Message ID | 87383sh7i6.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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++;