IPC fixes -- interrupt handler by keyonjie · Pull Request #850 · thesofproject/linux

keyonjie · 2019-04-19T17:51:35Z

We are using share irq for IPC and stream IOC interrupts.

This is a harden series to our IPC and stream interrupt handler, to make sure they won't take wrong ones and interfere with each other.

This fix might benefit to common IPC issues, e.g. thesofproject/sof#1254

plbossart

@keyonjie you need to explain the sequence by which the irq_status gets overwritten, it's completely unclear from your code+commit message. Also a number of cosmetic issues and commit message wording.

plbossart · 2019-04-19T20:21:34Z

Allow me to challenge this patch. The only time where sdev->irq_status is used is in hda_dsp_ipc_irq_thread(), where we do this:

/* here we handle IPC interrupts only */ if (!(sdev->irq_status & HDA_DSP_ADSPIS_IPC)) return IRQ_NONE;

So what does this fix improve?

@plbossart I agree. I think both the sdev->irq_status saving and the check again in hda_dsp_ipc_irq_thread() are unnecessary.

@plbossart
Let us think about a situation here:
Let IRQ A to be the IPC IRQ we need to handle and wake a thread.
Let IRQ B some IOC IRQ from HDA.

IRQ A comes, in the handle sdev->irq_status is set to be some useful value and we wake up the thread.

Before we go to the check bellow

/* here we handle IPC interrupts only */ if (!(sdev->irq_status & HDA_DSP_ADSPIS_IPC)) return IRQ_NONE;

IRQ B comes and reset the sdev->irq_status and do nothing

What will happen to the thread that already wake up by IRQ A?

It will just return IRQ_NONE;
And we will lost the chance to handle the IPC with IRQ A.

@ranj063
You mean we did not check the status again in thread if we return the right value in handler?
That will only work if the handler will only wake the thread bind to itself. I am not sure if the code will do like that. If so I agree with you change to leave the check in one place.

linux/sound/soc/sof/intel/hda.c

Lines 509 to 522 in 9086043

dev_dbg(sdev->dev, "using HDA IRQ %d\n", hdev->irq);

ret = request_threaded_irq(hdev->irq, hda_dsp_stream_interrupt,

hda_dsp_stream_threaded_handler,

IRQF_SHARED, "AudioHDA", bus);

if (ret < 0) {

dev_err(sdev->dev, "error: failed to register HDA IRQ %d\n",

hdev->irq);

goto free_irq_vector;

}

dev_dbg(sdev->dev, "using IPC IRQ %d\n", sdev->ipc_irq);

ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_ipc_irq_handler,

sof_ops(sdev)->irq_thread, IRQF_SHARED,

"AudioDSP", sdev);

plbossart · 2019-04-19T20:35:14Z

clarify this is the maximum total playback + capture streams supported by the hardware.
This is NOT the current total number of streams in use.

correct, maybe something like "num_of_streams_supported"?

plbossart · 2019-04-19T20:37:50Z

store total stream count (playback + capture) from GCAP

xiulipan

A good catch, hope I have make a right explanation to what you mean @keyonjie

Maybe we should figure out what will be request_threaded_irq do with the handler and thread that bind together.
If handler wakes only the thread bind to itself. No more check in thread is needed. Be careful in handler is enough.

xiulipan · 2019-04-19T23:20:46Z

@plbossart
Let us think about a situation here:
Let IRQ A to be the IPC IRQ we need to handle and wake a thread.
Let IRQ B some IOC IRQ from HDA.

IRQ A comes, in the handle sdev->irq_status is set to be some useful value and we wake up the thread.

Before we go to the check bellow

/* here we handle IPC interrupts only */ if (!(sdev->irq_status & HDA_DSP_ADSPIS_IPC)) return IRQ_NONE;

IRQ B comes and reset the sdev->irq_status and do nothing

What will happen to the thread that already wake up by IRQ A?

It will just return IRQ_NONE;
And we will lost the chance to handle the IPC with IRQ A.

