All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ASoC: intel: Replace kthread with work
@ 2016-11-28 15:32 Takashi Iwai
  2016-11-28 15:52 ` Vinod Koul
  2016-11-30 18:07 ` Applied "ASoC: intel: Replace kthread with work" to the asoc tree Mark Brown
  0 siblings, 2 replies; 4+ messages in thread
From: Takashi Iwai @ 2016-11-28 15:32 UTC (permalink / raw)
  To: Mark Brown; +Cc: Vinod Koul, alsa-devel, Liam Girdwood, Pierre-Louis Bossart

The usage pattern of kthread worker in Intel SST drivers can be
replaced gracefully with the normal workqueue, which is more light-
weight and easier to manage in general.  Let's do it.

While in the replacement, move the schedule_work() call inside the
spinlock for excluding the race, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---

Vinod, I tested this only lightly, so it'd be helpful if the patch is
checked on the affected devices before merging.

 sound/soc/intel/baytrail/sst-baytrail-ipc.c |  3 +-
 sound/soc/intel/common/sst-ipc.c            | 58 ++++++++++-------------------
 sound/soc/intel/common/sst-ipc.h            |  4 +-
 sound/soc/intel/haswell/sst-haswell-ipc.c   |  3 +-
 sound/soc/intel/skylake/skl-sst-cldma.c     |  1 -
 sound/soc/intel/skylake/skl-sst-ipc.c       |  2 +-
 sound/soc/intel/skylake/skl-sst-ipc.h       |  1 -
 7 files changed, 24 insertions(+), 48 deletions(-)

diff --git a/sound/soc/intel/baytrail/sst-baytrail-ipc.c b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
index 7ab14ce65a73..260447da32b8 100644
--- a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
+++ b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
@@ -23,7 +23,6 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
-#include <linux/kthread.h>
 #include <linux/firmware.h>
 #include <linux/io.h>
 #include <asm/div64.h>
@@ -338,7 +337,7 @@ static irqreturn_t sst_byt_irq_thread(int irq, void *context)
 	spin_unlock_irqrestore(&sst->spinlock, flags);
 
 	/* continue to send any remaining messages... */
-	kthread_queue_work(&ipc->kworker, &ipc->kwork);
+	schedule_work(&ipc->kwork);
 
 	return IRQ_HANDLED;
 }
diff --git a/sound/soc/intel/common/sst-ipc.c b/sound/soc/intel/common/sst-ipc.c
index 6c672ac79cce..d20356255d0c 100644
--- a/sound/soc/intel/common/sst-ipc.c
+++ b/sound/soc/intel/common/sst-ipc.c
@@ -26,7 +26,6 @@
 #include <linux/sched.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
-#include <linux/kthread.h>
 #include <sound/asound.h>
 
 #include "sst-dsp.h"
@@ -109,10 +108,9 @@ static int ipc_tx_message(struct sst_generic_ipc *ipc, u64 header,
 		ipc->ops.tx_data_copy(msg, tx_data, tx_bytes);
 
 	list_add_tail(&msg->list, &ipc->tx_list);
+	schedule_work(&ipc->kwork);
 	spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
 
-	kthread_queue_work(&ipc->kworker, &ipc->kwork);
-
 	if (wait)
 		return tx_wait_done(ipc, msg, rx_data);
 	else
@@ -156,35 +154,32 @@ static int msg_empty_list_init(struct sst_generic_ipc *ipc)
 	return -ENOMEM;
 }
 
