hda: track CIRB/CORB command/response states for each codec
RFC PATCH 1/1 hda: track CIRB/CORB command/response states for each codec
Recently we hit a bug in our dev board, whose HDMI codec#3 may emit redundant/spurious responses, which were then taken as responses to command for another onboard Realtek codec#2, and mess up both codecs.
Extend the azx_rb.cmds and azx_rb.res to array and track each codec's commands/responses separately. This helps keep good codec safe from broken ones.
--- sound/pci/hda/hda_codec.c | 2 sound/pci/hda/hda_codec.h | 2 sound/pci/hda/hda_intel.c | 76 +++++++++++++++++++++++++----------- 3 files changed, 56 insertions(+), 24 deletions(-)
sound/pci/hda/hda_codec.c +1 -1
@@ -185,7 +185,7 @@ static int codec_exec_verb(struct hda_comutex_lock (&bus->cmd_mutex); err = bus->ops.command (bus, cmd);if (!err && res) - *res = bus->ops.get_response (bus); + *res = bus->ops.get_response (bus, codec->addr);mutex_unlock (&bus->cmd_mutex);snd_hda_power_down (codec);if (res && *res ==-1 && bus->rirb_error) {
sound/pci/hda/hda_codec.h +1 -1
@@ -568,7 +568,7 @@ struct hda_bus_ops {/* send a single command */ int (*command)(struct hda_bus *bus,unsigned int cmd);/* get a response from the last command */ -unsigned int (*get_response)(struct hda_bus *bus); +unsigned int (*get_response)(struct hda_bus *bus,unsigned int addr);/* free the private data */ void (*private_free)(struct hda_bus *);/* attach a PCM stream */
sound/pci/hda/hda_intel.c +54 -22
198 lines not on this page · raw message
PATCH 1/1 hda: take cmd_mutex in probe_codec()
Now that each codec will have its own module, it is possible for the user to load one codec while another one is running.
So cmd_mutex would be a safe addition to probe_codec().
--- sound/pci/hda/hda_intel.c | 2 ++ 1 file changed, 2 insertions(+)
sound/pci/hda/hda_intel.c +2 -0
@@ -1282,10 +1282,12 @@ static int probe_codec(struct azx *chip, (AC_VERB_PARAMETERS <<8 ) |AC_PAR_VENDOR_ID ;unsigned int res; +mutex_lock (&chip->bus->cmd_mutex); chip->probing =1 ;azx_send_cmd (chip->bus, cmd); res =azx_get_response (chip->bus, addr); chip->probing =0 ; +mutex_unlock (&chip->bus->cmd_mutex);if (res ==-1 )return -EIO ;snd_printdd (SFX "codec #%d probed OK\n" , addr);
PATCH 1/1 hda: take reg_lock in azx_init_cmd_io/azx_free_cmd_io
In reply to Wu FengguangJust for safety. azx_init_cmd_io() and azx_free_cmd_io() may be called when switching to single command mode.
--- sound/pci/hda/hda_intel.c | 4 ++++ 1 file changed, 4 insertions(+)
sound/pci/hda/hda_intel.c +4 -0
@@ -520,6 +520,7 @@ static int azx_alloc_cmd_io(struct azx *static void azx_init_cmd_io (struct azx *chip) { +spin_lock_irq (&chip->reg_lock);/* CORB set up */ chip->corb.addr = chip->rb.addr; chip->corb.buf = (u32 *)chip->rb.area;
@@ -551,13 +552,16 @@ static void azx_init_cmd_io(struct azx *azx_writew (chip,RINTCNT ,1 );/* enable rirb dma and response irq */ azx_writeb (chip,RIRBCTL ,ICH6_RBCTL_DMA_EN |ICH6_RBCTL_IRQ_EN ); +spin_unlock_irq (&chip->reg_lock); }static void azx_free_cmd_io (struct azx *chip) { +spin_lock_irq (&chip->reg_lock);/* disable ringbuffer DMAs */ azx_writeb (chip,RIRBCTL ,0 );azx_writeb (chip,CORBCTL ,0 ); +spin_unlock_irq (&chip->reg_lock); }static unsigned int azx_command_addr (u32 cmd)
PATCH 1/1 hda: read CORBWP inside reg_lock
In reply to Wu FengguangThis converts the last CORBWP access outside of reg_lock.
--- sound/pci/hda/hda_intel.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
sound/pci/hda/hda_intel.c +3 -1
@@ -595,15 +595,17 @@ static int azx_corb_send_cmd(struct hda_unsigned int addr =azx_command_addr (val);unsigned int wp; +spin_lock_irq (&chip->reg_lock); +/* add command to corb */ wp =azx_readb (chip,CORBWP ); wp++; wp %=ICH6_MAX_CORB_ENTRIES ; -spin_lock_irq (&chip->reg_lock); chip->rirb.cmds[addr]++; chip->corb.buf[wp] =cpu_to_le32 (val);azx_writel (chip,CORBWP , wp); +spin_unlock_irq (&chip->reg_lock);return 0 ;
PATCH 1/1 hda: remember last command for each codec
In reply to Wu Fengguang--- sound/pci/hda/hda_intel.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
sound/pci/hda/hda_intel.c +6 -5
@@ -425,7 +425,7 @@ struct azx { unsigned int probing :1; /* codec probing phase *//* for debugging */ -unsigned int last_cmd;/* last issued command (to sync) */ +unsigned int last_cmd[AZX_MAX_CODECS ];/* for pending irqs */ struct work_struct irq_pending_work;
@@ -675,7 +675,8 @@ static unsigned int azx_rirb_get_responsif (chip->msi) { snd_printk(KERN_WARNING SFX "No response from codec, " - "disabling MSI: last cmd=0x%08x\n", chip->last_cmd); +"disabling MSI: last cmd=0x%08x\n" , + chip->last_cmd[addr]);free_irq (chip->irq, chip); chip->irq =-1 ;pci_disable_msi (chip->pci);
@@ -690,7 +691,7 @@ static unsigned int azx_rirb_get_responsif (!chip->polling_mode) { snd_printk(KERN_WARNING SFX "azx_get_response timeout, ""switching to polling mode: last cmd=0x%08x\n" , - chip->last_cmd); + chip->last_cmd[addr]); chip->polling_mode =1 ; goto again; }
@@ -714,7 +715,7 @@ static unsigned int azx_rirb_get_responssnd_printk (KERN_ERR "hda_intel: azx_get_response timeout, " "switching to single_cmd mode: last cmd=0x%08x\n" , - chip->last_cmd); + chip->last_cmd[addr]); chip->single_cmd =1 ; bus->response_reset =0 ;/* re-initialize CORB/RIRB */
@@ -801,7 +802,7 @@ static int azx_send_cmd(struct hda_bus * {struct azx *chip = bus->private_data; - chip->last_cmd = val; + chip->last_cmd[azx_command_addr (val)] = val;if (chip->single_cmd)return azx_single_send_cmd (bus, val);else
PATCH 1/1 hda: warn on spurious response
To help disclose hardware bugs.
--- sound/pci/hda/hda_intel.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
sound/pci/hda/hda_intel.c +5 -1
@@ -639,7 +639,11 @@ static void azx_update_rirb(struct azx * chip->rirb.res[addr] = res;smp_wmb (); chip->rirb.cmds[addr]--; - } + }else + snd_printk(KERN_ERR SFX "spurious response %#x:%#x, " +"last cmd=%#08x\n" , + res, res_ex, + chip->last_cmd[addr]); } }
At Sat, 1 Aug 2009 18:45:16 +0800,
Quoted 10 lines · Wu Fengguang
Recently we hit a bug in our dev board, whose HDMI codec#3 may emit redundant/spurious responses, which were then taken as responses to command for another onboard Realtek codec#2, and mess up both codecs.
Extend the azx_rb.cmds and azx_rb.res to array and track each codec's commands/responses separately. This helps keep good codec safe from broken ones.
Signed-off-by Wu Fengguang <[email protected]>
Good catch. I applied this series to fix/hda branch to be included in the next pull request.
thanks,
Takashi
Quoted 245 lines
--- sound/pci/hda/hda_codec.c | 2 sound/pci/hda/hda_codec.h | 2 sound/pci/hda/hda_intel.c | 76 +++++++++++++++++++++++++----------- 3 files changed, 56 insertions(+), 24 deletions(-)sound/pci/hda/hda_codec.c +1
-1@@ -185,7 +185,7 @@ static int codec_exec_verb(struct hda_comutex_lock (&bus->cmd_mutex); err = bus->ops.command (bus, cmd);if (!err && res) - *res = bus->ops.get_response (bus); + *res = bus->ops.get_response (bus, codec->addr);mutex_unlock (&bus->cmd_mutex);snd_hda_power_down (codec);if (res && *res ==-1 && bus->rirb_error) {sound/pci/hda/hda_codec.h +1
-1@@ -568,7 +568,7 @@ struct hda_bus_ops {/* send a single command */ int (*command)(struct hda_bus *bus,unsigned int cmd);/* get a response from the last command */ -unsigned int (*get_response)(struct hda_bus *bus); +unsigned int (*get_response)(struct hda_bus *bus,unsigned int addr);/* free the private data */ void (*private_free)(struct hda_bus *);/* attach a PCM stream */ sound/pci/hda/hda_intel.c +54
-22@@ -260,7 +260,7 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO/* STATESTS int mask: S3,SD2,SD1,SD0 */ #define AZX_MAX_CODECS 4 -#define STATESTS_INT_MASK 0x0f +#define STATESTS_INT_MASK ((1 << AZX_MAX_CODECS) - 1)/* SD_CTL bits */ #define SD_CTL_STREAM_RESET 0x01/* stream reset bit */ @@ -368,8 +368,8 @@ struct azx_rb {dma_addr_t addr;/* physical address of CORB/RIRB buffer */ /* for RIRB */ unsigned short rp, wp;/* read/write pointers */ -int cmds;/* number of pending requests */ -u32 res;/* last read value */ +int cmds[AZX_MAX_CODECS ];/* number of pending requests */ +u32 res[AZX_MAX_CODECS ];/* last read value */ };struct azx {@@ -538,7 +538,8 @@ static void azx_init_cmd_io(struct azx */* RIRB set up */ chip->rirb.addr = chip->rb.addr +2048 ; chip->rirb.buf = (u32 *)(chip->rb.area +2048 ); - chip->rirb.wp = chip->rirb.rp = chip->rirb.cmds =0 ; + chip->rirb.wp = chip->rirb.rp =0 ; +memset (chip->rirb.cmds,0 ,sizeof (chip->rirb.cmds));azx_writel (chip,RIRBLBASE , (u32 )chip->rirb.addr);azx_writel (chip,RIRBUBASE ,upper_32_bits (chip->rirb.addr));@@ -559,10 +560,35 @@ static void azx_free_cmd_io(struct azx *azx_writeb (chip,CORBCTL ,0 ); } +static unsigned int azx_command_addr (u32 cmd) +{ +unsigned int addr = cmd >>28 ; + +if (addr >=AZX_MAX_CODECS ) { +snd_BUG (); + addr =0 ; + } + +return addr; +} + +static unsigned int azx_response_addr (u32 res) +{ +unsigned int addr = res &0xf ; + +if (addr >=AZX_MAX_CODECS ) { +snd_BUG (); + addr =0 ; + } + +return addr; +} +/* send a command */ static int azx_corb_send_cmd (struct hda_bus *bus,u32 val) {struct azx *chip = bus->private_data; +unsigned int addr =azx_command_addr (val);unsigned int wp;/* add command to corb */ @@ -571,7 +597,7 @@ static int azx_corb_send_cmd(struct hda_ wp %=ICH6_MAX_CORB_ENTRIES ;spin_lock_irq (&chip->reg_lock); - chip->rirb.cmds++; + chip->rirb.cmds[addr]++; chip->corb.buf[wp] =cpu_to_le32 (val);azx_writel (chip,CORBWP , wp);spin_unlock_irq (&chip->reg_lock);@@ -585,13 +611,14 @@ static int azx_corb_send_cmd(struct hda_static void azx_update_rirb (struct azx *chip) {unsigned int rp, wp; +unsigned int addr;u32 res, res_ex; wp =azx_readb (chip,RIRBWP );if (wp == chip->rirb.wp)return ; chip->rirb.wp = wp; - +while (chip->rirb.rp != wp) { chip->rirb.rp++; chip->rirb.rp %=ICH6_MAX_RIRB_ENTRIES ;@@ -599,18 +626,20 @@ static void azx_update_rirb(struct azx * rp = chip->rirb.rp <<1 ;/* an RIRB entry is 8-bytes */ res_ex =le32_to_cpu (chip->rirb.buf[rp +1 ]); res =le32_to_cpu (chip->rirb.buf[rp]); + addr =azx_response_addr (res_ex);if (res_ex &ICH6_RIRB_EX_UNSOL_EV )snd_hda_queue_unsol_event (chip->bus, res, res_ex); -else if (chip->rirb.cmds) { - chip->rirb.res = res; +else if (chip->rirb.cmds[addr]) { + chip->rirb.res[addr] = res;smp_wmb (); - chip->rirb.cmds--; + chip->rirb.cmds[addr]--; } } }/* receive a response */ -static unsigned int azx_rirb_get_response (struct hda_bus *bus) +static unsigned int azx_rirb_get_response (struct hda_bus *bus, +unsigned int addr) {struct azx *chip = bus->private_data;unsigned long timeout;@@ -623,10 +652,10 @@ static unsigned int azx_rirb_get_responsazx_update_rirb (chip);spin_unlock_irq (&chip->reg_lock); } -if (!chip->rirb.cmds) { +if (!chip->rirb.cmds[addr]) {smp_rmb (); bus->rirb_error =0 ; -return chip->rirb.res;/* the last value */ +return chip->rirb.res[addr];/* the last value */ }if (time_after (jiffies, timeout))break ;@@ -699,7 +728,7 @@ static unsigned int azx_rirb_get_respons*/ /* receive a response */ -static int azx_single_wait_for_response (struct azx *chip) +static int azx_single_wait_for_response (struct azx *chip,unsigned int addr) {int timeout =50 ;@@ -707,7 +736,7 @@ static int azx_single_wait_for_response(/* check IRV busy bit */ if (azx_readw (chip,IRS ) &ICH6_IRS_VALID ) {/* reuse rirb.res as the response return value */ - chip->rirb.res =azx_readl (chip,IR ); + chip->rirb.res[addr] =azx_readl (chip,IR );return 0 ; }udelay (1 );@@ -715,7 +744,7 @@ static int azx_single_wait_for_response(if (printk_ratelimit ())snd_printd (SFX "get_response timeout: IRS=0x%x\n" ,azx_readw (chip,IRS )); - chip->rirb.res =-1 ; + chip->rirb.res[addr] =-1 ;return -EIO ; }@@ -723,6 +752,7 @@ static int azx_single_wait_for_response(static int azx_single_send_cmd (struct hda_bus *bus,u32 val) {struct azx *chip = bus->private_data; +unsigned int addr =azx_command_addr (val);int timeout =50 ; bus->rirb_error =0 ;@@ -735,7 +765,7 @@ static int azx_single_send_cmd(struct hdazx_writel (chip,IC , val);azx_writew (chip,IRS ,azx_readw (chip,IRS ) |ICH6_IRS_BUSY ); -return azx_single_wait_for_response (chip); +return azx_single_wait_for_response (chip, addr); }udelay (1 ); }@@ -746,10 +776,11 @@ static int azx_single_send_cmd(struct hd }/* receive a response */ -static unsigned int azx_single_get_response (struct hda_bus *bus) +static unsigned int azx_single_get_response (struct hda_bus *bus, +unsigned int addr) {struct azx *chip = bus->private_data; -return chip->rirb.res; +return chip->rirb.res[addr]; }/* @@ -772,13 +803,14 @@ static int azx_send_cmd(struct hda_bus * }/* get a response */ -static unsigned int azx_get_response (struct hda_bus *bus) +static unsigned int azx_get_response (struct hda_bus *bus, +unsigned int addr) {struct azx *chip = bus->private_data;if (chip->single_cmd) -return azx_single_get_response (bus); +return azx_single_get_response (bus, addr);else -return azx_rirb_get_response (bus); +return azx_rirb_get_response (bus, addr); }#ifdef CONFIG_SND_HDA_POWER_SAVE @@ -1252,7 +1284,7 @@ static int probe_codec(struct azx *chip, chip->probing =1 ;azx_send_cmd (chip->bus, cmd); - res =azx_get_response (chip->bus); + res =azx_get_response (chip->bus, addr); chip->probing =0 ;if (res ==-1 )return -EIO ;