Skip to content

ASoC: SOF: ipc: fix a race, leading to IPC timeouts #943

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
plbossart merged 1 commit into from
May 18, 2019
Merged

ASoC: SOF: ipc: fix a race, leading to IPC timeouts #943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions sound/soc/sof/intel/bdw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,11 +278,15 @@ static irqreturn_t bdw_irq_thread(int irq, void *context)
/* reply message from DSP */
if (ipcx & SHIM_IPCX_DONE &&
!(imrx & SHIM_IMRX_DONE)) {
unsigned long flags;

/* Mask Done interrupt before return */
snd_sof_dsp_update_bits_unlocked(sdev, BDW_DSP_BAR,
SHIM_IMRX, SHIM_IMRX_DONE,
SHIM_IMRX_DONE);

spin_lock_irqsave(&sdev->ipc_lock, flags);

/*
* handle immediate reply from DSP core. If the msg is
* found, set done bit in cmd_done which is called at the
Expand All @@ -294,6 +298,8 @@ static irqreturn_t bdw_irq_thread(int irq, void *context)
snd_sof_ipc_reply(sdev, ipcx);

bdw_dsp_done(sdev);

spin_unlock_irqrestore(&sdev->ipc_lock, flags);
}

ipcd = snd_sof_dsp_read(sdev, BDW_DSP_BAR, SHIM_IPCD);
Expand Down Expand Up @@ -485,7 +491,6 @@ static void bdw_get_reply(struct snd_sof_dev *sdev)
{
struct snd_sof_ipc_msg *msg = sdev->msg;
struct sof_ipc_reply reply;
unsigned long flags;
int ret = 0;

/*
Expand All @@ -501,8 +506,6 @@ static void bdw_get_reply(struct snd_sof_dev *sdev)
/* get reply */
sof_mailbox_read(sdev, sdev->host_box.offset, &reply, sizeof(reply));

spin_lock_irqsave(&sdev->ipc_lock, flags);

if (reply.error < 0) {
memcpy(msg->reply_data, &reply, sizeof(reply));
ret = reply.error;
Expand All @@ -521,8 +524,6 @@ static void bdw_get_reply(struct snd_sof_dev *sdev)
}

msg->reply_error = ret;

spin_unlock_irqrestore(&sdev->ipc_lock, flags);
}

static void bdw_host_done(struct snd_sof_dev *sdev)
Expand Down
12 changes: 7 additions & 5 deletions sound/soc/sof/intel/byt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,11 +324,16 @@ static irqreturn_t byt_irq_thread(int irq, void *context)
/* reply message from DSP */
if (ipcx & SHIM_BYT_IPCX_DONE &&
!(imrx & SHIM_IMRX_DONE)) {
unsigned long flags;

/* Mask Done interrupt before first */
snd_sof_dsp_update_bits64_unlocked(sdev, BYT_DSP_BAR,
SHIM_IMRX,
SHIM_IMRX_DONE,
SHIM_IMRX_DONE);

spin_lock_irqsave(&sdev->ipc_lock, flags);

/*
* handle immediate reply from DSP core. If the msg is
* found, set done bit in cmd_done which is called at the
Expand All @@ -340,6 +345,8 @@ static irqreturn_t byt_irq_thread(int irq, void *context)
snd_sof_ipc_reply(sdev, ipcx);

byt_dsp_done(sdev);

spin_unlock_irqrestore(&sdev->ipc_lock, flags);
}

/* new message from DSP */
Expand Down Expand Up @@ -383,7 +390,6 @@ static void byt_get_reply(struct snd_sof_dev *sdev)
{
struct snd_sof_ipc_msg *msg = sdev->msg;
struct sof_ipc_reply reply;
unsigned long flags;
int ret = 0;

/*
Expand All @@ -399,8 +405,6 @@ static void byt_get_reply(struct snd_sof_dev *sdev)
/* get reply */
sof_mailbox_read(sdev, sdev->host_box.offset, &reply, sizeof(reply));

spin_lock_irqsave(&sdev->ipc_lock, flags);

if (reply.error < 0) {
memcpy(msg->reply_data, &reply, sizeof(reply));
ret = reply.error;
Expand All @@ -419,8 +423,6 @@ static void byt_get_reply(struct snd_sof_dev *sdev)
}

msg->reply_error = ret;

spin_unlock_irqrestore(&sdev->ipc_lock, flags);
}

static void byt_host_done(struct snd_sof_dev *sdev)
Expand Down
6 changes: 6 additions & 0 deletions sound/soc/sof/intel/cnl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ static irqreturn_t cnl_ipc_irq_thread(int irq, void *context)
/* reply message from DSP */
if (hipcida & CNL_DSP_REG_HIPCIDA_DONE &&
hipcctl & CNL_DSP_REG_HIPCCTL_DONE) {
unsigned long flags;

hipci = snd_sof_dsp_read(sdev, HDA_DSP_BAR,
CNL_DSP_REG_HIPCIDR);
msg_ext = hipci & CNL_DSP_REG_HIPCIDR_MSG_MASK;
Expand All @@ -64,6 +66,8 @@ static irqreturn_t cnl_ipc_irq_thread(int irq, void *context)
CNL_DSP_REG_HIPCCTL,
CNL_DSP_REG_HIPCCTL_DONE, 0);

spin_lock_irqsave(&sdev->ipc_lock, flags);

/* handle immediate reply from DSP core */
hda_dsp_ipc_get_reply(sdev);
snd_sof_ipc_reply(sdev, msg);
Expand All @@ -75,6 +79,8 @@ static irqreturn_t cnl_ipc_irq_thread(int irq, void *context)

cnl_ipc_dsp_done(sdev);

spin_unlock_irqrestore(&sdev->ipc_lock, flags);

ret = IRQ_HANDLED;
}