-static void ipc_tx_msgs(struct kthread_work *work)
+static void ipc_tx_msgs(struct work_struct *work)
 {
 	struct sst_generic_ipc *ipc =
 		container_of(work, struct sst_generic_ipc, kwork);
 	struct ipc_message *msg;
-	unsigned long flags;
 
-	spin_lock_irqsave(&ipc->dsp->spinlock, flags);
+	spin_lock_irq(&ipc->dsp->spinlock);
 
-	if (list_empty(&ipc->tx_list) || ipc->pending) {
-		spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
-		return;
-	}
-
-	/* if the DSP is busy, we will TX messages after IRQ.
-	 * also postpone if we are in the middle of procesing completion irq*/
-	if (ipc->ops.is_dsp_busy && ipc->ops.is_dsp_busy(ipc->dsp)) {
-		dev_dbg(ipc->dev, "ipc_tx_msgs dsp busy\n");
-		spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
-		return;
-	}
+	while (!list_empty(&ipc->tx_list) && !ipc->pending) {
+		/* if the DSP is busy, we will TX messages after IRQ.
+		 * also postpone if we are in the middle of processing
+		 * completion irq
+		 */
+		if (ipc->ops.is_dsp_busy && ipc->ops.is_dsp_busy(ipc->dsp)) {
+			dev_dbg(ipc->dev, "ipc_tx_msgs dsp busy\n");
+			break;
+		}
 
-	msg = list_first_entry(&ipc->tx_list, struct ipc_message, list);
-	list_move(&msg->list, &ipc->rx_list);
+		msg = list_first_entry(&ipc->tx_list, struct ipc_message, list);
+		list_move(&msg->list, &ipc->rx_list);
 
-	if (ipc->ops.tx_msg != NULL)
-		ipc->ops.tx_msg(ipc, msg);
+		if (ipc->ops.tx_msg != NULL)
+			ipc->ops.tx_msg(ipc, msg);
+	}
 
-	spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
+	spin_unlock_irq(&ipc->dsp->spinlock);
 }
 
 int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, u64 header,
@@ -280,19 +275,7 @@ int sst_ipc_init(struct sst_generic_ipc *ipc)
 	if (ret < 0)
 		return -ENOMEM;
 
