linux-kernel - Re: [PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2dfeee7c-32bd-c054-22ff-3a2266e62c90@amd.com>
Date:   Tue, 23 May 2023 13:06:59 +0530
From:   "Mukunda,Vijendar" <vijendar.mukunda@....com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        broonie@...nel.org
Cc:     alsa-devel@...a-project.org, Basavaraj.Hiregoudar@....com,
        Sunil-kumar.Dommati@....com, Mastan.Katragadda@....com,
        Arungopal.kondaveeti@....com, mario.limonciello@....com,
        Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Syed Saba Kareem <Syed.SabaKareem@....com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA
 interrupts

On 22/05/23 23:42, Pierre-Louis Bossart wrote:
>
> On 5/22/23 08:31, Vijendar Mukunda wrote:
>> Initialize workqueue for SoundWire DMA interrupts handling.
>> Whenever audio data equal to the SoundWire FIFO watermark level
>> are produced/consumed, interrupt is generated.
>> Acknowledge the interrupt and schedule the workqueue.
> It would help to explain why a work queue is needed is the first place,
> as opposed to handling periods in the interrupt thread.
For SoundWire DAI link, we are setting nonatomic flag to true.
If we return period elapsed from hard irq handler instead of workqueue,
soft lock up is observed during stream closure.

We can use interrupt thread as well. To have a symmetry with
SoundWire manager work queues, we have used workqueue for
DMA interrupts.

>> +static void acp63_sdw_dma_workthread(struct work_struct *work)
>> +{
>> +	struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data,
>> +						    acp_sdw_dma_work);
>> +	struct sdw_dma_dev_data *sdw_dma_data;
>> +	u32 stream_index;
>> +	u16 pdev_index;
>> +
>> +	pdev_index = adata->sdw_dma_dev_index;
>> +	sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>> +
>> +	for (stream_index = 0; stream_index < ACP63_SDW0_DMA_MAX_STREAMS; stream_index++) {
>> +		if (adata->sdw0_dma_intr_stat[stream_index]) {
>> +			if (sdw_dma_data->sdw0_dma_stream[stream_index])
>> +				snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]);
>> +			adata->sdw0_dma_intr_stat[stream_index] = 0;
>> +		}
>> +	}
>> +	for (stream_index = 0; stream_index < ACP63_SDW1_DMA_MAX_STREAMS; stream_index++) {
>> +		if (adata->sdw1_dma_intr_stat[stream_index]) {
>> +			if (sdw_dma_data->sdw1_dma_stream[stream_index])
>> +				snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]);
>> +			adata->sdw1_dma_intr_stat[stream_index] = 0;
>> +		}
>> +	}
> I am not clear on the benefits of the workqueue which only tests a flag
> that's set ...
In top half, we are checking all stream irq mask and setting
corresponding stream id index in interrupt status array when dma
irq is raised.

Our intention is to handle snd_pcm_period_elapsed in process context.
if the flag is set, call the period elapsed for the substream based on stream
id in work queue.
>
>> +}
>> +
>>  static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>>  {
>>  	struct acp63_dev_data *adata;
>>  	struct pdm_dev_data *ps_pdm_data;
>>  	struct amd_sdw_manager *amd_manager;
>>  	u32 ext_intr_stat, ext_intr_stat1;
>> +	u32 stream_id = 0;
>>  	u16 irq_flag = 0;
>> +	u16 sdw_dma_irq_flag = 0;
>>  	u16 pdev_index;
>> +	u16 index;
>>  
>>  	adata = dev_id;
>>  	if (!adata)
>> @@ -148,7 +178,57 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>>  			snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
>>  		irq_flag = 1;
>>  	}
>> -	if (irq_flag)
>> +	if (ext_intr_stat & ACP_SDW_DMA_IRQ_MASK) {
>> +		for (index = ACP_AUDIO2_RX_THRESHOLD; index <= ACP_AUDIO0_TX_THRESHOLD; index++) {
>> +			if (ext_intr_stat & BIT(index)) {
>> +				writel(BIT(index), adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>> +				switch (index) {
>> +				case ACP_AUDIO0_TX_THRESHOLD:
>> +					stream_id = ACP_SDW0_AUDIO0_TX;
>> +					break;
>> +				case ACP_AUDIO1_TX_THRESHOLD:
>> +					stream_id = ACP_SDW0_AUDIO1_TX;
>> +					break;
>> +				case ACP_AUDIO2_TX_THRESHOLD:
>> +					stream_id = ACP_SDW0_AUDIO2_TX;
>> +					break;
>> +				case ACP_AUDIO0_RX_THRESHOLD:
>> +					stream_id = ACP_SDW0_AUDIO0_RX;
>> +					break;
>> +				case ACP_AUDIO1_RX_THRESHOLD:
>> +					stream_id = ACP_SDW0_AUDIO1_RX;
>> +					break;
>> +				case ACP_AUDIO2_RX_THRESHOLD:
>> +					stream_id = ACP_SDW0_AUDIO2_RX;
>> +					break;
>> +				}
>> +
>> +				adata->sdw0_dma_intr_stat[stream_id] = 1;
> .. here ...
Please refer above comment.
>> +				sdw_dma_irq_flag = 1;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* SDW1 BT RX */
>> +	if (ext_intr_stat1 & ACP_P1_AUDIO1_RX_THRESHOLD) {
>> +		writel(ACP_P1_AUDIO1_RX_THRESHOLD,
>> +		       adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
>> +		adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_RX] = 1;
> ... and here ...
>
>> +		sdw_dma_irq_flag = 1;
>> +	}
>> +
>> +	/* SDW1 BT TX*/
>> +	if (ext_intr_stat1 & ACP_P1_AUDIO1_TX_THRESHOLD) {
>> +		writel(ACP_P1_AUDIO1_TX_THRESHOLD,
>> +		       adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
>> +		adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_TX] = 1;
> ... or here ...
>
>> +		sdw_dma_irq_flag = 1;
>> +	}
>> +
>> +	if (sdw_dma_irq_flag)
>> +		schedule_work(&adata->acp_sdw_dma_work);
>> +
>> +	if (irq_flag || sdw_dma_irq_flag)
>>  		return IRQ_HANDLED;
>>  	else
>>  		return IRQ_NONE;