Expand Down
19 changes: 16 additions & 3 deletions sound/soc/sof/intel/hda-ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ void hda_dsp_ipc_get_reply(struct snd_sof_dev *sdev)
struct snd_sof_ipc_msg *msg = sdev->msg;
struct sof_ipc_reply reply;
struct sof_ipc_cmd_hdr *hdr;
unsigned long flags;
int ret = 0;

/*
Expand All @@ -84,7 +83,6 @@ void hda_dsp_ipc_get_reply(struct snd_sof_dev *sdev)
dev_warn(sdev->dev, "unexpected ipc interrupt raised!\n");
return;
}
spin_lock_irqsave(&sdev->ipc_lock, flags);

hdr = msg->msg_data;
if (hdr->cmd == (SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CTX_SAVE)) {
Expand Down Expand Up @@ -123,7 +121,6 @@ void hda_dsp_ipc_get_reply(struct snd_sof_dev *sdev)
out:
msg->reply_error = ret;

spin_unlock_irqrestore(&sdev->ipc_lock, flags);
}

static bool hda_dsp_ipc_is_sof(uint32_t msg)
Expand Down Expand Up @@ -158,6 +155,8 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
/* is this a reply message from the DSP */
if (hipcie & HDA_DSP_REG_HIPCIE_DONE &&
hipcctl & HDA_DSP_REG_HIPCCTL_DONE) {
unsigned long flags;

hipci = snd_sof_dsp_read(sdev, HDA_DSP_BAR,
HDA_DSP_REG_HIPCI);
msg = hipci & HDA_DSP_REG_HIPCI_MSG_MASK;
Expand All @@ -172,6 +171,18 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
HDA_DSP_REG_HIPCCTL,
HDA_DSP_REG_HIPCCTL_DONE, 0);

/*
* Make sure the interrupt thread cannot be preempted between
* waking up the sender and re-enabling the interrupt. Also
* protect against a theoretical race with sof_ipc_tx_message():
* if the DSP is fast enough to receive an IPC message, reply to
* it, and the host interrupt processing calls this function on
* a different core from the one, where the sending is taking
* place, the message might not yet be marked as expecting a
* reply.
*/
spin_lock_irqsave(&sdev->ipc_lock, flags);

/* handle immediate reply from DSP core - ignore ROM messages */
if (hda_dsp_ipc_is_sof(msg)) {
hda_dsp_ipc_get_reply(sdev);
Expand All @@ -187,6 +198,8 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
/* set the done bit */
hda_dsp_ipc_dsp_done(sdev);

spin_unlock_irqrestore(&sdev->ipc_lock, flags);

ret = IRQ_HANDLED;
}

Expand Down
13 changes: 0 additions & 13 deletions sound/soc/sof/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,19 +308,8 @@ EXPORT_SYMBOL(sof_ipc_tx_message);
int snd_sof_ipc_reply(struct snd_sof_dev *sdev, u32 msg_id)
{
struct snd_sof_ipc_msg *msg = &sdev->ipc->msg;
unsigned long flags;

/*
* Protect against a theoretical race with sof_ipc_tx_message(): if the
* DSP is fast enough to receive an IPC message, reply to it, and the
* host interrupt processing calls this function on a different core
* from the one, where the sending is taking place, the message might
* not yet be marked as expecting a reply.
*/
spin_lock_irqsave(&sdev->ipc_lock, flags);

if (msg->ipc_complete) {
spin_unlock_irqrestore(&sdev->ipc_lock, flags);
dev_err(sdev->dev, "error: no reply expected, received 0x%x",
msg_id);
return -EINVAL;
Expand All @@ -330,8 +319,6 @@ int snd_sof_ipc_reply(struct snd_sof_dev *sdev, u32 msg_id)
msg->ipc_complete = true;
wake_up(&msg->waitq);

spin_unlock_irqrestore(&sdev->ipc_lock, flags);

return 0;
}
EXPORT_SYMBOL(snd_sof_ipc_reply);
Expand Down