@ranj063
You mean we did not check the status again in thread if we return the right value in handler?
That will only work if the handler will only wake the thread bind to itself. I am not sure if the code will do like that. If so I agree with you change to leave the check in one place.

linux/sound/soc/sof/intel/hda.c

Lines 509 to 522 in 9086043

dev_dbg(sdev->dev, "using HDA IRQ %d\n", hdev->irq);

ret = request_threaded_irq(hdev->irq, hda_dsp_stream_interrupt,

hda_dsp_stream_threaded_handler,

IRQF_SHARED, "AudioHDA", bus);

if (ret < 0) {

dev_err(sdev->dev, "error: failed to register HDA IRQ %d\n",

hdev->irq);

goto free_irq_vector;

}

dev_dbg(sdev->dev, "using IPC IRQ %d\n", sdev->ipc_irq);

ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_ipc_irq_handler,

sof_ops(sdev)->irq_thread, IRQF_SHARED,

"AudioDSP", sdev);

keqiaozhang · 2019-04-22T06:00:57Z

I have verified #849 + #850 on GLK and WHL-I2S, Simultaneous playback/capturing can pass 1K iterations without errors on both platforms.

plbossart · 2019-04-22T15:07:39Z

no updated code -> PR will not be merged until mid-week...

keyonjie · 2019-04-23T01:27:19Z

@plbossart updated.

plbossart

Please explain the first commit message in more details, what you wrote is not incorrect but doesn't capture the discussions in this PR.

keyonjie · 2019-04-23T06:52:25Z

hi @ranj063 @plbossart Thanks for reviewing, updated now.

plbossart

You made a change that just invalidates everything we've talked about. If you remove the check for the irq_status, then there is no possible race condition and the irq_status is useless.
I can't merge stuff that makes no sense, sorry.

keyonjie · 2019-04-24T01:09:46Z

@plbossart @ranj063 updated, irq_status is removed from sdev, and commit message updated alongside.

plbossart

Confused on the first commit and open on the 3rd one, see below.
Thanks for the quick feedback, let's try and close this

plbossart · 2019-04-24T01:17:28Z

does it makes sense to place the log here, knowing that you read the same value below inside of a spin_lock?
Either remove this log or do this right.

@plbossart I'd like to keep this here, as we need debug the case return before the spin_lock below sometimes (returned at pm_runtime_active failed).

then add a comment that this is explicit and not a mistake

keyonjie · 2019-04-24T03:03:36Z

@plbossart @ranj063 can you help check if this commit message fine from your points:

    ASoC: SOF: Intel: hda-ipc: remove irq_status from sdev and fix missing
    handling to IPC IRQ in the thread
    
    We are using a shared IRQ for IPC and stream IOC interrupts. It is
    possible that the IPC interrupt handler could be run (before it is
    handled by stream interrupt handler) when a stream interrupt arrives,
    even while the IPC interrupt thread dealing with the current IPC
    interrupt hasn't been scheduled yet.
    
    In this case, the sdev->irq_status can be garbled to be 0 and then the
    later check (to sdev->irq_status) in ipc_irq_thread will fail and
    return, this IPC is dropped and IPC time out happens.
    
    To fix that, We need to check the interrupt source before waking up the
    handler thread to make sure we handle IPC interrupt here only.
    
    On the other hand, once we are in the thread, checking sdev->irq_status
    is superfluous, so here remove irq_status from definition of struct
    snd_sof_dev as nobody need it anymore.

ranj063 · 2019-04-24T03:40:39Z

@keyonjie the commit message is too complicated. I'd simplify it like this:

When using a shared IRQ between IPC interrupt and stream IOC interrupt, the interrupt handlers need to check the interrupt source before scheduling their respective IRQ threads. In the case of IPC handler, it should check if it is an IPC interrupt and wake up the the IPC IRQ thread.

The IPC IRQ thread, once scheduled, does not need to check the IRQ source again. So, remove the superfluous check in the thread and remove the irq_status field from snd_sof_dev struct as it is no longer needed.

Please feel free to modify it.

keyonjie · 2019-04-25T00:41:55Z

