[PATCH 0/3] ALSA: hdac: stream fixes

[PATCH 0/3] ALSA: hdac: stream fixes

[Vinod Koul]

Hi Takashi,

I had few off fixes on hdac piled up so time to send them out.

First one is a fix for stream assignment for decoupled mode used in SKL
driver. Second one is static checker fix reported by Dan Carpenter
Last is a potential issue which our team reported so a WA for that. The
command DMAs do not finish instantaneously, so we should per spec wait till
DMA finishes

Jeeja KP (2):
  ALSA: HDA: Fix stream assignment for host in decoupled mode
  ALSA: HDA: Dont check return for snd_hdac_chip_readl

Vinod Koul (1):
  ALSA: HDA: wait for RIRB, CORB DMA to finish

 sound/hda/ext/hdac_ext_controller.c |  6 ------
 sound/hda/ext/hdac_ext_stream.c     |  2 +-
 sound/hda/hdac_controller.c         | 14 ++++++++++++++
 3 files changed, 15 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/3] ALSA: HDA: Fix stream assignment for host in decoupled mode

[Vinod Koul]

From: Jeeja KP <jeeja.kp@intel.com>

This fixes issue in assigning host stream in case of
decoupled mode. The check to verify if the stream is already
in use was wrong so fix that

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/hda/ext/hdac_ext_stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c
index f8ffbdbb450d..3de47dd1a76d 100644
--- a/sound/hda/ext/hdac_ext_stream.c
+++ b/sound/hda/ext/hdac_ext_stream.c
@@ -299,7 +299,7 @@ hdac_ext_host_stream_assign(struct hdac_ext_bus *ebus,
 		if (stream->direction != substream->stream)
 			continue;
 
-		if (stream->opened) {
+		if (!stream->opened) {
 			if (!hstream->decoupled)
 				snd_hdac_ext_stream_decouple(ebus, hstream, true);
 			res = hstream;
-- 
1.9.1

Re: [PATCH 1/3] ALSA: HDA: Fix stream assignment for host in decoupled mode

[Takashi Iwai]

On Tue, 04 Aug 2015 05:58:38 +0200,
Vinod Koul wrote:
> 
> From: Jeeja KP <jeeja.kp@intel.com>
> 
> This fixes issue in assigning host stream in case of
> decoupled mode. The check to verify if the stream is already
> in use was wrong so fix that
> 
> Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>

Applied, thanks.


Takashi

> ---
>  sound/hda/ext/hdac_ext_stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c
> index f8ffbdbb450d..3de47dd1a76d 100644
> --- a/sound/hda/ext/hdac_ext_stream.c
> +++ b/sound/hda/ext/hdac_ext_stream.c
> @@ -299,7 +299,7 @@ hdac_ext_host_stream_assign(struct hdac_ext_bus *ebus,
>  		if (stream->direction != substream->stream)
>  			continue;
>  
> -		if (stream->opened) {
> +		if (!stream->opened) {
>  			if (!hstream->decoupled)
>  				snd_hdac_ext_stream_decouple(ebus, hstream, true);
>  			res = hstream;
> -- 
> 1.9.1
> 

[PATCH 2/3] ALSA: HDA: Dont check return for snd_hdac_chip_readl

[Vinod Koul]

From: Jeeja KP <jeeja.kp@intel.com>

The snd_hdac_chip_readl return can never be less than zeros,
so no point in checking for the return value

This fixes following static checker warnings in
snd_hdac_ext_bus_parse_capabilities

       sound/hda/ext/hdac_ext_controller.c:47
 snd_hdac_ext_bus_parse_capabilities()
       warn: unsigned 'offset' is never less than zero.

       sound/hda/ext/hdac_ext_controller.c:54
 snd_hdac_ext_bus_parse_capabilities()
       warn: unsigned 'cur_cap' is never less than zero.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/hda/ext/hdac_ext_controller.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/sound/hda/ext/hdac_ext_controller.c b/sound/hda/ext/hdac_ext_controller.c
index b2da19b60f4e..358f16195483 100644
--- a/sound/hda/ext/hdac_ext_controller.c
+++ b/sound/hda/ext/hdac_ext_controller.c
@@ -44,16 +44,10 @@ int snd_hdac_ext_bus_parse_capabilities(struct hdac_ext_bus *ebus)
 
 	offset = snd_hdac_chip_readl(bus, LLCH);
 
-	if (offset < 0)
-		return -EIO;
-
 	/* Lets walk the linked capabilities list */
 	do {
 		cur_cap = _snd_hdac_chip_read(l, bus, offset);
 
-		if (cur_cap < 0)
-			return -EIO;
-
 		dev_dbg(bus->dev, "Capability version: 0x%x\n",
 				((cur_cap & AZX_CAP_HDR_VER_MASK) >> AZX_CAP_HDR_VER_OFF));
 
-- 
1.9.1

Re: [PATCH 2/3] ALSA: HDA: Dont check return for snd_hdac_chip_readl

[Takashi Iwai]

On Tue, 04 Aug 2015 05:58:39 +0200,
Vinod Koul wrote:
> 
> From: Jeeja KP <jeeja.kp@intel.com>
> 
> The snd_hdac_chip_readl return can never be less than zeros,
> so no point in checking for the return value
> 
> This fixes following static checker warnings in
> snd_hdac_ext_bus_parse_capabilities
> 
>        sound/hda/ext/hdac_ext_controller.c:47
>  snd_hdac_ext_bus_parse_capabilities()
>        warn: unsigned 'offset' is never less than zero.
> 
>        sound/hda/ext/hdac_ext_controller.c:54
>  snd_hdac_ext_bus_parse_capabilities()
>        warn: unsigned 'cur_cap' is never less than zero.
> 
> Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>

Applied, thanks.


Takashi

> ---
>  sound/hda/ext/hdac_ext_controller.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/sound/hda/ext/hdac_ext_controller.c b/sound/hda/ext/hdac_ext_controller.c
> index b2da19b60f4e..358f16195483 100644
> --- a/sound/hda/ext/hdac_ext_controller.c
> +++ b/sound/hda/ext/hdac_ext_controller.c
> @@ -44,16 +44,10 @@ int snd_hdac_ext_bus_parse_capabilities(struct hdac_ext_bus *ebus)
>  
>  	offset = snd_hdac_chip_readl(bus, LLCH);
>  
> -	if (offset < 0)
> -		return -EIO;
> -
>  	/* Lets walk the linked capabilities list */
>  	do {
>  		cur_cap = _snd_hdac_chip_read(l, bus, offset);
>  
> -		if (cur_cap < 0)
> -			return -EIO;
> -
>  		dev_dbg(bus->dev, "Capability version: 0x%x\n",
>  				((cur_cap & AZX_CAP_HDR_VER_MASK) >> AZX_CAP_HDR_VER_OFF));
>  
> -- 
> 1.9.1
> 

[PATCH 3/3] ALSA: HDA: wait for RIRB, CORB DMA to finish

[Vinod Koul]

HDA spec says that RORB and CORB DMA stop will take some
time to complete.  So we should wait till the DMAs are
stopped.

Although the current controllers don't have multilinks so
doesn't impact much, but SKL onwards we have multiple links
so waiting for DMAs to stop makes better sense.

Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/hda/hdac_controller.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index b5a17cb510a0..3b5d07174d79 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -86,10 +86,24 @@ EXPORT_SYMBOL_GPL(snd_hdac_bus_init_cmd_io);
  */
 void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus)
 {
+	unsigned long timeout;
+
 	spin_lock_irq(&bus->reg_lock);
 	/* disable ringbuffer DMAs */
 	snd_hdac_chip_writeb(bus, RIRBCTL, 0);
 	snd_hdac_chip_writeb(bus, CORBCTL, 0);
+
+	/* poll DMAs to check if they stopped or not */
+
+	timeout = jiffies + msecs_to_jiffies(100);
+	while ((snd_hdac_chip_readb(bus, RIRBCTL) & AZX_RBCTL_DMA_EN) &&
+	       time_before(jiffies, timeout))
+		usleep_range(500, 1000);
+	timeout = jiffies + msecs_to_jiffies(100);
+	while ((snd_hdac_chip_readb(bus, CORBCTL) & AZX_CORBCTL_RUN) &&
+	       time_before(jiffies, timeout))
+		usleep_range(500, 1000);
+
 	/* disable unsolicited responses */
 	snd_hdac_chip_updatel(bus, GCTL, AZX_GCTL_UNSOL, 0);
 	spin_unlock_irq(&bus->reg_lock);
-- 
1.9.1

Re: [PATCH 3/3] ALSA: HDA: wait for RIRB, CORB DMA to finish

[Takashi Iwai]

On Tue, 04 Aug 2015 05:58:40 +0200,
Vinod Koul wrote:
> 
> HDA spec says that RORB and CORB DMA stop will take some
> time to complete.  So we should wait till the DMAs are
> stopped.
> 
> Although the current controllers don't have multilinks so
> doesn't impact much, but SKL onwards we have multiple links
> so waiting for DMAs to stop makes better sense.
> 
> Signed-off-by: Jeeja KP <jeeja.kp@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  sound/hda/hdac_controller.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
> index b5a17cb510a0..3b5d07174d79 100644
> --- a/sound/hda/hdac_controller.c
> +++ b/sound/hda/hdac_controller.c
> @@ -86,10 +86,24 @@ EXPORT_SYMBOL_GPL(snd_hdac_bus_init_cmd_io);
>   */
>  void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus)
>  {
> +	unsigned long timeout;
> +
>  	spin_lock_irq(&bus->reg_lock);
>  	/* disable ringbuffer DMAs */
>  	snd_hdac_chip_writeb(bus, RIRBCTL, 0);
>  	snd_hdac_chip_writeb(bus, CORBCTL, 0);
> +
> +	/* poll DMAs to check if they stopped or not */
> +
> +	timeout = jiffies + msecs_to_jiffies(100);
> +	while ((snd_hdac_chip_readb(bus, RIRBCTL) & AZX_RBCTL_DMA_EN) &&
> +	       time_before(jiffies, timeout))
> +		usleep_range(500, 1000);

You must not use *sleep() inside atomic context.


Takashi

Re: [PATCH 3/3] ALSA: HDA: wait for RIRB, CORB DMA to finish

[Vinod Koul]

On Tue, Aug 04, 2015 at 07:13:12AM +0200, Takashi Iwai wrote:
> > @@ -86,10 +86,24 @@ EXPORT_SYMBOL_GPL(snd_hdac_bus_init_cmd_io);
> >   */
> >  void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus)
> >  {
> > +	unsigned long timeout;
> > +
> >  	spin_lock_irq(&bus->reg_lock);
> >  	/* disable ringbuffer DMAs */
> >  	snd_hdac_chip_writeb(bus, RIRBCTL, 0);
> >  	snd_hdac_chip_writeb(bus, CORBCTL, 0);
> > +
> > +	/* poll DMAs to check if they stopped or not */
> > +
> > +	timeout = jiffies + msecs_to_jiffies(100);
> > +	while ((snd_hdac_chip_readb(bus, RIRBCTL) & AZX_RBCTL_DMA_EN) &&
> > +	       time_before(jiffies, timeout))
> > +		usleep_range(500, 1000);
> 
> You must not use *sleep() inside atomic context.
Right, not sure why it didnt crib when we tested, will send updated one soon

Thanks for the super quick review :)

-- 
~Vinod