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@xxxxxxx>
> ---
>
> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