@keyonjie the commit message is too complicated. I'd simplify it like this:

When using a shared IRQ between IPC interrupt and stream IOC interrupt, the interrupt handlers need to check the interrupt source before scheduling their respective IRQ threads. In the case of IPC handler, it should check if it is an IPC interrupt and wake up the the IPC IRQ thread.

The IPC IRQ thread, once scheduled, does not need to check the IRQ source again. So, remove the superfluous check in the thread and remove the irq_status field from snd_sof_dev struct as it is no longer needed.

Please feel free to modify it.

Thanks, let me take it.

keyonjie · 2019-04-25T00:48:13Z

updated, @ranj063 @plbossart please help review.

plbossart

ok, we're almost good. please fix the first commit title Commit 5d4f2f69badd ("ASoC: SOF: Intel: hda-ipc: remove irq_status from sdev and fix missing handling to IPC IRQ in the thread") is really too long, and add a comment for the dev_vdbg. Thanks!

plbossart · 2019-04-25T00:54:59Z

then add a comment that this is explicit and not a mistake

keyonjie · 2019-04-25T01:10:01Z

ok, we're almost good. please fix the first commit title Commit 5d4f2f6 ("ASoC: SOF: Intel: hda-ipc: remove irq_status from sdev and fix missing handling to IPC IRQ in the thread") is really too long, and add a comment for the dev_vdbg. Thanks!

Thanks, both addressed and updated, please help review.

bardliao

Thanks @keyonjie It looks good to me.

xiulipan · 2019-04-25T05:13:15Z

@keyonjie
LGTM, I think this the right way for irq_handler and irq_thread.

ranj063 · 2019-04-25T15:47:25Z

@keyonjie do we need to intialize this to 0? we had to remove a lot of these lately. so we should be carefulof adding these in the future too.

Good catch, this init is not needed?

not needed, let me remove it.

plbossart

one more pass needed, thanks @ranj063 for the review.

plbossart · 2019-04-25T21:25:59Z

Good catch, this init is not needed?

plbossart · 2019-04-25T21:29:37Z

The explanation isn't very useful...
"Intentionally add verbose log before pm_runtime and spin lock"

I don't want to add comments with "hardcoding" consequent invoking, which means we need maintain/update it once those invoking changed, maybe remove this verbose debugging can make everybody happy.

When using a shared IRQ between IPC interrupt and stream IOC interrupt, the interrupt handlers need to check the interrupt source before scheduling their respective IRQ threads. In the case of IPC handler, it should check if it is an IPC interrupt before waking up the IPC IRQ thread. The IPC IRQ thread, once scheduled, does not need to check the IRQ source again. So, remove the superfluous check in the thread. Remove the irq_status field from snd_sof_dev struct also as it is no longer needed. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Add stream_max into struct sof_intel_hda_dev to store the total hda stream number that the platform can support, and initialize it at stream_init. This can be used later e.g. for stream bitmask. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie · 2019-04-26T04:58:13Z

@plbossart @ranj063 updated.

The stream and IPC share the same interrupt. The stream interrupt handler mistakenly uses the ipc interrupt and return IRQ_HANDLED, causing the ipc interrupt to be missed. Make sure the stream interrupt handler only deals with stream-related interrupts. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

plbossart

Thanks @keyonjie

keyonjie requested review from RanderWang, lgirdwood, lyakh, plbossart and ranj063 April 19, 2019 17:51

keyonjie mentioned this pull request Apr 19, 2019

SKL+ ipc timeout fix series #845

Closed

plbossart requested changes Apr 19, 2019

View reviewed changes

xiulipan suggested changes Apr 19, 2019

View reviewed changes

keyonjie mentioned this pull request Apr 20, 2019

[CML] Random IPC time out issues thesofproject/sof#1254

Closed

keqiaozhang mentioned this pull request Apr 22, 2019

IPC fixes -- for snd_pcm_period_elapse() invoking #849

Merged

keyonjie force-pushed the ipc_handler_fix branch from 9fb8432 to 009f7a5 Compare April 23, 2019 01:27

