Re: [alsa-devel] [PATCH] ASoC: intel: Replace kthread with work

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