[alsa-devel] [RFC: PATCH] ALSA: hda - clear IRQ statu twice on some Intel platforms

[Yang, Libin]

Hi Takashi,

Our QA has tested your patch. It works and the basic test cases passed.

Regards,
Libin


> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Tuesday, February 23, 2016 11:04 PM
> To: Yang, Libin
> Cc: Lin, Mengdong; libin.yang at linux.intel.com; alsa-devel at alsa-
> project.org
> Subject: Re: [alsa-devel] [RFC: PATCH] ALSA: hda - clear IRQ statu twice on
> some Intel platforms
> 
> On Tue, 23 Feb 2016 15:27:50 +0100,
> Yang, Libin wrote:
> >
> > > -----Original Message-----
> > > From: alsa-devel-bounces at alsa-project.org [mailto:alsa-devel-
> > > bounces at alsa-project.org] On Behalf Of Takashi Iwai
> > > Sent: Tuesday, February 23, 2016 5:28 PM
> > > To: Yang, Libin
> > > Cc: Lin, Mengdong; libin.yang at linux.intel.com; alsa-devel at alsa-
> > > project.org
> > > Subject: Re: [alsa-devel] [RFC: PATCH] ALSA: hda - clear IRQ statu
> twice on
> > > some Intel platforms
> > >
> > > On Tue, 23 Feb 2016 09:57:13 +0100,
> > > Yang, Libin wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > > I'm testing your patch in stress level. So far, it seems to works
> smoothly.
> > > >
> > > > Which patch do you like? And your patch seems to impact on all the
> > > platforms?
> > >
> > > One potential problem in your patch is that the later triggered event
> > > shall be ignored by the post-irq ack.
> >
> > Yes. So it seems your patch is better.
> >
> > >
> > > OTOH, my patch would impact to all platforms, yes.  However, in
> > > theory, all platforms may have this issue, so it's anyway good in one
> > > side.  In another flip, of course, it might introduce some regression.
> > >
> > > We can limit this loop check only for some known platforms, or we let
> > > it tested more :)
> > >
> > > I'm going to cook the patch a bit more and give you for testing.
> >
> > I'd like to test your updated patch.
> 
> OK, below is the revised patch.  Let me know if this works.
> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai at suse.de>
> Subject: [PATCH] ALSA: hda - Loop interrupt handling until really cleared
> 
> Currently the interrupt handler of HD-audio driver assumes that no irq
> update is needed while processing the irq.  But in reality, it has
> been confirmed that the HW irq is issued even during the irq
> handling.  Since we clear the irq status at the beginning, process the
> interrupt, then exits from the handler, the lately issued interrupt is
> left untouched without being properly processed.
> 
> This patch changes the interrupt handler code to loop over the
> check-and-process.  The handler tries repeatedly as long as the IRQ
> status are turned on, and either stream or CORB/RIRB is handled.
> 
> For checking the stream handling, snd_hdac_bus_handle_stream_irq()
> returns a value indicating the stream indices bits.  Other than that,
> the change is only in the irq handler itself.
> 
> Reported-by: Libin Yang <libin.yang at linux.intel.com>
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
>  include/sound/hdaudio.h        |  2 +-
>  sound/hda/hdac_controller.c    |  7 ++++++-
>  sound/pci/hda/hda_controller.c | 47 +++++++++++++++++++++++-------
> ------------
>  3 files changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index e2b712c90d3f..c21c38ce7450 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -343,7 +343,7 @@ void snd_hdac_bus_enter_link_reset(struct
> hdac_bus *bus);
>  void snd_hdac_bus_exit_link_reset(struct hdac_bus *bus);
> 
>  void snd_hdac_bus_update_rirb(struct hdac_bus *bus);
> -void snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned
> int status,
> +int snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned
> int status,
>  				    void (*ack)(struct hdac_bus *,
>  						struct hdac_stream *));
> 
> diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
> index b5a17cb510a0..8c486235c905 100644
> --- a/sound/hda/hdac_controller.c
> +++ b/sound/hda/hdac_controller.c
> @@ -426,18 +426,22 @@
> EXPORT_SYMBOL_GPL(snd_hdac_bus_stop_chip);
>   * @bus: HD-audio core bus
>   * @status: INTSTS register value
>   * @ask: callback to be called for woken streams
> + *
> + * Returns the bits of handled streams, or zero if no stream is handled.
>   */
> -void snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned
> int status,
> +int snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned
> int status,
>  				    void (*ack)(struct hdac_bus *,
>  						struct hdac_stream *))
>  {
>  	struct hdac_stream *azx_dev;
>  	u8 sd_status;
> +	int handled = 0;
> 
>  	list_for_each_entry(azx_dev, &bus->stream_list, list) {
>  		if (status & azx_dev->sd_int_sta_mask) {
>  			sd_status = snd_hdac_stream_readb(azx_dev,
> SD_STS);
>  			snd_hdac_stream_writeb(azx_dev, SD_STS,
> SD_INT_MASK);
> +			handled |= 1 << azx_dev->index;
>  			if (!azx_dev->substream || !azx_dev->running
> ||
>  			    !(sd_status & SD_INT_COMPLETE))
>  				continue;
> @@ -445,6 +449,7 @@ void snd_hdac_bus_handle_stream_irq(struct
> hdac_bus *bus, unsigned int status,
>  				ack(bus, azx_dev);
>  		}
>  	}
> +	return handled;
>  }
>  EXPORT_SYMBOL_GPL(snd_hdac_bus_handle_stream_irq);
> 
> diff --git a/sound/pci/hda/hda_controller.c
> b/sound/pci/hda/hda_controller.c
> index 37cf9cee9835..27de8015717d 100644
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -930,6 +930,8 @@ irqreturn_t azx_interrupt(int irq, void *dev_id)
>  	struct azx *chip = dev_id;
>  	struct hdac_bus *bus = azx_bus(chip);
>  	u32 status;
> +	bool active, handled = false;
> +	int repeat = 0; /* count for avoiding endless loop */
> 
>  #ifdef CONFIG_PM
>  	if (azx_has_pm_runtime(chip))
> @@ -939,33 +941,36 @@ irqreturn_t azx_interrupt(int irq, void *dev_id)
> 
>  	spin_lock(&bus->reg_lock);
> 
> -	if (chip->disabled) {
> -		spin_unlock(&bus->reg_lock);
> -		return IRQ_NONE;
> -	}
> -
> -	status = azx_readl(chip, INTSTS);
> -	if (status == 0 || status == 0xffffffff) {
> -		spin_unlock(&bus->reg_lock);
> -		return IRQ_NONE;
> -	}
> +	if (chip->disabled)
> +		goto unlock;
> 
> -	snd_hdac_bus_handle_stream_irq(bus, status, stream_update);
> +	do {
> +		status = azx_readl(chip, INTSTS);
> +		if (status == 0 || status == 0xffffffff)
> +			break;
> 
> -	/* clear rirb int */
> -	status = azx_readb(chip, RIRBSTS);
> -	if (status & RIRB_INT_MASK) {
> -		if (status & RIRB_INT_RESPONSE) {
> -			if (chip->driver_caps &
> AZX_DCAPS_CTX_WORKAROUND)
> -				udelay(80);
> -			snd_hdac_bus_update_rirb(bus);
> +		handled = true;
> +		active = false;
> +		if (snd_hdac_bus_handle_stream_irq(bus, status,
> stream_update))
> +			active = true;
> +
> +		/* clear rirb int */
> +		status = azx_readb(chip, RIRBSTS);
> +		if (status & RIRB_INT_MASK) {
> +			active = true;
> +			if (status & RIRB_INT_RESPONSE) {
> +				if (chip->driver_caps &
> AZX_DCAPS_CTX_WORKAROUND)
> +					udelay(80);
> +				snd_hdac_bus_update_rirb(bus);
> +			}
> +			azx_writeb(chip, RIRBSTS, RIRB_INT_MASK);
>  		}
> -		azx_writeb(chip, RIRBSTS, RIRB_INT_MASK);
> -	}
> +	} while (active && ++repeat < 10);
> 
> + unlock:
>  	spin_unlock(&bus->reg_lock);
> 
> -	return IRQ_HANDLED;
> +	return IRQ_RETVAL(handled);
>  }
>  EXPORT_SYMBOL_GPL(azx_interrupt);
> 
> --
> 2.7.1