plbossart requested changes Apr 23, 2019

View reviewed changes

Comment thread sound/soc/sof/intel/hda-ipc.c Outdated

keyonjie force-pushed the ipc_handler_fix branch from 009f7a5 to 4dbfc02 Compare April 23, 2019 06:51

plbossart requested changes Apr 23, 2019

View reviewed changes

Comment thread sound/soc/sof/intel/hda-ipc.c Outdated

keyonjie force-pushed the ipc_handler_fix branch from 4dbfc02 to 51b1523 Compare April 24, 2019 01:07

plbossart requested changes Apr 24, 2019

View reviewed changes

keyonjie force-pushed the ipc_handler_fix branch from 51b1523 to 7644f4a Compare April 25, 2019 00:47

keyonjie requested review from plbossart and xiulipan April 25, 2019 00:47

keyonjie requested a review from bardliao April 25, 2019 00:55

plbossart reviewed Apr 25, 2019

View reviewed changes

Comment thread sound/soc/sof/intel/hda-stream.c Outdated

plbossart Apr 25, 2019

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then add a comment that this is explicit and not a mistake

keyonjie force-pushed the ipc_handler_fix branch from 7644f4a to 70f9f01 Compare April 25, 2019 01:09

keyonjie requested a review from plbossart April 25, 2019 01:10

bardliao previously approved these changes Apr 25, 2019

View reviewed changes

xiulipan previously approved these changes Apr 25, 2019

View reviewed changes

ranj063 reviewed Apr 25, 2019

View reviewed changes

plbossart requested changes Apr 25, 2019

View reviewed changes

keyonjie added 2 commits April 26, 2019 12:49

keyonjie dismissed stale reviews from xiulipan and bardliao via d9f2c03 April 26, 2019 04:56

keyonjie force-pushed the ipc_handler_fix branch from 70f9f01 to d9f2c03 Compare April 26, 2019 04:56

keyonjie requested a review from plbossart April 26, 2019 04:57

ranj063 approved these changes Apr 26, 2019

View reviewed changes

keyonjie requested a review from ranj063 April 26, 2019 14:16

ranj063 approved these changes Apr 26, 2019

View reviewed changes

plbossart approved these changes Apr 26, 2019

View reviewed changes

plbossart merged commit af71673 into thesofproject:topic/sof-dev Apr 26, 2019

	dev_dbg(sdev->dev, "using HDA IRQ %d\n", hdev->irq);
	ret = request_threaded_irq(hdev->irq, hda_dsp_stream_interrupt,
	hda_dsp_stream_threaded_handler,
	IRQF_SHARED, "AudioHDA", bus);
	if (ret < 0) {
	dev_err(sdev->dev, "error: failed to register HDA IRQ %d\n",
	hdev->irq);
	goto free_irq_vector;
	}

	dev_dbg(sdev->dev, "using IPC IRQ %d\n", sdev->ipc_irq);
	ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_ipc_irq_handler,
	sof_ops(sdev)->irq_thread, IRQF_SHARED,
	"AudioDSP", sdev);

Conversation

keyonjie commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie Apr 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiulipan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keqiaozhang commented Apr 22, 2019

Uh oh!

plbossart commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyonjie commented Apr 23, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keyonjie commented Apr 23, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keyonjie commented Apr 24, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Apr 24, 2019

Uh oh!

ranj063 commented Apr 24, 2019

Uh oh!

keyonjie commented Apr 25, 2019

Uh oh!

keyonjie commented Apr 25, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Apr 25, 2019

Uh oh!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

xiulipan commented Apr 25, 2019

Uh oh!

ranj063 Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

keyonjie commented Apr 19, 2019 •

edited

Loading

keyonjie Apr 20, 2019 •

edited

Loading

plbossart commented Apr 22, 2019 •

edited

Loading

keyonjie Apr 24, 2019 •

edited

Loading

ranj063 Apr 25, 2019 •

edited

Loading

keyonjie Apr 26, 2019 •

edited

Loading