diff mbox

[RFC] bind/unbind SoC devices with deferred list

Message ID 87383sh7i6.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kuninori Morimoto April 22, 2015, 8:51 a.m. UTC
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(-)

Comments

Takashi Iwai April 22, 2015, 12:02 p.m. UTC | #1
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
>
Lars-Peter Clausen April 22, 2015, 12:06 p.m. UTC | #2
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
Kuninori Morimoto April 23, 2015, 12:19 a.m. UTC | #3
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 mbox

Patch

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++;