* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-05-16 9:49 ` Takashi Iwai
0 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-05-16 9:49 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: alsa-devel, Leon Romanovsky, Takashi Iwai, linux-kernel,
Gustavo A. R. Silva
On Sun, 16 May 2021 10:31:41 +0200,
Sergey Senozhatsky wrote:
>
> On (21/05/16 17:30), Sergey Senozhatsky wrote:
> > On (21/05/14 20:16), Sergey Senozhatsky wrote:
> > > > --- a/sound/pci/intel8x0.c
> > > > +++ b/sound/pci/intel8x0.c
> > > > @@ -691,6 +691,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > > > int status, civ, i, step;
> > > > int ack = 0;
> > > >
> > > > + if (!ichdev->substream || ichdev->suspended)
> > > > + return;
> > > > +
> > > > spin_lock_irqsave(&chip->reg_lock, flags);
> > > > status = igetbyte(chip, port + ichdev->roff_sr);
> > > > civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
> >
> > This does the problem for me.
>
> ^^^ does fix
OK, thanks for confirmation. So this looks like some spurious
interrupt with the unexpected hardware bits.
However, the suggested check doesn't seem covering enough, and it
might still hit if the suspend/resume happens before the device is
opened but not set up (and such a spurious irq is triggered).
Below is more comprehensive fix. Let me know if this works, too.
thanks,
Takashi
-- 8< --
Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
the hardware sets the corresponding status bit for each stream. This
works fine for most cases as long as the hardware behaves properly.
But when the hardware gives a wrong bit set, this leads to a NULL
dereference Oops, and reportedly, this seems what happened on a VM.
For fixing the crash, this patch adds a internal flag indicating that
the stream is ready to be updated, and check it (as well as the flag
being in suspended) to ignore such spurious update.
Cc: <stable@vger.kernel.org>
Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/pci/intel8x0.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 35903d1a1cbd..5b124c4ad572 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -331,6 +331,7 @@ struct ichdev {
unsigned int ali_slot; /* ALI DMA slot */
struct ac97_pcm *pcm;
int pcm_open_flag;
+ unsigned int prepared:1;
unsigned int suspended: 1;
};
@@ -691,6 +692,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
int status, civ, i, step;
int ack = 0;
+ if (!ichdev->prepared || ichdev->suspended)
+ return;
+
spin_lock_irqsave(&chip->reg_lock, flags);
status = igetbyte(chip, port + ichdev->roff_sr);
civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
@@ -881,6 +885,7 @@ static int snd_intel8x0_hw_params(struct snd_pcm_substream *substream,
if (ichdev->pcm_open_flag) {
snd_ac97_pcm_close(ichdev->pcm);
ichdev->pcm_open_flag = 0;
+ ichdev->prepared = 0;
}
err = snd_ac97_pcm_open(ichdev->pcm, params_rate(hw_params),
params_channels(hw_params),
@@ -902,6 +907,7 @@ static int snd_intel8x0_hw_free(struct snd_pcm_substream *substream)
if (ichdev->pcm_open_flag) {
snd_ac97_pcm_close(ichdev->pcm);
ichdev->pcm_open_flag = 0;
+ ichdev->prepared = 0;
}
return 0;
}
@@ -976,6 +982,7 @@ static int snd_intel8x0_pcm_prepare(struct snd_pcm_substream *substream)
ichdev->pos_shift = (runtime->sample_bits > 16) ? 2 : 1;
}
snd_intel8x0_setup_periods(chip, ichdev);
+ ichdev->prepared = 1;
return 0;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-05-16 9:49 ` Takashi Iwai
@ 2021-05-16 10:59 ` Sergey Senozhatsky
-1 siblings, 0 replies; 38+ messages in thread
From: Sergey Senozhatsky @ 2021-05-16 10:59 UTC (permalink / raw)
To: Takashi Iwai
Cc: Sergey Senozhatsky, Jaroslav Kysela, Takashi Iwai,
Gustavo A. R. Silva, Leon Romanovsky, alsa-devel, linux-kernel
On (21/05/16 11:49), Takashi Iwai wrote:
> Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
>
> The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> the hardware sets the corresponding status bit for each stream. This
> works fine for most cases as long as the hardware behaves properly.
> But when the hardware gives a wrong bit set, this leads to a NULL
> dereference Oops, and reportedly, this seems what happened on a VM.
>
> For fixing the crash, this patch adds a internal flag indicating that
> the stream is ready to be updated, and check it (as well as the flag
> being in suspended) to ignore such spurious update.
>
> Cc: <stable@vger.kernel.org>
> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
I kicked the tests. Will let you know.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-05-16 10:59 ` Sergey Senozhatsky
0 siblings, 0 replies; 38+ messages in thread
From: Sergey Senozhatsky @ 2021-05-16 10:59 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Leon Romanovsky, linux-kernel, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On (21/05/16 11:49), Takashi Iwai wrote:
> Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
>
> The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> the hardware sets the corresponding status bit for each stream. This
> works fine for most cases as long as the hardware behaves properly.
> But when the hardware gives a wrong bit set, this leads to a NULL
> dereference Oops, and reportedly, this seems what happened on a VM.
>
> For fixing the crash, this patch adds a internal flag indicating that
> the stream is ready to be updated, and check it (as well as the flag
> being in suspended) to ignore such spurious update.
>
> Cc: <stable@vger.kernel.org>
> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
I kicked the tests. Will let you know.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-05-16 9:49 ` Takashi Iwai
@ 2021-05-16 11:23 ` Sergey Senozhatsky
-1 siblings, 0 replies; 38+ messages in thread
From: Sergey Senozhatsky @ 2021-05-16 11:23 UTC (permalink / raw)
To: Takashi Iwai
Cc: Sergey Senozhatsky, Jaroslav Kysela, Takashi Iwai,
Gustavo A. R. Silva, Leon Romanovsky, alsa-devel, linux-kernel
On (21/05/16 11:49), Takashi Iwai wrote:
> Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
>
> The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> the hardware sets the corresponding status bit for each stream. This
> works fine for most cases as long as the hardware behaves properly.
> But when the hardware gives a wrong bit set, this leads to a NULL
> dereference Oops, and reportedly, this seems what happened on a VM.
VM, yes. I didn't see NULL derefs, my VMs crash because of div by
zero in `% size`.
> For fixing the crash, this patch adds a internal flag indicating that
> the stream is ready to be updated, and check it (as well as the flag
> being in suspended) to ignore such spurious update.
I reproduced the "spurious IRQ" case, and the patch handled it correctly
(VM did not crash).
> Cc: <stable@vger.kernel.org>
> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
I'll keep running test, but seems that it works as intended
Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-05-16 11:23 ` Sergey Senozhatsky
0 siblings, 0 replies; 38+ messages in thread
From: Sergey Senozhatsky @ 2021-05-16 11:23 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Leon Romanovsky, linux-kernel, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On (21/05/16 11:49), Takashi Iwai wrote:
> Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
>
> The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> the hardware sets the corresponding status bit for each stream. This
> works fine for most cases as long as the hardware behaves properly.
> But when the hardware gives a wrong bit set, this leads to a NULL
> dereference Oops, and reportedly, this seems what happened on a VM.
VM, yes. I didn't see NULL derefs, my VMs crash because of div by
zero in `% size`.
> For fixing the crash, this patch adds a internal flag indicating that
> the stream is ready to be updated, and check it (as well as the flag
> being in suspended) to ignore such spurious update.
I reproduced the "spurious IRQ" case, and the patch handled it correctly
(VM did not crash).
> Cc: <stable@vger.kernel.org>
> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
I'll keep running test, but seems that it works as intended
Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-05-16 11:23 ` Sergey Senozhatsky
@ 2021-05-16 12:07 ` Takashi Iwai
-1 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-05-16 12:07 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Jaroslav Kysela, Takashi Iwai, Gustavo A. R. Silva,
Leon Romanovsky, alsa-devel, linux-kernel
On Sun, 16 May 2021 13:23:21 +0200,
Sergey Senozhatsky wrote:
>
> On (21/05/16 11:49), Takashi Iwai wrote:
> > Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
> >
> > The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> > the hardware sets the corresponding status bit for each stream. This
> > works fine for most cases as long as the hardware behaves properly.
> > But when the hardware gives a wrong bit set, this leads to a NULL
> > dereference Oops, and reportedly, this seems what happened on a VM.
>
> VM, yes. I didn't see NULL derefs, my VMs crash because of div by
> zero in `% size`.
Ah, right, I'll fix the description.
> > For fixing the crash, this patch adds a internal flag indicating that
> > the stream is ready to be updated, and check it (as well as the flag
> > being in suspended) to ignore such spurious update.
>
> I reproduced the "spurious IRQ" case, and the patch handled it correctly
> (VM did not crash).
>
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
>
> I'll keep running test, but seems that it works as intended
>
> Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
OK, below is the revised patch I'm going to apply.
Thanks!
Takashi
-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: intel8x0: Don't update period unless prepared
The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
the hardware sets the corresponding status bit for each stream. This
works fine for most cases as long as the hardware behaves properly.
But when the hardware gives a wrong bit set, this leads to a zero-
division Oops, and reportedly, this seems what happened on a VM.
For fixing the crash, this patch adds a internal flag indicating that
the stream is ready to be updated, and check it (as well as the flag
being in suspended) to ignore such spurious update.
Cc: <stable@vger.kernel.org>
Reported-and-tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: fixed description, updated tested-by tag
sound/pci/intel8x0.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 35903d1a1cbd..5b124c4ad572 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -331,6 +331,7 @@ struct ichdev {
unsigned int ali_slot; /* ALI DMA slot */
struct ac97_pcm *pcm;
int pcm_open_flag;
+ unsigned int prepared:1;
unsigned int suspended: 1;
};
@@ -691,6 +692,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
int status, civ, i, step;
int ack = 0;
+ if (!ichdev->prepared || ichdev->suspended)
+ return;
+
spin_lock_irqsave(&chip->reg_lock, flags);
status = igetbyte(chip, port + ichdev->roff_sr);
civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
@@ -881,6 +885,7 @@ static int snd_intel8x0_hw_params(struct snd_pcm_substream *substream,
if (ichdev->pcm_open_flag) {
snd_ac97_pcm_close(ichdev->pcm);
ichdev->pcm_open_flag = 0;
+ ichdev->prepared = 0;
}
err = snd_ac97_pcm_open(ichdev->pcm, params_rate(hw_params),
params_channels(hw_params),
@@ -902,6 +907,7 @@ static int snd_intel8x0_hw_free(struct snd_pcm_substream *substream)
if (ichdev->pcm_open_flag) {
snd_ac97_pcm_close(ichdev->pcm);
ichdev->pcm_open_flag = 0;
+ ichdev->prepared = 0;
}
return 0;
}
@@ -976,6 +982,7 @@ static int snd_intel8x0_pcm_prepare(struct snd_pcm_substream *substream)
ichdev->pos_shift = (runtime->sample_bits > 16) ? 2 : 1;
}
snd_intel8x0_setup_periods(chip, ichdev);
+ ichdev->prepared = 1;
return 0;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-05-16 12:07 ` Takashi Iwai
0 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-05-16 12:07 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: alsa-devel, Leon Romanovsky, Takashi Iwai, linux-kernel,
Gustavo A. R. Silva
On Sun, 16 May 2021 13:23:21 +0200,
Sergey Senozhatsky wrote:
>
> On (21/05/16 11:49), Takashi Iwai wrote:
> > Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
> >
> > The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> > the hardware sets the corresponding status bit for each stream. This
> > works fine for most cases as long as the hardware behaves properly.
> > But when the hardware gives a wrong bit set, this leads to a NULL
> > dereference Oops, and reportedly, this seems what happened on a VM.
>
> VM, yes. I didn't see NULL derefs, my VMs crash because of div by
> zero in `% size`.
Ah, right, I'll fix the description.
> > For fixing the crash, this patch adds a internal flag indicating that
> > the stream is ready to be updated, and check it (as well as the flag
> > being in suspended) to ignore such spurious update.
>
> I reproduced the "spurious IRQ" case, and the patch handled it correctly
> (VM did not crash).
>
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
>
> I'll keep running test, but seems that it works as intended
>
> Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
OK, below is the revised patch I'm going to apply.
Thanks!
Takashi
-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: intel8x0: Don't update period unless prepared
The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
the hardware sets the corresponding status bit for each stream. This
works fine for most cases as long as the hardware behaves properly.
But when the hardware gives a wrong bit set, this leads to a zero-
division Oops, and reportedly, this seems what happened on a VM.
For fixing the crash, this patch adds a internal flag indicating that
the stream is ready to be updated, and check it (as well as the flag
being in suspended) to ignore such spurious update.
Cc: <stable@vger.kernel.org>
Reported-and-tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: fixed description, updated tested-by tag
sound/pci/intel8x0.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 35903d1a1cbd..5b124c4ad572 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -331,6 +331,7 @@ struct ichdev {
unsigned int ali_slot; /* ALI DMA slot */
struct ac97_pcm *pcm;
int pcm_open_flag;
+ unsigned int prepared:1;
unsigned int suspended: 1;
};
@@ -691,6 +692,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
int status, civ, i, step;
int ack = 0;
+ if (!ichdev->prepared || ichdev->suspended)
+ return;
+
spin_lock_irqsave(&chip->reg_lock, flags);
status = igetbyte(chip, port + ichdev->roff_sr);
civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
@@ -881,6 +885,7 @@ static int snd_intel8x0_hw_params(struct snd_pcm_substream *substream,
if (ichdev->pcm_open_flag) {
snd_ac97_pcm_close(ichdev->pcm);
ichdev->pcm_open_flag = 0;
+ ichdev->prepared = 0;
}
err = snd_ac97_pcm_open(ichdev->pcm, params_rate(hw_params),
params_channels(hw_params),
@@ -902,6 +907,7 @@ static int snd_intel8x0_hw_free(struct snd_pcm_substream *substream)
if (ichdev->pcm_open_flag) {
snd_ac97_pcm_close(ichdev->pcm);
ichdev->pcm_open_flag = 0;
+ ichdev->prepared = 0;
}
return 0;
}
@@ -976,6 +982,7 @@ static int snd_intel8x0_pcm_prepare(struct snd_pcm_substream *substream)
ichdev->pos_shift = (runtime->sample_bits > 16) ? 2 : 1;
}
snd_intel8x0_setup_periods(chip, ichdev);
+ ichdev->prepared = 1;
return 0;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-05-16 12:07 ` Takashi Iwai
@ 2021-05-16 12:55 ` Sergey Senozhatsky
-1 siblings, 0 replies; 38+ messages in thread
From: Sergey Senozhatsky @ 2021-05-16 12:55 UTC (permalink / raw)
To: Takashi Iwai
Cc: Sergey Senozhatsky, Jaroslav Kysela, Takashi Iwai,
Gustavo A. R. Silva, Leon Romanovsky, alsa-devel, linux-kernel
On (21/05/16 14:07), Takashi Iwai wrote:
> > > For fixing the crash, this patch adds a internal flag indicating that
> > > the stream is ready to be updated, and check it (as well as the flag
> > > being in suspended) to ignore such spurious update.
> >
> > I reproduced the "spurious IRQ" case, and the patch handled it correctly
> > (VM did not crash).
> >
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >
> > I'll keep running test, but seems that it works as intended
> >
> > Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
>
> OK, below is the revised patch I'm going to apply.
>
Sounds good.
> Thanks!
Thank you.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-05-16 12:55 ` Sergey Senozhatsky
0 siblings, 0 replies; 38+ messages in thread
From: Sergey Senozhatsky @ 2021-05-16 12:55 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Leon Romanovsky, linux-kernel, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On (21/05/16 14:07), Takashi Iwai wrote:
> > > For fixing the crash, this patch adds a internal flag indicating that
> > > the stream is ready to be updated, and check it (as well as the flag
> > > being in suspended) to ignore such spurious update.
> >
> > I reproduced the "spurious IRQ" case, and the patch handled it correctly
> > (VM did not crash).
> >
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >
> > I'll keep running test, but seems that it works as intended
> >
> > Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
>
> OK, below is the revised patch I'm going to apply.
>
Sounds good.
> Thanks!
Thank you.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-05-16 9:49 ` Takashi Iwai
@ 2021-07-06 17:50 ` Max Filippov
-1 siblings, 0 replies; 38+ messages in thread
From: Max Filippov @ 2021-07-06 17:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: Sergey Senozhatsky, alsa-devel, Leon Romanovsky, Takashi Iwai,
LKML, Gustavo A. R. Silva
Hello,
On Sun, May 16, 2021 at 2:50 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Sun, 16 May 2021 10:31:41 +0200,
> Sergey Senozhatsky wrote:
> >
> > On (21/05/16 17:30), Sergey Senozhatsky wrote:
> > > On (21/05/14 20:16), Sergey Senozhatsky wrote:
> > > > > --- a/sound/pci/intel8x0.c
> > > > > +++ b/sound/pci/intel8x0.c
> > > > > @@ -691,6 +691,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > > > > int status, civ, i, step;
> > > > > int ack = 0;
> > > > >
> > > > > + if (!ichdev->substream || ichdev->suspended)
> > > > > + return;
> > > > > +
> > > > > spin_lock_irqsave(&chip->reg_lock, flags);
> > > > > status = igetbyte(chip, port + ichdev->roff_sr);
> > > > > civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
> > >
> > > This does the problem for me.
> >
> > ^^^ does fix
>
> OK, thanks for confirmation. So this looks like some spurious
> interrupt with the unexpected hardware bits.
>
> However, the suggested check doesn't seem covering enough, and it
> might still hit if the suspend/resume happens before the device is
> opened but not set up (and such a spurious irq is triggered).
>
> Below is more comprehensive fix. Let me know if this works, too.
>
>
> thanks,
>
> Takashi
>
> -- 8< --
> Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
>
> The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> the hardware sets the corresponding status bit for each stream. This
> works fine for most cases as long as the hardware behaves properly.
> But when the hardware gives a wrong bit set, this leads to a NULL
> dereference Oops, and reportedly, this seems what happened on a VM.
>
> For fixing the crash, this patch adds a internal flag indicating that
> the stream is ready to be updated, and check it (as well as the flag
> being in suspended) to ignore such spurious update.
>
> Cc: <stable@vger.kernel.org>
> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> sound/pci/intel8x0.c | 7 +++++++
> 1 file changed, 7 insertions(+)
linux v5.13 booting on qemu-system-xtensa virt board gets stuck inside
snd_intel8x0_probe -> intel8x0_measure_ac97_clock with this patch.
Prior to it it boots successfully for me.
I'm curious if this issue has been reported yet.
What I see is an IRQ flood, at some point snd_intel8x0_interrupt
and timer ISR are called in loop and execution never returns to
the interrupted function intel8x0_measure_ac97_clock.
Any idea what it could be?
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-07-06 17:50 ` Max Filippov
0 siblings, 0 replies; 38+ messages in thread
From: Max Filippov @ 2021-07-06 17:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Leon Romanovsky, LKML, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
Hello,
On Sun, May 16, 2021 at 2:50 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Sun, 16 May 2021 10:31:41 +0200,
> Sergey Senozhatsky wrote:
> >
> > On (21/05/16 17:30), Sergey Senozhatsky wrote:
> > > On (21/05/14 20:16), Sergey Senozhatsky wrote:
> > > > > --- a/sound/pci/intel8x0.c
> > > > > +++ b/sound/pci/intel8x0.c
> > > > > @@ -691,6 +691,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > > > > int status, civ, i, step;
> > > > > int ack = 0;
> > > > >
> > > > > + if (!ichdev->substream || ichdev->suspended)
> > > > > + return;
> > > > > +
> > > > > spin_lock_irqsave(&chip->reg_lock, flags);
> > > > > status = igetbyte(chip, port + ichdev->roff_sr);
> > > > > civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
> > >
> > > This does the problem for me.
> >
> > ^^^ does fix
>
> OK, thanks for confirmation. So this looks like some spurious
> interrupt with the unexpected hardware bits.
>
> However, the suggested check doesn't seem covering enough, and it
> might still hit if the suspend/resume happens before the device is
> opened but not set up (and such a spurious irq is triggered).
>
> Below is more comprehensive fix. Let me know if this works, too.
>
>
> thanks,
>
> Takashi
>
> -- 8< --
> Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
>
> The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> the hardware sets the corresponding status bit for each stream. This
> works fine for most cases as long as the hardware behaves properly.
> But when the hardware gives a wrong bit set, this leads to a NULL
> dereference Oops, and reportedly, this seems what happened on a VM.
>
> For fixing the crash, this patch adds a internal flag indicating that
> the stream is ready to be updated, and check it (as well as the flag
> being in suspended) to ignore such spurious update.
>
> Cc: <stable@vger.kernel.org>
> Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> sound/pci/intel8x0.c | 7 +++++++
> 1 file changed, 7 insertions(+)
linux v5.13 booting on qemu-system-xtensa virt board gets stuck inside
snd_intel8x0_probe -> intel8x0_measure_ac97_clock with this patch.
Prior to it it boots successfully for me.
I'm curious if this issue has been reported yet.
What I see is an IRQ flood, at some point snd_intel8x0_interrupt
and timer ISR are called in loop and execution never returns to
the interrupted function intel8x0_measure_ac97_clock.
Any idea what it could be?
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-07-06 17:50 ` Max Filippov
@ 2021-07-07 7:02 ` Takashi Iwai
-1 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-07-07 7:02 UTC (permalink / raw)
To: Max Filippov
Cc: Sergey Senozhatsky, alsa-devel, Leon Romanovsky, Takashi Iwai,
LKML, Gustavo A. R. Silva
On Tue, 06 Jul 2021 19:50:08 +0200,
Max Filippov wrote:
>
> Hello,
>
> On Sun, May 16, 2021 at 2:50 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Sun, 16 May 2021 10:31:41 +0200,
> > Sergey Senozhatsky wrote:
> > >
> > > On (21/05/16 17:30), Sergey Senozhatsky wrote:
> > > > On (21/05/14 20:16), Sergey Senozhatsky wrote:
> > > > > > --- a/sound/pci/intel8x0.c
> > > > > > +++ b/sound/pci/intel8x0.c
> > > > > > @@ -691,6 +691,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > > > > > int status, civ, i, step;
> > > > > > int ack = 0;
> > > > > >
> > > > > > + if (!ichdev->substream || ichdev->suspended)
> > > > > > + return;
> > > > > > +
> > > > > > spin_lock_irqsave(&chip->reg_lock, flags);
> > > > > > status = igetbyte(chip, port + ichdev->roff_sr);
> > > > > > civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
> > > >
> > > > This does the problem for me.
> > >
> > > ^^^ does fix
> >
> > OK, thanks for confirmation. So this looks like some spurious
> > interrupt with the unexpected hardware bits.
> >
> > However, the suggested check doesn't seem covering enough, and it
> > might still hit if the suspend/resume happens before the device is
> > opened but not set up (and such a spurious irq is triggered).
> >
> > Below is more comprehensive fix. Let me know if this works, too.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > -- 8< --
> > Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
> >
> > The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> > the hardware sets the corresponding status bit for each stream. This
> > works fine for most cases as long as the hardware behaves properly.
> > But when the hardware gives a wrong bit set, this leads to a NULL
> > dereference Oops, and reportedly, this seems what happened on a VM.
> >
> > For fixing the crash, this patch adds a internal flag indicating that
> > the stream is ready to be updated, and check it (as well as the flag
> > being in suspended) to ignore such spurious update.
> >
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > sound/pci/intel8x0.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
>
> linux v5.13 booting on qemu-system-xtensa virt board gets stuck inside
> snd_intel8x0_probe -> intel8x0_measure_ac97_clock with this patch.
> Prior to it it boots successfully for me.
> I'm curious if this issue has been reported yet.
>
> What I see is an IRQ flood, at some point snd_intel8x0_interrupt
> and timer ISR are called in loop and execution never returns to
> the interrupted function intel8x0_measure_ac97_clock.
>
> Any idea what it could be?
That's something odd with the VM. As the chip itself has never shown
such a problem on real systems, maybe the best action would be to just
skip the clock measurement on VM. The measurement itself is
unreliable on VM, so it makes more sense.
That said, something like below would work?
thanks,
Takashi
---
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 2d1bfbcba933..b75f832d7777 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -2199,6 +2199,9 @@ static int snd_intel8x0_mixer(struct intel8x0 *chip, int ac97_clock,
pbus->private_free = snd_intel8x0_mixer_free_ac97_bus;
if (ac97_clock >= 8000 && ac97_clock <= 48000)
pbus->clock = ac97_clock;
+ else if (chip->inside_vm)
+ pbus->clock = 48000;
+
/* FIXME: my test board doesn't work well with VRA... */
if (chip->device_type == DEVICE_ALI)
pbus->no_vra = 1;
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-07-07 7:02 ` Takashi Iwai
0 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-07-07 7:02 UTC (permalink / raw)
To: Max Filippov
Cc: alsa-devel, Leon Romanovsky, LKML, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On Tue, 06 Jul 2021 19:50:08 +0200,
Max Filippov wrote:
>
> Hello,
>
> On Sun, May 16, 2021 at 2:50 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Sun, 16 May 2021 10:31:41 +0200,
> > Sergey Senozhatsky wrote:
> > >
> > > On (21/05/16 17:30), Sergey Senozhatsky wrote:
> > > > On (21/05/14 20:16), Sergey Senozhatsky wrote:
> > > > > > --- a/sound/pci/intel8x0.c
> > > > > > +++ b/sound/pci/intel8x0.c
> > > > > > @@ -691,6 +691,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > > > > > int status, civ, i, step;
> > > > > > int ack = 0;
> > > > > >
> > > > > > + if (!ichdev->substream || ichdev->suspended)
> > > > > > + return;
> > > > > > +
> > > > > > spin_lock_irqsave(&chip->reg_lock, flags);
> > > > > > status = igetbyte(chip, port + ichdev->roff_sr);
> > > > > > civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
> > > >
> > > > This does the problem for me.
> > >
> > > ^^^ does fix
> >
> > OK, thanks for confirmation. So this looks like some spurious
> > interrupt with the unexpected hardware bits.
> >
> > However, the suggested check doesn't seem covering enough, and it
> > might still hit if the suspend/resume happens before the device is
> > opened but not set up (and such a spurious irq is triggered).
> >
> > Below is more comprehensive fix. Let me know if this works, too.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > -- 8< --
> > Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
> >
> > The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever
> > the hardware sets the corresponding status bit for each stream. This
> > works fine for most cases as long as the hardware behaves properly.
> > But when the hardware gives a wrong bit set, this leads to a NULL
> > dereference Oops, and reportedly, this seems what happened on a VM.
> >
> > For fixing the crash, this patch adds a internal flag indicating that
> > the stream is ready to be updated, and check it (as well as the flag
> > being in suspended) to ignore such spurious update.
> >
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > sound/pci/intel8x0.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
>
> linux v5.13 booting on qemu-system-xtensa virt board gets stuck inside
> snd_intel8x0_probe -> intel8x0_measure_ac97_clock with this patch.
> Prior to it it boots successfully for me.
> I'm curious if this issue has been reported yet.
>
> What I see is an IRQ flood, at some point snd_intel8x0_interrupt
> and timer ISR are called in loop and execution never returns to
> the interrupted function intel8x0_measure_ac97_clock.
>
> Any idea what it could be?
That's something odd with the VM. As the chip itself has never shown
such a problem on real systems, maybe the best action would be to just
skip the clock measurement on VM. The measurement itself is
unreliable on VM, so it makes more sense.
That said, something like below would work?
thanks,
Takashi
---
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 2d1bfbcba933..b75f832d7777 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -2199,6 +2199,9 @@ static int snd_intel8x0_mixer(struct intel8x0 *chip, int ac97_clock,
pbus->private_free = snd_intel8x0_mixer_free_ac97_bus;
if (ac97_clock >= 8000 && ac97_clock <= 48000)
pbus->clock = ac97_clock;
+ else if (chip->inside_vm)
+ pbus->clock = 48000;
+
/* FIXME: my test board doesn't work well with VRA... */
if (chip->device_type == DEVICE_ALI)
pbus->no_vra = 1;
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-07-07 7:02 ` Takashi Iwai
@ 2021-07-07 17:50 ` Max Filippov
-1 siblings, 0 replies; 38+ messages in thread
From: Max Filippov @ 2021-07-07 17:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: Sergey Senozhatsky, alsa-devel, Leon Romanovsky, Takashi Iwai,
LKML, Gustavo A. R. Silva
On Wed, Jul 7, 2021 at 12:02 AM Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 06 Jul 2021 19:50:08 +0200, Max Filippov wrote:
> > linux v5.13 booting on qemu-system-xtensa virt board gets stuck inside
> > snd_intel8x0_probe -> intel8x0_measure_ac97_clock with this patch.
> > Prior to it it boots successfully for me.
> > I'm curious if this issue has been reported yet.
> >
> > What I see is an IRQ flood, at some point snd_intel8x0_interrupt
> > and timer ISR are called in loop and execution never returns to
> > the interrupted function intel8x0_measure_ac97_clock.
> >
> > Any idea what it could be?
>
> That's something odd with the VM. As the chip itself has never shown
> such a problem on real systems, maybe the best action would be to just
> skip the clock measurement on VM. The measurement itself is
> unreliable on VM, so it makes more sense.
>
> That said, something like below would work?
It didn't change anything in my case. My further observation is that
the snd_intel8x0_update is called before the ichdev->prepared
is set to one and as a result IRQ is apparently never cleared.
Perhaps because intel8x0_measure_ac97_clock is called from the
snd_intel8x0_probe, well before the snd_intel8x0_pcm_prepare
that sets ichdev->prepared is called.
> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> index 2d1bfbcba933..b75f832d7777 100644
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
> @@ -2199,6 +2199,9 @@ static int snd_intel8x0_mixer(struct intel8x0 *chip, int ac97_clock,
> pbus->private_free = snd_intel8x0_mixer_free_ac97_bus;
> if (ac97_clock >= 8000 && ac97_clock <= 48000)
> pbus->clock = ac97_clock;
> + else if (chip->inside_vm)
> + pbus->clock = 48000;
> +
> /* FIXME: my test board doesn't work well with VRA... */
> if (chip->device_type == DEVICE_ALI)
> pbus->no_vra = 1;
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-07-07 17:50 ` Max Filippov
0 siblings, 0 replies; 38+ messages in thread
From: Max Filippov @ 2021-07-07 17:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Leon Romanovsky, LKML, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On Wed, Jul 7, 2021 at 12:02 AM Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 06 Jul 2021 19:50:08 +0200, Max Filippov wrote:
> > linux v5.13 booting on qemu-system-xtensa virt board gets stuck inside
> > snd_intel8x0_probe -> intel8x0_measure_ac97_clock with this patch.
> > Prior to it it boots successfully for me.
> > I'm curious if this issue has been reported yet.
> >
> > What I see is an IRQ flood, at some point snd_intel8x0_interrupt
> > and timer ISR are called in loop and execution never returns to
> > the interrupted function intel8x0_measure_ac97_clock.
> >
> > Any idea what it could be?
>
> That's something odd with the VM. As the chip itself has never shown
> such a problem on real systems, maybe the best action would be to just
> skip the clock measurement on VM. The measurement itself is
> unreliable on VM, so it makes more sense.
>
> That said, something like below would work?
It didn't change anything in my case. My further observation is that
the snd_intel8x0_update is called before the ichdev->prepared
is set to one and as a result IRQ is apparently never cleared.
Perhaps because intel8x0_measure_ac97_clock is called from the
snd_intel8x0_probe, well before the snd_intel8x0_pcm_prepare
that sets ichdev->prepared is called.
> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> index 2d1bfbcba933..b75f832d7777 100644
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
> @@ -2199,6 +2199,9 @@ static int snd_intel8x0_mixer(struct intel8x0 *chip, int ac97_clock,
> pbus->private_free = snd_intel8x0_mixer_free_ac97_bus;
> if (ac97_clock >= 8000 && ac97_clock <= 48000)
> pbus->clock = ac97_clock;
> + else if (chip->inside_vm)
> + pbus->clock = 48000;
> +
> /* FIXME: my test board doesn't work well with VRA... */
> if (chip->device_type == DEVICE_ALI)
> pbus->no_vra = 1;
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-07-07 17:50 ` Max Filippov
@ 2021-07-07 18:14 ` Takashi Iwai
-1 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-07-07 18:14 UTC (permalink / raw)
To: Max Filippov
Cc: Sergey Senozhatsky, alsa-devel, Leon Romanovsky, Takashi Iwai,
LKML, Gustavo A. R. Silva
On Wed, 07 Jul 2021 19:50:07 +0200,
Max Filippov wrote:
>
> On Wed, Jul 7, 2021 at 12:02 AM Takashi Iwai <tiwai@suse.de> wrote:
> > On Tue, 06 Jul 2021 19:50:08 +0200, Max Filippov wrote:
> > > linux v5.13 booting on qemu-system-xtensa virt board gets stuck inside
> > > snd_intel8x0_probe -> intel8x0_measure_ac97_clock with this patch.
> > > Prior to it it boots successfully for me.
> > > I'm curious if this issue has been reported yet.
> > >
> > > What I see is an IRQ flood, at some point snd_intel8x0_interrupt
> > > and timer ISR are called in loop and execution never returns to
> > > the interrupted function intel8x0_measure_ac97_clock.
> > >
> > > Any idea what it could be?
> >
> > That's something odd with the VM. As the chip itself has never shown
> > such a problem on real systems, maybe the best action would be to just
> > skip the clock measurement on VM. The measurement itself is
> > unreliable on VM, so it makes more sense.
> >
> > That said, something like below would work?
>
> It didn't change anything in my case. My further observation is that
> the snd_intel8x0_update is called before the ichdev->prepared
> is set to one and as a result IRQ is apparently never cleared.
So it's broken in anyway no matter whether
intel8x0_measure_ac97_clock() is called or not, right?
I'm afraid that something is wrong in VM, then. The driver has been
working over decades on thousands of real different boards.
Skipping the clock measurement on VM would be still useful,
independent from your problem, though.
Takashi
> Perhaps because intel8x0_measure_ac97_clock is called from the
> snd_intel8x0_probe, well before the snd_intel8x0_pcm_prepare
> that sets ichdev->prepared is called.
>
> > thanks,
> >
> > Takashi
> >
> > ---
> > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> > index 2d1bfbcba933..b75f832d7777 100644
> > --- a/sound/pci/intel8x0.c
> > +++ b/sound/pci/intel8x0.c
> > @@ -2199,6 +2199,9 @@ static int snd_intel8x0_mixer(struct intel8x0 *chip, int ac97_clock,
> > pbus->private_free = snd_intel8x0_mixer_free_ac97_bus;
> > if (ac97_clock >= 8000 && ac97_clock <= 48000)
> > pbus->clock = ac97_clock;
> > + else if (chip->inside_vm)
> > + pbus->clock = 48000;
> > +
> > /* FIXME: my test board doesn't work well with VRA... */
> > if (chip->device_type == DEVICE_ALI)
> > pbus->no_vra = 1;
>
> --
> Thanks.
> -- Max
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-07-07 18:14 ` Takashi Iwai
0 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-07-07 18:14 UTC (permalink / raw)
To: Max Filippov
Cc: alsa-devel, Leon Romanovsky, LKML, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On Wed, 07 Jul 2021 19:50:07 +0200,
Max Filippov wrote:
>
> On Wed, Jul 7, 2021 at 12:02 AM Takashi Iwai <tiwai@suse.de> wrote:
> > On Tue, 06 Jul 2021 19:50:08 +0200, Max Filippov wrote:
> > > linux v5.13 booting on qemu-system-xtensa virt board gets stuck inside
> > > snd_intel8x0_probe -> intel8x0_measure_ac97_clock with this patch.
> > > Prior to it it boots successfully for me.
> > > I'm curious if this issue has been reported yet.
> > >
> > > What I see is an IRQ flood, at some point snd_intel8x0_interrupt
> > > and timer ISR are called in loop and execution never returns to
> > > the interrupted function intel8x0_measure_ac97_clock.
> > >
> > > Any idea what it could be?
> >
> > That's something odd with the VM. As the chip itself has never shown
> > such a problem on real systems, maybe the best action would be to just
> > skip the clock measurement on VM. The measurement itself is
> > unreliable on VM, so it makes more sense.
> >
> > That said, something like below would work?
>
> It didn't change anything in my case. My further observation is that
> the snd_intel8x0_update is called before the ichdev->prepared
> is set to one and as a result IRQ is apparently never cleared.
So it's broken in anyway no matter whether
intel8x0_measure_ac97_clock() is called or not, right?
I'm afraid that something is wrong in VM, then. The driver has been
working over decades on thousands of real different boards.
Skipping the clock measurement on VM would be still useful,
independent from your problem, though.
Takashi
> Perhaps because intel8x0_measure_ac97_clock is called from the
> snd_intel8x0_probe, well before the snd_intel8x0_pcm_prepare
> that sets ichdev->prepared is called.
>
> > thanks,
> >
> > Takashi
> >
> > ---
> > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> > index 2d1bfbcba933..b75f832d7777 100644
> > --- a/sound/pci/intel8x0.c
> > +++ b/sound/pci/intel8x0.c
> > @@ -2199,6 +2199,9 @@ static int snd_intel8x0_mixer(struct intel8x0 *chip, int ac97_clock,
> > pbus->private_free = snd_intel8x0_mixer_free_ac97_bus;
> > if (ac97_clock >= 8000 && ac97_clock <= 48000)
> > pbus->clock = ac97_clock;
> > + else if (chip->inside_vm)
> > + pbus->clock = 48000;
> > +
> > /* FIXME: my test board doesn't work well with VRA... */
> > if (chip->device_type == DEVICE_ALI)
> > pbus->no_vra = 1;
>
> --
> Thanks.
> -- Max
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-07-07 18:14 ` Takashi Iwai
@ 2021-07-07 20:33 ` Max Filippov
-1 siblings, 0 replies; 38+ messages in thread
From: Max Filippov @ 2021-07-07 20:33 UTC (permalink / raw)
To: Takashi Iwai
Cc: Sergey Senozhatsky, alsa-devel, Leon Romanovsky, Takashi Iwai,
LKML, Gustavo A. R. Silva
On Wed, Jul 7, 2021 at 11:14 AM Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 07 Jul 2021 19:50:07 +0200, Max Filippov wrote:
> > It didn't change anything in my case. My further observation is that
> > the snd_intel8x0_update is called before the ichdev->prepared
> > is set to one and as a result IRQ is apparently never cleared.
>
> So it's broken in anyway no matter whether
> intel8x0_measure_ac97_clock() is called or not, right?
The change that you suggested didn't eliminate the call to
intel8x0_measure_ac97_clock, it's still called and an interrupt
flood happens at the same place.
I've also tried the following change instead and it fixes my issue:
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 5b124c4ad572..13d1c9edea10 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -692,11 +692,14 @@ static inline void snd_intel8x0_update(struct
intel8x0 *chip, struct ichdev *ich
int status, civ, i, step;
int ack = 0;
- if (!ichdev->prepared || ichdev->suspended)
- return;
-
spin_lock_irqsave(&chip->reg_lock, flags);
status = igetbyte(chip, port + ichdev->roff_sr);
+ if (!ichdev->prepared || ichdev->suspended) {
+ spin_unlock_irqrestore(&chip->reg_lock, flags);
+ iputbyte(chip, port + ichdev->roff_sr,
+ status & (ICH_FIFOE | ICH_BCIS | ICH_LVBCI));
+ return;
+ }
civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
if (!(status & ICH_BCIS)) {
step = 0;
> I'm afraid that something is wrong in VM, then. The driver has been
> working over decades on thousands of real different boards.
>
> Skipping the clock measurement on VM would be still useful,
> independent from your problem, though.
--
Thanks.
-- Max
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-07-07 20:33 ` Max Filippov
0 siblings, 0 replies; 38+ messages in thread
From: Max Filippov @ 2021-07-07 20:33 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Leon Romanovsky, LKML, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On Wed, Jul 7, 2021 at 11:14 AM Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 07 Jul 2021 19:50:07 +0200, Max Filippov wrote:
> > It didn't change anything in my case. My further observation is that
> > the snd_intel8x0_update is called before the ichdev->prepared
> > is set to one and as a result IRQ is apparently never cleared.
>
> So it's broken in anyway no matter whether
> intel8x0_measure_ac97_clock() is called or not, right?
The change that you suggested didn't eliminate the call to
intel8x0_measure_ac97_clock, it's still called and an interrupt
flood happens at the same place.
I've also tried the following change instead and it fixes my issue:
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 5b124c4ad572..13d1c9edea10 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -692,11 +692,14 @@ static inline void snd_intel8x0_update(struct
intel8x0 *chip, struct ichdev *ich
int status, civ, i, step;
int ack = 0;
- if (!ichdev->prepared || ichdev->suspended)
- return;
-
spin_lock_irqsave(&chip->reg_lock, flags);
status = igetbyte(chip, port + ichdev->roff_sr);
+ if (!ichdev->prepared || ichdev->suspended) {
+ spin_unlock_irqrestore(&chip->reg_lock, flags);
+ iputbyte(chip, port + ichdev->roff_sr,
+ status & (ICH_FIFOE | ICH_BCIS | ICH_LVBCI));
+ return;
+ }
civ = igetbyte(chip, port + ICH_REG_OFF_CIV);
if (!(status & ICH_BCIS)) {
step = 0;
> I'm afraid that something is wrong in VM, then. The driver has been
> working over decades on thousands of real different boards.
>
> Skipping the clock measurement on VM would be still useful,
> independent from your problem, though.
--
Thanks.
-- Max
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-07-07 20:33 ` Max Filippov
@ 2021-07-08 7:13 ` Takashi Iwai
-1 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-07-08 7:13 UTC (permalink / raw)
To: Max Filippov
Cc: Sergey Senozhatsky, alsa-devel, Leon Romanovsky, Takashi Iwai,
LKML, Gustavo A. R. Silva
On Wed, 07 Jul 2021 22:33:22 +0200,
Max Filippov wrote:
>
> On Wed, Jul 7, 2021 at 11:14 AM Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 07 Jul 2021 19:50:07 +0200, Max Filippov wrote:
> > > It didn't change anything in my case. My further observation is that
> > > the snd_intel8x0_update is called before the ichdev->prepared
> > > is set to one and as a result IRQ is apparently never cleared.
> >
> > So it's broken in anyway no matter whether
> > intel8x0_measure_ac97_clock() is called or not, right?
>
> The change that you suggested didn't eliminate the call to
> intel8x0_measure_ac97_clock, it's still called and an interrupt
> flood happens at the same place.
Ah I see the point. Then the fix would be a oneliner like below.
Takashi
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -694,7 +694,7 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
int status, civ, i, step;
int ack = 0;
- if (!ichdev->prepared || ichdev->suspended)
+ if (!(ichdev->prepared || ichdev->in_measurement) || ichdev->suspended)
return;
spin_lock_irqsave(&chip->reg_lock, flags);
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-07-08 7:13 ` Takashi Iwai
0 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-07-08 7:13 UTC (permalink / raw)
To: Max Filippov
Cc: alsa-devel, Leon Romanovsky, LKML, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On Wed, 07 Jul 2021 22:33:22 +0200,
Max Filippov wrote:
>
> On Wed, Jul 7, 2021 at 11:14 AM Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 07 Jul 2021 19:50:07 +0200, Max Filippov wrote:
> > > It didn't change anything in my case. My further observation is that
> > > the snd_intel8x0_update is called before the ichdev->prepared
> > > is set to one and as a result IRQ is apparently never cleared.
> >
> > So it's broken in anyway no matter whether
> > intel8x0_measure_ac97_clock() is called or not, right?
>
> The change that you suggested didn't eliminate the call to
> intel8x0_measure_ac97_clock, it's still called and an interrupt
> flood happens at the same place.
Ah I see the point. Then the fix would be a oneliner like below.
Takashi
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -694,7 +694,7 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
int status, civ, i, step;
int ack = 0;
- if (!ichdev->prepared || ichdev->suspended)
+ if (!(ichdev->prepared || ichdev->in_measurement) || ichdev->suspended)
return;
spin_lock_irqsave(&chip->reg_lock, flags);
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-07-08 7:13 ` Takashi Iwai
@ 2021-07-08 8:41 ` Max Filippov
-1 siblings, 0 replies; 38+ messages in thread
From: Max Filippov @ 2021-07-08 8:41 UTC (permalink / raw)
To: Takashi Iwai
Cc: Sergey Senozhatsky, alsa-devel, Leon Romanovsky, Takashi Iwai,
LKML, Gustavo A. R. Silva
On Thu, Jul 8, 2021 at 12:13 AM Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 07 Jul 2021 22:33:22 +0200,
> Max Filippov wrote:
> >
> > On Wed, Jul 7, 2021 at 11:14 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > On Wed, 07 Jul 2021 19:50:07 +0200, Max Filippov wrote:
> > > > It didn't change anything in my case. My further observation is that
> > > > the snd_intel8x0_update is called before the ichdev->prepared
> > > > is set to one and as a result IRQ is apparently never cleared.
> > >
> > > So it's broken in anyway no matter whether
> > > intel8x0_measure_ac97_clock() is called or not, right?
> >
> > The change that you suggested didn't eliminate the call to
> > intel8x0_measure_ac97_clock, it's still called and an interrupt
> > flood happens at the same place.
>
> Ah I see the point. Then the fix would be a oneliner like below.
>
>
> Takashi
>
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
> @@ -694,7 +694,7 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> int status, civ, i, step;
> int ack = 0;
>
> - if (!ichdev->prepared || ichdev->suspended)
> + if (!(ichdev->prepared || ichdev->in_measurement) || ichdev->suspended)
There's no ichdev::in_measurement, but if replaced with
chip->in_measurement it indeed fixes my issue.
So with this change:
Tested-by: Max Filippov <jcmvbkbc@gmail.com>
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-07-08 8:41 ` Max Filippov
0 siblings, 0 replies; 38+ messages in thread
From: Max Filippov @ 2021-07-08 8:41 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Leon Romanovsky, LKML, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On Thu, Jul 8, 2021 at 12:13 AM Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 07 Jul 2021 22:33:22 +0200,
> Max Filippov wrote:
> >
> > On Wed, Jul 7, 2021 at 11:14 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > On Wed, 07 Jul 2021 19:50:07 +0200, Max Filippov wrote:
> > > > It didn't change anything in my case. My further observation is that
> > > > the snd_intel8x0_update is called before the ichdev->prepared
> > > > is set to one and as a result IRQ is apparently never cleared.
> > >
> > > So it's broken in anyway no matter whether
> > > intel8x0_measure_ac97_clock() is called or not, right?
> >
> > The change that you suggested didn't eliminate the call to
> > intel8x0_measure_ac97_clock, it's still called and an interrupt
> > flood happens at the same place.
>
> Ah I see the point. Then the fix would be a oneliner like below.
>
>
> Takashi
>
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
> @@ -694,7 +694,7 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> int status, civ, i, step;
> int ack = 0;
>
> - if (!ichdev->prepared || ichdev->suspended)
> + if (!(ichdev->prepared || ichdev->in_measurement) || ichdev->suspended)
There's no ichdev::in_measurement, but if replaced with
chip->in_measurement it indeed fixes my issue.
So with this change:
Tested-by: Max Filippov <jcmvbkbc@gmail.com>
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-07-08 8:41 ` Max Filippov
@ 2021-07-08 9:00 ` Takashi Iwai
-1 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-07-08 9:00 UTC (permalink / raw)
To: Max Filippov
Cc: Sergey Senozhatsky, alsa-devel, Leon Romanovsky, Takashi Iwai,
LKML, Gustavo A. R. Silva
On Thu, 08 Jul 2021 10:41:50 +0200,
Max Filippov wrote:
>
> On Thu, Jul 8, 2021 at 12:13 AM Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 07 Jul 2021 22:33:22 +0200,
> > Max Filippov wrote:
> > >
> > > On Wed, Jul 7, 2021 at 11:14 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > > On Wed, 07 Jul 2021 19:50:07 +0200, Max Filippov wrote:
> > > > > It didn't change anything in my case. My further observation is that
> > > > > the snd_intel8x0_update is called before the ichdev->prepared
> > > > > is set to one and as a result IRQ is apparently never cleared.
> > > >
> > > > So it's broken in anyway no matter whether
> > > > intel8x0_measure_ac97_clock() is called or not, right?
> > >
> > > The change that you suggested didn't eliminate the call to
> > > intel8x0_measure_ac97_clock, it's still called and an interrupt
> > > flood happens at the same place.
> >
> > Ah I see the point. Then the fix would be a oneliner like below.
> >
> >
> > Takashi
> >
> > --- a/sound/pci/intel8x0.c
> > +++ b/sound/pci/intel8x0.c
> > @@ -694,7 +694,7 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > int status, civ, i, step;
> > int ack = 0;
> >
> > - if (!ichdev->prepared || ichdev->suspended)
> > + if (!(ichdev->prepared || ichdev->in_measurement) || ichdev->suspended)
>
> There's no ichdev::in_measurement, but if replaced with
> chip->in_measurement it indeed fixes my issue.
One must compile the code before sending out :-<
> So with this change:
> Tested-by: Max Filippov <jcmvbkbc@gmail.com>
Great, thanks for quick testing, I'll prepare the fix patch now.
Takashi
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-07-08 9:00 ` Takashi Iwai
0 siblings, 0 replies; 38+ messages in thread
From: Takashi Iwai @ 2021-07-08 9:00 UTC (permalink / raw)
To: Max Filippov
Cc: alsa-devel, Leon Romanovsky, LKML, Gustavo A. R. Silva,
Takashi Iwai, Sergey Senozhatsky
On Thu, 08 Jul 2021 10:41:50 +0200,
Max Filippov wrote:
>
> On Thu, Jul 8, 2021 at 12:13 AM Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 07 Jul 2021 22:33:22 +0200,
> > Max Filippov wrote:
> > >
> > > On Wed, Jul 7, 2021 at 11:14 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > > On Wed, 07 Jul 2021 19:50:07 +0200, Max Filippov wrote:
> > > > > It didn't change anything in my case. My further observation is that
> > > > > the snd_intel8x0_update is called before the ichdev->prepared
> > > > > is set to one and as a result IRQ is apparently never cleared.
> > > >
> > > > So it's broken in anyway no matter whether
> > > > intel8x0_measure_ac97_clock() is called or not, right?
> > >
> > > The change that you suggested didn't eliminate the call to
> > > intel8x0_measure_ac97_clock, it's still called and an interrupt
> > > flood happens at the same place.
> >
> > Ah I see the point. Then the fix would be a oneliner like below.
> >
> >
> > Takashi
> >
> > --- a/sound/pci/intel8x0.c
> > +++ b/sound/pci/intel8x0.c
> > @@ -694,7 +694,7 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > int status, civ, i, step;
> > int ack = 0;
> >
> > - if (!ichdev->prepared || ichdev->suspended)
> > + if (!(ichdev->prepared || ichdev->in_measurement) || ichdev->suspended)
>
> There's no ichdev::in_measurement, but if replaced with
> chip->in_measurement it indeed fixes my issue.
One must compile the code before sending out :-<
> So with this change:
> Tested-by: Max Filippov <jcmvbkbc@gmail.com>
Great, thanks for quick testing, I'll prepare the fix patch now.
Takashi
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
2021-07-08 9:00 ` Takashi Iwai
@ 2021-07-08 10:12 ` Sergey Senozhatsky
-1 siblings, 0 replies; 38+ messages in thread
From: Sergey Senozhatsky @ 2021-07-08 10:12 UTC (permalink / raw)
To: Takashi Iwai
Cc: Max Filippov, Sergey Senozhatsky, alsa-devel, Leon Romanovsky,
Takashi Iwai, LKML, Gustavo A. R. Silva
On (21/07/08 11:00), Takashi Iwai wrote:
> > > --- a/sound/pci/intel8x0.c
> > > +++ b/sound/pci/intel8x0.c
> > > @@ -694,7 +694,7 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > > int status, civ, i, step;
> > > int ack = 0;
> > >
> > > - if (!ichdev->prepared || ichdev->suspended)
> > > + if (!(ichdev->prepared || ichdev->in_measurement) || ichdev->suspended)
> >
> > There's no ichdev::in_measurement, but if replaced with
> > chip->in_measurement it indeed fixes my issue.
>
> One must compile the code before sending out :-<
>
> > So with this change:
> > Tested-by: Max Filippov <jcmvbkbc@gmail.com>
>
> Great, thanks for quick testing, I'll prepare the fix patch now.
Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: ALSA: intel8x0: div by zero in snd_intel8x0_update()
@ 2021-07-08 10:12 ` Sergey Senozhatsky
0 siblings, 0 replies; 38+ messages in thread
From: Sergey Senozhatsky @ 2021-07-08 10:12 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Gustavo A. R. Silva, Leon Romanovsky,
Sergey Senozhatsky, LKML, Takashi Iwai, Max Filippov
On (21/07/08 11:00), Takashi Iwai wrote:
> > > --- a/sound/pci/intel8x0.c
> > > +++ b/sound/pci/intel8x0.c
> > > @@ -694,7 +694,7 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich
> > > int status, civ, i, step;
> > > int ack = 0;
> > >
> > > - if (!ichdev->prepared || ichdev->suspended)
> > > + if (!(ichdev->prepared || ichdev->in_measurement) || ichdev->suspended)
> >
> > There's no ichdev::in_measurement, but if replaced with
> > chip->in_measurement it indeed fixes my issue.
>
> One must compile the code before sending out :-<
>
> > So with this change:
> > Tested-by: Max Filippov <jcmvbkbc@gmail.com>
>
> Great, thanks for quick testing, I'll prepare the fix patch now.
Tested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
^ permalink raw reply [flat|nested] 38+ messages in thread