-	/* start the IPC message thread */
-	kthread_init_worker(&ipc->kworker);
-	ipc->tx_thread = kthread_run(kthread_worker_fn,
-					&ipc->kworker, "%s",
-					dev_name(ipc->dev));
-	if (IS_ERR(ipc->tx_thread)) {
-		dev_err(ipc->dev, "error: failed to create message TX task\n");
-		ret = PTR_ERR(ipc->tx_thread);
-		kfree(ipc->msg);
-		return ret;
-	}
-
-	kthread_init_work(&ipc->kwork, ipc_tx_msgs);
+	INIT_WORK(&ipc->kwork, ipc_tx_msgs);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(sst_ipc_init);
@@ -301,8 +284,7 @@ void sst_ipc_fini(struct sst_generic_ipc *ipc)
 {
 	int i;
 
-	if (ipc->tx_thread)
-		kthread_stop(ipc->tx_thread);
+	cancel_work_sync(&ipc->kwork);
 
 	if (ipc->msg) {
 		for (i = 0; i < IPC_EMPTY_LIST_SIZE; i++) {
diff --git a/sound/soc/intel/common/sst-ipc.h b/sound/soc/intel/common/sst-ipc.h
index ceb7e468a3fa..ea79cff5c6cc 100644
--- a/sound/soc/intel/common/sst-ipc.h
+++ b/sound/soc/intel/common/sst-ipc.h
@@ -23,7 +23,6 @@
 #include <linux/list.h>
 #include <linux/workqueue.h>
 #include <linux/sched.h>
-#include <linux/kthread.h>
 
 #define IPC_MAX_MAILBOX_BYTES	256
 
@@ -65,8 +64,7 @@ struct sst_generic_ipc {
 	struct list_head empty_list;
 	wait_queue_head_t wait_txq;
 	struct task_struct *tx_thread;
-	struct kthread_worker kworker;
-	struct kthread_work kwork;
+	struct work_struct kwork;
 	bool pending;
 	struct ipc_message *msg;
 	int tx_data_max_size;
diff --git a/sound/soc/intel/haswell/sst-haswell-ipc.c b/sound/soc/intel/haswell/sst-haswell-ipc.c
index e432a31fd9f2..a3459d1682a6 100644
--- a/sound/soc/intel/haswell/sst-haswell-ipc.c
+++ b/sound/soc/intel/haswell/sst-haswell-ipc.c
@@ -26,7 +26,6 @@
 #include <linux/delay.h>
 #include <linux/sched.h>
 #include <linux/platform_device.h>
-#include <linux/kthread.h>
 #include <linux/firmware.h>
 #include <linux/dma-mapping.h>
 #include <linux/debugfs.h>
@@ -818,7 +817,7 @@ static irqreturn_t hsw_irq_thread(int irq, void *context)
 	spin_unlock_irqrestore(&sst->spinlock, flags);
 
 	/* continue to send any remaining messages... */
-	kthread_queue_work(&ipc->kworker, &ipc->kwork);
+	schedule_work(&ipc->kwork);
 
 	return IRQ_HANDLED;
 }
diff --git a/sound/soc/intel/skylake/skl-sst-cldma.c b/sound/soc/intel/skylake/skl-sst-cldma.c
index efa2532114ba..c9f6d87381db 100644
--- a/sound/soc/intel/skylake/skl-sst-cldma.c
+++ b/sound/soc/intel/skylake/skl-sst-cldma.c
@@ -17,7 +17,6 @@
 
 #include <linux/device.h>
 #include <linux/mm.h>
-#include <linux/kthread.h>
 #include <linux/delay.h>
 #include "../common/sst-dsp.h"
 #include "../common/sst-dsp-priv.h"
diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c
index 797cf4053235..499c0b15e5d0 100644
--- a/sound/soc/intel/skylake/skl-sst-ipc.c
+++ b/sound/soc/intel/skylake/skl-sst-ipc.c
@@ -464,7 +464,7 @@ irqreturn_t skl_dsp_irq_thread_handler(int irq, void *context)
 	skl_ipc_int_enable(dsp);
 
 	/* continue to send any remaining messages... */
-	kthread_queue_work(&ipc->kworker, &ipc->kwork);
+	schedule_work(&ipc->kwork);
 
 	return IRQ_HANDLED;
 }
diff --git a/sound/soc/intel/skylake/skl-sst-ipc.h b/sound/soc/intel/skylake/skl-sst-ipc.h
index 0334ed4af031..5c37442a6db4 100644
--- a/sound/soc/intel/skylake/skl-sst-ipc.h
+++ b/sound/soc/intel/skylake/skl-sst-ipc.h
@@ -16,7 +16,6 @@
 #ifndef __SKL_IPC_H
 #define __SKL_IPC_H
 
-#include <linux/kthread.h>
 #include <linux/irqreturn.h>
 #include "../common/sst-ipc.h"
 
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] ASoC: intel: Replace kthread with work
  2016-11-28 15:32 [PATCH] ASoC: intel: Replace kthread with work Takashi Iwai
@ 2016-11-28 15:52 ` Vinod Koul
  2016-11-29 13:43   ` Vinod Koul
  2016-11-30 18:07 ` Applied "ASoC: intel: Replace kthread with work" to the asoc tree Mark Brown
  1 sibling, 1 reply; 4+ messages in thread
From: Vinod Koul @ 2016-11-28 15:52 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Mark Brown, Liam Girdwood, Pierre-Louis Bossart

On Mon, Nov 28, 2016 at 04:32:19PM +0100, Takashi Iwai wrote:
> The usage pattern of kthread worker in Intel SST drivers can be
> replaced gracefully with the normal workqueue, which is more light-
> weight and easier to manage in general.  Let's do it.
> 
> While in the replacement, move the schedule_work() call inside the
> spinlock for excluding the race, too.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> 
> Vinod, I tested this only lightly, so it'd be helpful if the patch is
> checked on the affected devices before merging.

We had  apatch done last week internally for this, but this one is fine too.

I will get it verfied on SKL tomorrow and get back

Thanks

> 
>  sound/soc/intel/baytrail/sst-baytrail-ipc.c |  3 +-
>  sound/soc/intel/common/sst-ipc.c            | 58 ++++++++++-------------------
>  sound/soc/intel/common/sst-ipc.h            |  4 +-
>  sound/soc/intel/haswell/sst-haswell-ipc.c   |  3 +-
>  sound/soc/intel/skylake/skl-sst-cldma.c     |  1 -
>  sound/soc/intel/skylake/skl-sst-ipc.c       |  2 +-
>  sound/soc/intel/skylake/skl-sst-ipc.h       |  1 -
>  7 files changed, 24 insertions(+), 48 deletions(-)
> 
> diff --git a/sound/soc/intel/baytrail/sst-baytrail-ipc.c b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> index 7ab14ce65a73..260447da32b8 100644
> --- a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> +++ b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> @@ -23,7 +23,6 @@
>  #include <linux/slab.h>
>  #include <linux/delay.h>
>  #include <linux/platform_device.h>
> -#include <linux/kthread.h>
>  #include <linux/firmware.h>
>  #include <linux/io.h>
>  #include <asm/div64.h>
> @@ -338,7 +337,7 @@ static irqreturn_t sst_byt_irq_thread(int irq, void *context)
>  	spin_unlock_irqrestore(&sst->spinlock, flags);
>  
>  	/* continue to send any remaining messages... */
> -	kthread_queue_work(&ipc->kworker, &ipc->kwork);
> +	schedule_work(&ipc->kwork);
>  
>  	return IRQ_HANDLED;
>  }
> diff --git a/sound/soc/intel/common/sst-ipc.c b/sound/soc/intel/common/sst-ipc.c
> index 6c672ac79cce..d20356255d0c 100644
> --- a/sound/soc/intel/common/sst-ipc.c
> +++ b/sound/soc/intel/common/sst-ipc.c
> @@ -26,7 +26,6 @@
>  #include <linux/sched.h>
>  #include <linux/delay.h>
>  #include <linux/platform_device.h>
> -#include <linux/kthread.h>
>  #include <sound/asound.h>
>  
>  #include "sst-dsp.h"
> @@ -109,10 +108,9 @@ static int ipc_tx_message(struct sst_generic_ipc *ipc, u64 header,
>  		ipc->ops.tx_data_copy(msg, tx_data, tx_bytes);
>  
>  	list_add_tail(&msg->list, &ipc->tx_list);
> +	schedule_work(&ipc->kwork);
>  	spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
>  
> -	kthread_queue_work(&ipc->kworker, &ipc->kwork);
> -
>  	if (wait)
>  		return tx_wait_done(ipc, msg, rx_data);
>  	else
> @@ -156,35 +154,32 @@ static int msg_empty_list_init(struct sst_generic_ipc *ipc)
>  	return -ENOMEM;
>  }
>  
> -static void ipc_tx_msgs(struct kthread_work *work)
> +static void ipc_tx_msgs(struct work_struct *work)
>  {
>  	struct sst_generic_ipc *ipc =
>  		container_of(work, struct sst_generic_ipc, kwork);
>  	struct ipc_message *msg;
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&ipc->dsp->spinlock, flags);
> +	spin_lock_irq(&ipc->dsp->spinlock);
>  
> -	if (list_empty(&ipc->tx_list) || ipc->pending) {
> -		spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
> -		return;
> -	}
> -
> -	/* if the DSP is busy, we will TX messages after IRQ.
> -	 * also postpone if we are in the middle of procesing completion irq*/
> -	if (ipc->ops.is_dsp_busy && ipc->ops.is_dsp_busy(ipc->dsp)) {
> -		dev_dbg(ipc->dev, "ipc_tx_msgs dsp busy\n");
> -		spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
> -		return;
> -	}
> +	while (!list_empty(&ipc->tx_list) && !ipc->pending) {
> +		/* if the DSP is busy, we will TX messages after IRQ.
> +		 * also postpone if we are in the middle of processing
> +		 * completion irq
> +		 */
> +		if (ipc->ops.is_dsp_busy && ipc->ops.is_dsp_busy(ipc->dsp)) {
> +			dev_dbg(ipc->dev, "ipc_tx_msgs dsp busy\n");
> +			break;
> +		}
>  
> -	msg = list_first_entry(&ipc->tx_list, struct ipc_message, list);
> -	list_move(&msg->list, &ipc->rx_list);
> +		msg = list_first_entry(&ipc->tx_list, struct ipc_message, list);
> +		list_move(&msg->list, &ipc->rx_list);
>  
> -	if (ipc->ops.tx_msg != NULL)
> -		ipc->ops.tx_msg(ipc, msg);
> +		if (ipc->ops.tx_msg != NULL)
> +			ipc->ops.tx_msg(ipc, msg);
> +	}
>  
> -	spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
> +	spin_unlock_irq(&ipc->dsp->spinlock);
>  }
>  
>  int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, u64 header,
> @@ -280,19 +275,7 @@ int sst_ipc_init(struct sst_generic_ipc *ipc)
>  	if (ret < 0)
>  		return -ENOMEM;
>  
> -	/* start the IPC message thread */
> -	kthread_init_worker(&ipc->kworker);
> -	ipc->tx_thread = kthread_run(kthread_worker_fn,
> -					&ipc->kworker, "%s",
> -					dev_name(ipc->dev));
> -	if (IS_ERR(ipc->tx_thread)) {
> -		dev_err(ipc->dev, "error: failed to create message TX task\n");
> -		ret = PTR_ERR(ipc->tx_thread);
> -		kfree(ipc->msg);
> -		return ret;
> -	}
> -
> -	kthread_init_work(&ipc->kwork, ipc_tx_msgs);
> +	INIT_WORK(&ipc->kwork, ipc_tx_msgs);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(sst_ipc_init);
> @@ -301,8 +284,7 @@ void sst_ipc_fini(struct sst_generic_ipc *ipc)
>  {
>  	int i;
>  
> -	if (ipc->tx_thread)
> -		kthread_stop(ipc->tx_thread);
> +	cancel_work_sync(&ipc->kwork);
>  
>  	if (ipc->msg) {
>  		for (i = 0; i < IPC_EMPTY_LIST_SIZE; i++) {
> diff --git a/sound/soc/intel/common/sst-ipc.h b/sound/soc/intel/common/sst-ipc.h
> index ceb7e468a3fa..ea79cff5c6cc 100644
> --- a/sound/soc/intel/common/sst-ipc.h
> +++ b/sound/soc/intel/common/sst-ipc.h
> @@ -23,7 +23,6 @@
>  #include <linux/list.h>
>  #include <linux/workqueue.h>
>  #include <linux/sched.h>
> -#include <linux/kthread.h>
>  
>  #define IPC_MAX_MAILBOX_BYTES	256
>  
> @@ -65,8 +64,7 @@ struct sst_generic_ipc {
>  	struct list_head empty_list;
>  	wait_queue_head_t wait_txq;
>  	struct task_struct *tx_thread;
> -	struct kthread_worker kworker;
> -	struct kthread_work kwork;
> +	struct work_struct kwork;
>  	bool pending;
>  	struct ipc_message *msg;
>  	int tx_data_max_size;
> diff --git a/sound/soc/intel/haswell/sst-haswell-ipc.c b/sound/soc/intel/haswell/sst-haswell-ipc.c
> index e432a31fd9f2..a3459d1682a6 100644
> --- a/sound/soc/intel/haswell/sst-haswell-ipc.c
> +++ b/sound/soc/intel/haswell/sst-haswell-ipc.c
> @@ -26,7 +26,6 @@
>  #include <linux/delay.h>
>  #include <linux/sched.h>
>  #include <linux/platform_device.h>
> -#include <linux/kthread.h>
>  #include <linux/firmware.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/debugfs.h>
> @@ -818,7 +817,7 @@ static irqreturn_t hsw_irq_thread(int irq, void *context)
>  	spin_unlock_irqrestore(&sst->spinlock, flags);
>  
>  	/* continue to send any remaining messages... */
> -	kthread_queue_work(&ipc->kworker, &ipc->kwork);
> +	schedule_work(&ipc->kwork);
>  
>  	return IRQ_HANDLED;
>  }
> diff --git a/sound/soc/intel/skylake/skl-sst-cldma.c b/sound/soc/intel/skylake/skl-sst-cldma.c
> index efa2532114ba..c9f6d87381db 100644
> --- a/sound/soc/intel/skylake/skl-sst-cldma.c
> +++ b/sound/soc/intel/skylake/skl-sst-cldma.c
> @@ -17,7 +17,6 @@
>  
>  #include <linux/device.h>
>  #include <linux/mm.h>
> -#include <linux/kthread.h>
>  #include <linux/delay.h>
>  #include "../common/sst-dsp.h"
>  #include "../common/sst-dsp-priv.h"
> diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c
> index 797cf4053235..499c0b15e5d0 100644
> --- a/sound/soc/intel/skylake/skl-sst-ipc.c
> +++ b/sound/soc/intel/skylake/skl-sst-ipc.c
> @@ -464,7 +464,7 @@ irqreturn_t skl_dsp_irq_thread_handler(int irq, void *context)
>  	skl_ipc_int_enable(dsp);
>  
>  	/* continue to send any remaining messages... */
> -	kthread_queue_work(&ipc->kworker, &ipc->kwork);
> +	schedule_work(&ipc->kwork);
>  
>  	return IRQ_HANDLED;
>  }
> diff --git a/sound/soc/intel/skylake/skl-sst-ipc.h b/sound/soc/intel/skylake/skl-sst-ipc.h
> index 0334ed4af031..5c37442a6db4 100644
> --- a/sound/soc/intel/skylake/skl-sst-ipc.h
> +++ b/sound/soc/intel/skylake/skl-sst-ipc.h
> @@ -16,7 +16,6 @@
>  #ifndef __SKL_IPC_H
>  #define __SKL_IPC_H
>  
> -#include <linux/kthread.h>
>  #include <linux/irqreturn.h>
>  #include "../common/sst-ipc.h"
>  
> -- 
> 2.10.2
> 

-- 
~Vinod

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ASoC: intel: Replace kthread with work
  2016-11-28 15:52 ` Vinod Koul
@ 2016-11-29 13:43   ` Vinod Koul
  0 siblings, 0 replies; 4+ messages in thread
From: Vinod Koul @ 2016-11-29 13:43 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Mark Brown, Liam Girdwood, Pierre-Louis Bossart

On Mon, Nov 28, 2016 at 09:22:36PM +0530, Vinod Koul wrote:
> On Mon, Nov 28, 2016 at 04:32:19PM +0100, Takashi Iwai wrote:
> > The usage pattern of kthread worker in Intel SST drivers can be
> > replaced gracefully with the normal workqueue, which is more light-
> > weight and easier to manage in general.  Let's do it.
> > 
> > While in the replacement, move the schedule_work() call inside the
> > spinlock for excluding the race, too.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > 
> > Vinod, I tested this only lightly, so it'd be helpful if the patch is
> > checked on the affected devices before merging.
> 
> We had  apatch done last week internally for this, but this one is fine too.
> 
> I will get it verfied on SKL tomorrow and get back

Hi Takashi,

Works fine :)

Acked-by: Vinod Koul <vinod.koul@intel.com>
Tested-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>

Thanks
-- 
~Vinod

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Applied "ASoC: intel: Replace kthread with work" to the asoc tree
  2016-11-28 15:32 [PATCH] ASoC: intel: Replace kthread with work Takashi Iwai
  2016-11-28 15:52 ` Vinod Koul
@ 2016-11-30 18:07 ` Mark Brown
  1 sibling, 0 replies; 4+ messages in thread
From: Mark Brown @ 2016-11-30 18:07 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: alsa-devel, Vinod Koul, Liam Girdwood, Pierre-Louis Bossart,
	Mark Brown, Subhransu S. Prusty

The patch

   ASoC: intel: Replace kthread with work

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 786e1c37194e8e822eb72a0aed5fa850e07071a0 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Mon, 28 Nov 2016 16:32:19 +0100
Subject: [PATCH] ASoC: intel: Replace kthread with work

The usage pattern of kthread worker in Intel SST drivers can be
replaced gracefully with the normal workqueue, which is more light-
weight and easier to manage in general.  Let's do it.

While in the replacement, move the schedule_work() call inside the
spinlock for excluding the race, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
Acked-by: Vinod Koul <vinod.koul@intel.com>
Tested-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/baytrail/sst-baytrail-ipc.c |  3 +-
 sound/soc/intel/common/sst-ipc.c            | 58 ++++++++++-------------------
 sound/soc/intel/common/sst-ipc.h            |  4 +-
 sound/soc/intel/haswell/sst-haswell-ipc.c   |  3 +-
 sound/soc/intel/skylake/skl-sst-cldma.c     |  1 -
 sound/soc/intel/skylake/skl-sst-ipc.c       |  2 +-
 sound/soc/intel/skylake/skl-sst-ipc.h       |  1 -
 7 files changed, 24 insertions(+), 48 deletions(-)

diff --git a/sound/soc/intel/baytrail/sst-baytrail-ipc.c b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
index 7ab14ce65a73..260447da32b8 100644
--- a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
+++ b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
@@ -23,7 +23,6 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
-#include <linux/kthread.h>
 #include <linux/firmware.h>
 #include <linux/io.h>
 #include <asm/div64.h>
@@ -338,7 +337,7 @@ static irqreturn_t sst_byt_irq_thread(int irq, void *context)
 	spin_unlock_irqrestore(&sst->spinlock, flags);
 
 	/* continue to send any remaining messages... */
-	kthread_queue_work(&ipc->kworker, &ipc->kwork);
+	schedule_work(&ipc->kwork);
 
 	return IRQ_HANDLED;
 }
diff --git a/sound/soc/intel/common/sst-ipc.c b/sound/soc/intel/common/sst-ipc.c
index 2c1b3159ac1a..62f3a8e0ec87 100644
--- a/sound/soc/intel/common/sst-ipc.c
+++ b/sound/soc/intel/common/sst-ipc.c
@@ -26,7 +26,6 @@
 #include <linux/sched.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
-#include <linux/kthread.h>
 #include <sound/asound.h>
 
 #include "sst-dsp.h"
@@ -109,10 +108,9 @@ static int ipc_tx_message(struct sst_generic_ipc *ipc, u64 header,
 		ipc->ops.tx_data_copy(msg, tx_data, tx_bytes);
 
 	list_add_tail(&msg->list, &ipc->tx_list);
+	schedule_work(&ipc->kwork);
 	spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
 
-	kthread_queue_work(&ipc->kworker, &ipc->kwork);
-
 	if (wait)
 		return tx_wait_done(ipc, msg, rx_data);
 	else
@@ -156,35 +154,32 @@ static int msg_empty_list_init(struct sst_generic_ipc *ipc)
 	return -ENOMEM;
 }
 
-static void ipc_tx_msgs(struct kthread_work *work)
+static void ipc_tx_msgs(struct work_struct *work)
 {
 	struct sst_generic_ipc *ipc =
 		container_of(work, struct sst_generic_ipc, kwork);
 	struct ipc_message *msg;
-	unsigned long flags;
 
-	spin_lock_irqsave(&ipc->dsp->spinlock, flags);
+	spin_lock_irq(&ipc->dsp->spinlock);
 
-	if (list_empty(&ipc->tx_list) || ipc->pending) {
-		spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
-		return;
-	}
-
-	/* if the DSP is busy, we will TX messages after IRQ.
-	 * also postpone if we are in the middle of procesing completion irq*/
-	if (ipc->ops.is_dsp_busy && ipc->ops.is_dsp_busy(ipc->dsp)) {
-		dev_dbg(ipc->dev, "ipc_tx_msgs dsp busy\n");
-		spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
-		return;
-	}
+	while (!list_empty(&ipc->tx_list) && !ipc->pending) {
+		/* if the DSP is busy, we will TX messages after IRQ.
+		 * also postpone if we are in the middle of processing
+		 * completion irq
+		 */
+		if (ipc->ops.is_dsp_busy && ipc->ops.is_dsp_busy(ipc->dsp)) {
+			dev_dbg(ipc->dev, "ipc_tx_msgs dsp busy\n");
+			break;
+		}
 
-	msg = list_first_entry(&ipc->tx_list, struct ipc_message, list);
-	list_move(&msg->list, &ipc->rx_list);
+		msg = list_first_entry(&ipc->tx_list, struct ipc_message, list);
+		list_move(&msg->list, &ipc->rx_list);
 
-	if (ipc->ops.tx_msg != NULL)
-		ipc->ops.tx_msg(ipc, msg);
+		if (ipc->ops.tx_msg != NULL)
+			ipc->ops.tx_msg(ipc, msg);
+	}
 
-	spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
+	spin_unlock_irq(&ipc->dsp->spinlock);
 }
 
 int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, u64 header,
@@ -305,19 +300,7 @@ int sst_ipc_init(struct sst_generic_ipc *ipc)
 	if (ret < 0)
 		return -ENOMEM;
 
-	/* start the IPC message thread */
-	kthread_init_worker(&ipc->kworker);
-	ipc->tx_thread = kthread_run(kthread_worker_fn,
-					&ipc->kworker, "%s",
-					dev_name(ipc->dev));
-	if (IS_ERR(ipc->tx_thread)) {
-		dev_err(ipc->dev, "error: failed to create message TX task\n");
-		ret = PTR_ERR(ipc->tx_thread);
-		kfree(ipc->msg);
-		return ret;
-	}
-
-	kthread_init_work(&ipc->kwork, ipc_tx_msgs);
+	INIT_WORK(&ipc->kwork, ipc_tx_msgs);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(sst_ipc_init);
@@ -326,8 +309,7 @@ void sst_ipc_fini(struct sst_generic_ipc *ipc)
 {
 	int i;
 
-	if (ipc->tx_thread)
-		kthread_stop(ipc->tx_thread);
+	cancel_work_sync(&ipc->kwork);
 
 	if (ipc->msg) {
 		for (i = 0; i < IPC_EMPTY_LIST_SIZE; i++) {
diff --git a/sound/soc/intel/common/sst-ipc.h b/sound/soc/intel/common/sst-ipc.h
index f4aab1b3789a..7ed42a640ad6 100644
--- a/sound/soc/intel/common/sst-ipc.h
+++ b/sound/soc/intel/common/sst-ipc.h
@@ -23,7 +23,6 @@
 #include <linux/list.h>
 #include <linux/workqueue.h>
 #include <linux/sched.h>
-#include <linux/kthread.h>
 
 #define IPC_MAX_MAILBOX_BYTES	256
 
@@ -66,8 +65,7 @@ struct sst_generic_ipc {
 	struct list_head empty_list;
 	wait_queue_head_t wait_txq;
 	struct task_struct *tx_thread;
-	struct kthread_worker kworker;
-	struct kthread_work kwork;
+	struct work_struct kwork;
 	bool pending;
 	struct ipc_message *msg;
 	int tx_data_max_size;
diff --git a/sound/soc/intel/haswell/sst-haswell-ipc.c b/sound/soc/intel/haswell/sst-haswell-ipc.c
index e432a31fd9f2..a3459d1682a6 100644
--- a/sound/soc/intel/haswell/sst-haswell-ipc.c
+++ b/sound/soc/intel/haswell/sst-haswell-ipc.c
@@ -26,7 +26,6 @@
 #include <linux/delay.h>
 #include <linux/sched.h>
 #include <linux/platform_device.h>
-#include <linux/kthread.h>
 #include <linux/firmware.h>
 #include <linux/dma-mapping.h>
 #include <linux/debugfs.h>
@@ -818,7 +817,7 @@ static irqreturn_t hsw_irq_thread(int irq, void *context)
 	spin_unlock_irqrestore(&sst->spinlock, flags);
 
 	/* continue to send any remaining messages... */
-	kthread_queue_work(&ipc->kworker, &ipc->kwork);
+	schedule_work(&ipc->kwork);
 
 	return IRQ_HANDLED;
 }
diff --git a/sound/soc/intel/skylake/skl-sst-cldma.c b/sound/soc/intel/skylake/skl-sst-cldma.c
index efa2532114ba..c9f6d87381db 100644
--- a/sound/soc/intel/skylake/skl-sst-cldma.c
+++ b/sound/soc/intel/skylake/skl-sst-cldma.c
@@ -17,7 +17,6 @@
 
 #include <linux/device.h>
 #include <linux/mm.h>
-#include <linux/kthread.h>
 #include <linux/delay.h>
 #include "../common/sst-dsp.h"
 #include "../common/sst-dsp-priv.h"
diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c
index 594623a4b4cd..e1391dfbc9e9 100644
--- a/sound/soc/intel/skylake/skl-sst-ipc.c
+++ b/sound/soc/intel/skylake/skl-sst-ipc.c
@@ -498,7 +498,7 @@ irqreturn_t skl_dsp_irq_thread_handler(int irq, void *context)
 	skl_ipc_int_enable(dsp);
 
 	/* continue to send any remaining messages... */
-	kthread_queue_work(&ipc->kworker, &ipc->kwork);
+	schedule_work(&ipc->kwork);
 
 	return IRQ_HANDLED;
 }
diff --git a/sound/soc/intel/skylake/skl-sst-ipc.h b/sound/soc/intel/skylake/skl-sst-ipc.h
index 0568f2e8fc57..cc40341233fa 100644
--- a/sound/soc/intel/skylake/skl-sst-ipc.h
+++ b/sound/soc/intel/skylake/skl-sst-ipc.h
@@ -16,7 +16,6 @@
 #ifndef __SKL_IPC_H
 #define __SKL_IPC_H
 
-#include <linux/kthread.h>
 #include <linux/irqreturn.h>
 #include "../common/sst-ipc.h"
 
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-11-30 18:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-28 15:32 [PATCH] ASoC: intel: Replace kthread with work Takashi Iwai
2016-11-28 15:52 ` Vinod Koul
2016-11-29 13:43   ` Vinod Koul
2016-11-30 18:07 ` Applied "ASoC: intel: Replace kthread with work" to the asoc tree Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.