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

[Takashi Iwai <tiwai@suse.de>]

On Fri, 26 Feb 2016 06:22:34 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> Our QA has tested your patch. It works and the basic test cases passed.

OK, I queued it up now.


thanks,

Takashi

> 
> Regards,
> Libin
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, February 23, 2016 11:04 PM
> > To: Yang, Libin
> > Cc: Lin, Mengdong; libin.yang@linux.intel.com; alsa-devel@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@alsa-project.org [mailto:alsa-devel-
> > > > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > > > Sent: Tuesday, February 23, 2016 5:28 PM
> > > > To: Yang, Libin
> > > > Cc: Lin, Mengdong; libin.yang@linux.intel.com; alsa-devel@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@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@linux.intel.com>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@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
>