[PATCH 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[PATCH 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Yu Zhao]

This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.

The patch doesn't fix accessing memory with null pointer in
skl_interrupt().

There are two problems: 1) skl_init_chip() is called twice, before
and after dma buffer is allocate. The first call sets bus->chip_init
which prevents the second from initializing bus->corb.buf and
rirb.buf from bus->rb.area. 2) snd_hdac_bus_init_chip() enables
interrupt before snd_hdac_bus_init_cmd_io() initializing dma buffers.
There is a small window which skl_interrupt() can be called if irq
has been acquired. If so, it crashes when using null dma buffer
pointers.

Will fix the problems in the following patches. Also attaching the
crash for future reference.

[   16.949148] general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI
[   16.950829] gsmi: Log Shutdown Reason 0x03
[   16.950830] Modules linked in: uvcvideo(+) videobuf2_vmalloc snd_soc_skl(+) videobuf2_memops videobuf2_v4l2 videobuf2_core snd_soc_skl_ipc snd_soc_sst_ipc snd_soc_sst_dsp snd_soc_acpi snd_hda_ext_core snd_hda_core snd_soc_max98357a acpi_als snd_soc_da7219 lzo lzo_compress zram snd_seq_dummy snd_seq snd_seq_device bridge stp llc ipt_MASQUERADE nf_nat_masquerade_ipv4 xt_mark fuse cfg80211 iio_trig_sysfs cros_ec_sensors cros_ec_sensors_ring cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf industrialio smsc95xx usbnet mii joydev
[   16.950874] CPU: 0 PID: 1083 Comm: chrome Not tainted 4.14.64 #14
[   16.950875] Hardware name: Google Yorp/Yorp, BIOS Google_Yorp.10985.0.2018_08_20_1648 08/17/2018
[   16.950878] task: ffff88015b1c2b80 task.stack: ffff880155f30000
[   16.950887] RIP: 0010:snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core]
[   16.950889] RSP: 0000:ffff88015c807c08 EFLAGS: 00010003
[   16.950891] RAX: 0000000000000101 RBX: 000000000000080c RCX: 1ffff10026822185
[   16.950893] RDX: dffffc0000000000 RSI: ffff88015b1c2b80 RDI: ffffc90000514058
[   16.950894] RBP: ffff88015c807c68 R08: 0000000000000000 R09: 0000000000000000
[   16.950895] R10: 0000000000000000 R11: ffffffffc043074f R12: 0000000000000800
[   16.950897] R13: 0000000000000001 R14: 0000000000000002 R15: 1ffff10026822119
[   16.950899] FS:  00007d85924cc740(0000) GS:ffff88015c800000(0000) knlGS:0000000000000000
[   16.950900] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   16.950902] CR2: 000058a54df16038 CR3: 00000001551c0000 CR4: 00000000003406f0
[   16.950903] Call Trace:
[   16.950906]  <IRQ>
[   16.950918]  skl_interrupt+0x19e/0x2d6 [snd_soc_skl]
[   16.950926]  ? dma_supported+0xb5/0xb5 [snd_soc_skl]
[   16.950933]  __handle_irq_event_percpu+0x27a/0x6c8
[   16.950937]  ? __irq_wake_thread+0x1d1/0x1d1
[   16.950942]  ? __do_softirq+0x57a/0x69e
[   16.950944]  handle_irq_event_percpu+0x95/0x1ba
[   16.950948]  ? _raw_spin_unlock+0x65/0xdc
[   16.950951]  ? __handle_irq_event_percpu+0x6c8/0x6c8
[   16.950953]  ? _raw_spin_unlock+0x65/0xdc
[   16.950957]  ? time_cpufreq_notifier+0x483/0x483
[   16.950959]  handle_irq_event+0x89/0x123
[   16.950962]  handle_fasteoi_irq+0x16f/0x425
[   16.950965]  handle_irq+0x1fe/0x28e
[   16.950969]  do_IRQ+0x6e/0x12e
[   16.950972]  common_interrupt+0x7a/0x7a
[   16.950974]  </IRQ>
[   16.950976] RIP: 0033:0x58097f61a5c0
[   16.950978] RSP: 002b:00007ffe95c971a8 EFLAGS: 00000206 ORIG_RAX: ffffffffffffffbc
[   16.950980] RAX: 000058097f61a5c0 RBX: 00000e4ac5220560 RCX: 0000000000004e10
[   16.950982] RDX: 000058098563df20 RSI: 00007ffe95c97250 RDI: 00000e4ac5220500
[   16.950983] RBP: 00007ffe95c97410 R08: 0000000000000000 R09: 00007ffe95c97250
[   16.950984] R10: 0000000000000000 R11: 0000000000000000 R12: 00000e4ac5220560
[   16.950986] R13: 00000e4ac5220560 R14: 00000e4ac47c9650 R15: 0000580987646350
[   16.950988] Code: 74 12 48 89 df e8 eb 2d 8e cd 48 ba 00 00 00 00 00 fc ff df 4c 8b 23 44 89 f0 83 c8 01 0f b7 c0 49 8d 1c 84 48 89 d8 48 c1 e8 03 <8a> 04 10 84 c0 0f 85 da 01 00 00 44 8b 3b 41 0f b7 c6 49 8d 1c
[   16.951031] RIP: snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core] RSP: ffff88015c807c08
[   16.951036] ---[ end trace 58bf9ece1775bc92 ]---
[   16.956871] Kernel panic - not syncing: Fatal exception in interrupt
[   16.956888] Kernel Offset: 0xc800000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 sound/soc/intel/skylake/skl.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index cf09721ca13e..dce649485649 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -838,7 +838,11 @@ static int skl_first_init(struct hdac_bus *bus)
 
 	snd_hdac_bus_parse_capabilities(bus);
 
+	if (skl_acquire_irq(bus, 0) < 0)
+		return -EBUSY;
+
 	pci_set_master(pci);
+	synchronize_irq(bus->irq);
 
 	gcap = snd_hdac_chip_readw(bus, GCAP);
 	dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap);
@@ -871,12 +875,6 @@ static int skl_first_init(struct hdac_bus *bus)
 	if (err < 0)
 		return err;
 
-	err = skl_acquire_irq(bus, 0);
-	if (err < 0)
-		return err;
-
-	synchronize_irq(bus->irq);
-
 	/* initialize chip */
 	skl_init_pci(skl);
 
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

[PATCH 2/3] sound: enable interrupt after dma buffer initialization

[Yu Zhao]

In snd_hdac_bus_init_chip(), we enable interrupt before
snd_hdac_bus_init_cmd_io() initializing dma buffers. If irq has
been acquired and irq handler uses the dma buffer, kernel may crash
when interrupt comes in.

Fix the problem by postponing enabling irq after dma buffer
initialization. And warn once on null dma buffer pointer during the
initialization.

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 sound/hda/hdac_controller.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 560ec0986e1a..11057d9f84ec 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -40,6 +40,8 @@ static void azx_clear_corbrp(struct hdac_bus *bus)
  */
 void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus)
 {
+	WARN_ON_ONCE(!bus->rb.area);
+
 	spin_lock_irq(&bus->reg_lock);
 	/* CORB set up */
 	bus->corb.addr = bus->rb.addr;
@@ -479,13 +481,15 @@ bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset)
 	/* reset controller */
 	azx_reset(bus, full_reset);
 
-	/* initialize interrupts */
+	/* clear interrupts */
 	azx_int_clear(bus);
-	azx_int_enable(bus);
 
 	/* initialize the codec command I/O */
 	snd_hdac_bus_init_cmd_io(bus);
 
+	/* enable interrupts after CORB/RIRB buffers are initialized above */
+	azx_int_enable(bus);
+
 	/* program the position buffer */
 	if (bus->use_posbuf && bus->posbuf.addr) {
 		snd_hdac_chip_writel(bus, DPLBASE, (u32)bus->posbuf.addr);
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

[PATCH 3/3] sound: don't call skl_init_chip() to reset intel skl soc

[Yu Zhao]

Internally, skl_init_chip() calls snd_hdac_bus_init_chip() which
1) sets bus->chip_init to prevent multiple entrances before device
is stopped; 2) enables interrupt.

We shouldn't use it for the purpose of resetting device only because
1) when we really want to initialize device, we won't be able to do
so; 2) we are ready to handle interrupt yet, and kernel crashes when
interrupt comes in.

Rename azx_reset() to snd_hdac_bus_reset_link(), and use it to reset
device properly.

Fixes: 60767abcea3d ("ASoC: Intel: Skylake: Reset the controller in probe")
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 include/sound/hdaudio.h       | 1 +
 sound/hda/hdac_controller.c   | 7 ++++---
 sound/soc/intel/skylake/skl.c | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 6f1e1f3b3063..cd1773d0e08f 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -412,6 +412,7 @@ void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus);
 void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus);
 void snd_hdac_bus_enter_link_reset(struct hdac_bus *bus);
 void snd_hdac_bus_exit_link_reset(struct hdac_bus *bus);
+int snd_hdac_bus_reset_link(struct hdac_bus *bus, bool full_reset);
 
 void snd_hdac_bus_update_rirb(struct hdac_bus *bus);
 int snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned int status,
diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 11057d9f84ec..74244d8e2909 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -385,7 +385,7 @@ void snd_hdac_bus_exit_link_reset(struct hdac_bus *bus)
 EXPORT_SYMBOL_GPL(snd_hdac_bus_exit_link_reset);
 
 /* reset codec link */
-static int azx_reset(struct hdac_bus *bus, bool full_reset)
+int snd_hdac_bus_reset_link(struct hdac_bus *bus, bool full_reset)
 {
 	if (!full_reset)
 		goto skip_reset;
@@ -410,7 +410,7 @@ static int azx_reset(struct hdac_bus *bus, bool full_reset)
  skip_reset:
 	/* check to see if controller is ready */
 	if (!snd_hdac_chip_readb(bus, GCTL)) {
-		dev_dbg(bus->dev, "azx_reset: controller not ready!\n");
+		dev_dbg(bus->dev, "controller not ready!\n");
 		return -EBUSY;
 	}
 
@@ -425,6 +425,7 @@ static int azx_reset(struct hdac_bus *bus, bool full_reset)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(snd_hdac_bus_reset_link);
 
 /* enable interrupts */
 static void azx_int_enable(struct hdac_bus *bus)
@@ -479,7 +480,7 @@ bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset)
 		return false;
 
 	/* reset controller */
-	azx_reset(bus, full_reset);
+	snd_hdac_bus_reset_link(bus, full_reset);
 
 	/* clear interrupts */
 	azx_int_clear(bus);
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index dce649485649..1d17be0f78a0 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -834,7 +834,7 @@ static int skl_first_init(struct hdac_bus *bus)
 		return -ENXIO;
 	}
 
-	skl_init_chip(bus, true);
+	snd_hdac_bus_reset_link(bus, true);
 
 	snd_hdac_bus_parse_capabilities(bus);
 
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Takashi Iwai]

On Mon, 10 Sep 2018 23:17:18 +0200,
Yu Zhao wrote:
> 
> This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.
> 
> The patch doesn't fix accessing memory with null pointer in
> skl_interrupt().
> 
> There are two problems: 1) skl_init_chip() is called twice, before
> and after dma buffer is allocate. The first call sets bus->chip_init
> which prevents the second from initializing bus->corb.buf and
> rirb.buf from bus->rb.area. 2) snd_hdac_bus_init_chip() enables
> interrupt before snd_hdac_bus_init_cmd_io() initializing dma buffers.
> There is a small window which skl_interrupt() can be called if irq
> has been acquired. If so, it crashes when using null dma buffer
> pointers.
> 
> Will fix the problems in the following patches. Also attaching the
> crash for future reference.
> 
> [   16.949148] general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI
> [   16.950829] gsmi: Log Shutdown Reason 0x03
> [   16.950830] Modules linked in: uvcvideo(+) videobuf2_vmalloc snd_soc_skl(+) videobuf2_memops videobuf2_v4l2 videobuf2_core snd_soc_skl_ipc snd_soc_sst_ipc snd_soc_sst_dsp snd_soc_acpi snd_hda_ext_core snd_hda_core snd_soc_max98357a acpi_als snd_soc_da7219 lzo lzo_compress zram snd_seq_dummy snd_seq snd_seq_device bridge stp llc ipt_MASQUERADE nf_nat_masquerade_ipv4 xt_mark fuse cfg80211 iio_trig_sysfs cros_ec_sensors cros_ec_sensors_ring cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf industrialio smsc95xx usbnet mii joydev
> [   16.950874] CPU: 0 PID: 1083 Comm: chrome Not tainted 4.14.64 #14
> [   16.950875] Hardware name: Google Yorp/Yorp, BIOS Google_Yorp.10985.0.2018_08_20_1648 08/17/2018
> [   16.950878] task: ffff88015b1c2b80 task.stack: ffff880155f30000
> [   16.950887] RIP: 0010:snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core]
> [   16.950889] RSP: 0000:ffff88015c807c08 EFLAGS: 00010003
> [   16.950891] RAX: 0000000000000101 RBX: 000000000000080c RCX: 1ffff10026822185
> [   16.950893] RDX: dffffc0000000000 RSI: ffff88015b1c2b80 RDI: ffffc90000514058
> [   16.950894] RBP: ffff88015c807c68 R08: 0000000000000000 R09: 0000000000000000
> [   16.950895] R10: 0000000000000000 R11: ffffffffc043074f R12: 0000000000000800
> [   16.950897] R13: 0000000000000001 R14: 0000000000000002 R15: 1ffff10026822119
> [   16.950899] FS:  00007d85924cc740(0000) GS:ffff88015c800000(0000) knlGS:0000000000000000
> [   16.950900] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   16.950902] CR2: 000058a54df16038 CR3: 00000001551c0000 CR4: 00000000003406f0
> [   16.950903] Call Trace:
> [   16.950906]  <IRQ>
> [   16.950918]  skl_interrupt+0x19e/0x2d6 [snd_soc_skl]
> [   16.950926]  ? dma_supported+0xb5/0xb5 [snd_soc_skl]
> [   16.950933]  __handle_irq_event_percpu+0x27a/0x6c8
> [   16.950937]  ? __irq_wake_thread+0x1d1/0x1d1
> [   16.950942]  ? __do_softirq+0x57a/0x69e
> [   16.950944]  handle_irq_event_percpu+0x95/0x1ba
> [   16.950948]  ? _raw_spin_unlock+0x65/0xdc
> [   16.950951]  ? __handle_irq_event_percpu+0x6c8/0x6c8
> [   16.950953]  ? _raw_spin_unlock+0x65/0xdc
> [   16.950957]  ? time_cpufreq_notifier+0x483/0x483
> [   16.950959]  handle_irq_event+0x89/0x123
> [   16.950962]  handle_fasteoi_irq+0x16f/0x425
> [   16.950965]  handle_irq+0x1fe/0x28e
> [   16.950969]  do_IRQ+0x6e/0x12e
> [   16.950972]  common_interrupt+0x7a/0x7a
> [   16.950974]  </IRQ>
> [   16.950976] RIP: 0033:0x58097f61a5c0
> [   16.950978] RSP: 002b:00007ffe95c971a8 EFLAGS: 00000206 ORIG_RAX: ffffffffffffffbc
> [   16.950980] RAX: 000058097f61a5c0 RBX: 00000e4ac5220560 RCX: 0000000000004e10
> [   16.950982] RDX: 000058098563df20 RSI: 00007ffe95c97250 RDI: 00000e4ac5220500
> [   16.950983] RBP: 00007ffe95c97410 R08: 0000000000000000 R09: 00007ffe95c97250
> [   16.950984] R10: 0000000000000000 R11: 0000000000000000 R12: 00000e4ac5220560
> [   16.950986] R13: 00000e4ac5220560 R14: 00000e4ac47c9650 R15: 0000580987646350
> [   16.950988] Code: 74 12 48 89 df e8 eb 2d 8e cd 48 ba 00 00 00 00 00 fc ff df 4c 8b 23 44 89 f0 83 c8 01 0f b7 c0 49 8d 1c 84 48 89 d8 48 c1 e8 03 <8a> 04 10 84 c0 0f 85 da 01 00 00 44 8b 3b 41 0f b7 c6 49 8d 1c
> [   16.951031] RIP: snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core] RSP: ffff88015c807c08
> [   16.951036] ---[ end trace 58bf9ece1775bc92 ]---
> [   16.956871] Kernel panic - not syncing: Fatal exception in interrupt
> [   16.956888] Kernel Offset: 0xc800000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)

Please try to rip off uninteresting hex values and other stuff there.

> Signed-off-by: Yu Zhao <yuzhao@google.com>

Put Fixes tag.


thanks,

Takashi

Re: [PATCH 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Takashi Iwai]

On Mon, 10 Sep 2018 23:17:18 +0200,
Yu Zhao wrote:
> 
> This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.
> 
> The patch doesn't fix accessing memory with null pointer in
> skl_interrupt().
> 
> There are two problems: 1) skl_init_chip() is called twice, before
> and after dma buffer is allocate. The first call sets bus->chip_init
> which prevents the second from initializing bus->corb.buf and
> rirb.buf from bus->rb.area. 2) snd_hdac_bus_init_chip() enables
> interrupt before snd_hdac_bus_init_cmd_io() initializing dma buffers.
> There is a small window which skl_interrupt() can be called if irq
> has been acquired. If so, it crashes when using null dma buffer
> pointers.
> 
> Will fix the problems in the following patches. Also attaching the
> crash for future reference.
> 
> [   16.949148] general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI
> [   16.950829] gsmi: Log Shutdown Reason 0x03
> [   16.950830] Modules linked in: uvcvideo(+) videobuf2_vmalloc snd_soc_skl(+) videobuf2_memops videobuf2_v4l2 videobuf2_core snd_soc_skl_ipc snd_soc_sst_ipc snd_soc_sst_dsp snd_soc_acpi snd_hda_ext_core snd_hda_core snd_soc_max98357a acpi_als snd_soc_da7219 lzo lzo_compress zram snd_seq_dummy snd_seq snd_seq_device bridge stp llc ipt_MASQUERADE nf_nat_masquerade_ipv4 xt_mark fuse cfg80211 iio_trig_sysfs cros_ec_sensors cros_ec_sensors_ring cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf industrialio smsc95xx usbnet mii joydev
> [   16.950874] CPU: 0 PID: 1083 Comm: chrome Not tainted 4.14.64 #14
> [   16.950875] Hardware name: Google Yorp/Yorp, BIOS Google_Yorp.10985.0.2018_08_20_1648 08/17/2018
> [   16.950878] task: ffff88015b1c2b80 task.stack: ffff880155f30000
> [   16.950887] RIP: 0010:snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core]
> [   16.950889] RSP: 0000:ffff88015c807c08 EFLAGS: 00010003
> [   16.950891] RAX: 0000000000000101 RBX: 000000000000080c RCX: 1ffff10026822185
> [   16.950893] RDX: dffffc0000000000 RSI: ffff88015b1c2b80 RDI: ffffc90000514058
> [   16.950894] RBP: ffff88015c807c68 R08: 0000000000000000 R09: 0000000000000000
> [   16.950895] R10: 0000000000000000 R11: ffffffffc043074f R12: 0000000000000800
> [   16.950897] R13: 0000000000000001 R14: 0000000000000002 R15: 1ffff10026822119
> [   16.950899] FS:  00007d85924cc740(0000) GS:ffff88015c800000(0000) knlGS:0000000000000000
> [   16.950900] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   16.950902] CR2: 000058a54df16038 CR3: 00000001551c0000 CR4: 00000000003406f0
> [   16.950903] Call Trace:
> [   16.950906]  <IRQ>
> [   16.950918]  skl_interrupt+0x19e/0x2d6 [snd_soc_skl]
> [   16.950926]  ? dma_supported+0xb5/0xb5 [snd_soc_skl]
> [   16.950933]  __handle_irq_event_percpu+0x27a/0x6c8
> [   16.950937]  ? __irq_wake_thread+0x1d1/0x1d1
> [   16.950942]  ? __do_softirq+0x57a/0x69e
> [   16.950944]  handle_irq_event_percpu+0x95/0x1ba
> [   16.950948]  ? _raw_spin_unlock+0x65/0xdc
> [   16.950951]  ? __handle_irq_event_percpu+0x6c8/0x6c8
> [   16.950953]  ? _raw_spin_unlock+0x65/0xdc
> [   16.950957]  ? time_cpufreq_notifier+0x483/0x483
> [   16.950959]  handle_irq_event+0x89/0x123
> [   16.950962]  handle_fasteoi_irq+0x16f/0x425
> [   16.950965]  handle_irq+0x1fe/0x28e
> [   16.950969]  do_IRQ+0x6e/0x12e
> [   16.950972]  common_interrupt+0x7a/0x7a
> [   16.950974]  </IRQ>
> [   16.950976] RIP: 0033:0x58097f61a5c0
> [   16.950978] RSP: 002b:00007ffe95c971a8 EFLAGS: 00000206 ORIG_RAX: ffffffffffffffbc
> [   16.950980] RAX: 000058097f61a5c0 RBX: 00000e4ac5220560 RCX: 0000000000004e10
> [   16.950982] RDX: 000058098563df20 RSI: 00007ffe95c97250 RDI: 00000e4ac5220500
> [   16.950983] RBP: 00007ffe95c97410 R08: 0000000000000000 R09: 00007ffe95c97250
> [   16.950984] R10: 0000000000000000 R11: 0000000000000000 R12: 00000e4ac5220560
> [   16.950986] R13: 00000e4ac5220560 R14: 00000e4ac47c9650 R15: 0000580987646350
> [   16.950988] Code: 74 12 48 89 df e8 eb 2d 8e cd 48 ba 00 00 00 00 00 fc ff df 4c 8b 23 44 89 f0 83 c8 01 0f b7 c0 49 8d 1c 84 48 89 d8 48 c1 e8 03 <8a> 04 10 84 c0 0f 85 da 01 00 00 44 8b 3b 41 0f b7 c6 49 8d 1c
> [   16.951031] RIP: snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core] RSP: ffff88015c807c08
> [   16.951036] ---[ end trace 58bf9ece1775bc92 ]---
> [   16.956871] Kernel panic - not syncing: Fatal exception in interrupt
> [   16.956888] Kernel Offset: 0xc800000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)

Please try to rip off uninteresting hex values and other stuff there.

> Signed-off-by: Yu Zhao <yuzhao@google.com>

Put Fixes tag.


thanks,

Takashi

Re: [PATCH 2/3] sound: enable interrupt after dma buffer initialization

[Takashi Iwai]

On Mon, 10 Sep 2018 23:21:50 +0200,
Yu Zhao wrote:
> 
> In snd_hdac_bus_init_chip(), we enable interrupt before
> snd_hdac_bus_init_cmd_io() initializing dma buffers. If irq has
> been acquired and irq handler uses the dma buffer, kernel may crash
> when interrupt comes in.
> 
> Fix the problem by postponing enabling irq after dma buffer
> initialization. And warn once on null dma buffer pointer during the
> initialization.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>

Looks good to me.

Reviewed-by: Takashi Iwai <tiwai@suse.de>


BTW, the reason why this hasn't been hit on the legacy HD-audio driver
is that we allocate usually with MSI, so the irq is isolated.

Any reason that Intel SKL driver doesn't use MST?


thanks,

Takashi

Re: [PATCH 2/3] sound: enable interrupt after dma buffer initialization

[Takashi Iwai]

On Mon, 10 Sep 2018 23:21:50 +0200,
Yu Zhao wrote:
> 
> In snd_hdac_bus_init_chip(), we enable interrupt before
> snd_hdac_bus_init_cmd_io() initializing dma buffers. If irq has
> been acquired and irq handler uses the dma buffer, kernel may crash
> when interrupt comes in.
> 
> Fix the problem by postponing enabling irq after dma buffer
> initialization. And warn once on null dma buffer pointer during the
> initialization.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>

Looks good to me.

Reviewed-by: Takashi Iwai <tiwai@suse.de>


BTW, the reason why this hasn't been hit on the legacy HD-audio driver
is that we allocate usually with MSI, so the irq is isolated.

Any reason that Intel SKL driver doesn't use MST?


thanks,

Takashi

Re: [PATCH 3/3] sound: don't call skl_init_chip() to reset intel skl soc

[Takashi Iwai]

On Mon, 10 Sep 2018 23:23:58 +0200,
Yu Zhao wrote:
> 
> Internally, skl_init_chip() calls snd_hdac_bus_init_chip() which
> 1) sets bus->chip_init to prevent multiple entrances before device
> is stopped; 2) enables interrupt.
> 
> We shouldn't use it for the purpose of resetting device only because
> 1) when we really want to initialize device, we won't be able to do
> so; 2) we are ready to handle interrupt yet, and kernel crashes when
> interrupt comes in.
> 
> Rename azx_reset() to snd_hdac_bus_reset_link(), and use it to reset
> device properly.
> 
> Fixes: 60767abcea3d ("ASoC: Intel: Skylake: Reset the controller in probe")
> Signed-off-by: Yu Zhao <yuzhao@google.com>

That makes sense.

And I noticed that the legacy HD-audio driver potentially needs the
same reset (although we haven't heard of any problem for years).

So now I wonder whether this requirement of reset is really mandatory
for the real hardware, or just theoretical...

In anyway,

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH 3/3] sound: don't call skl_init_chip() to reset intel skl soc

[Takashi Iwai]

On Mon, 10 Sep 2018 23:23:58 +0200,
Yu Zhao wrote:
> 
> Internally, skl_init_chip() calls snd_hdac_bus_init_chip() which
> 1) sets bus->chip_init to prevent multiple entrances before device
> is stopped; 2) enables interrupt.
> 
> We shouldn't use it for the purpose of resetting device only because
> 1) when we really want to initialize device, we won't be able to do
> so; 2) we are ready to handle interrupt yet, and kernel crashes when
> interrupt comes in.
> 
> Rename azx_reset() to snd_hdac_bus_reset_link(), and use it to reset
> device properly.
> 
> Fixes: 60767abcea3d ("ASoC: Intel: Skylake: Reset the controller in probe")
> Signed-off-by: Yu Zhao <yuzhao@google.com>

That makes sense.

And I noticed that the legacy HD-audio driver potentially needs the
same reset (although we haven't heard of any problem for years).

So now I wonder whether this requirement of reset is really mandatory
for the real hardware, or just theoretical...

In anyway,

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

On Tue, Sep 11, 2018 at 08:03:21AM +0200, Takashi Iwai wrote:
> Yu Zhao wrote:

> > Will fix the problems in the following patches. Also attaching the
> > crash for future reference.

> > [   16.950887] RIP: 0010:snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core]
> > [   16.950889] RSP: 0000:ffff88015c807c08 EFLAGS: 00010003
> > [   16.950891] RAX: 0000000000000101 RBX: 000000000000080c RCX: 1ffff10026822185

> Please try to rip off uninteresting hex values and other stuff there.

Right:

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative then it's
usually better to pull out the relevant sections.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 820 bytes --]

On Tue, Sep 11, 2018 at 08:03:21AM +0200, Takashi Iwai wrote:
> Yu Zhao wrote:

> > Will fix the problems in the following patches. Also attaching the
> > crash for future reference.

> > [   16.950887] RIP: 0010:snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core]
> > [   16.950889] RSP: 0000:ffff88015c807c08 EFLAGS: 00010003
> > [   16.950891] RAX: 0000000000000101 RBX: 000000000000080c RCX: 1ffff10026822185

> Please try to rip off uninteresting hex values and other stuff there.

Right:

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative then it's
usually better to pull out the relevant sections.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Yu Zhao]

On Tue, Sep 11, 2018 at 05:36:36PM +0100, Mark Brown wrote:
> On Tue, Sep 11, 2018 at 08:03:21AM +0200, Takashi Iwai wrote:
> > Yu Zhao wrote:
> 
> > > Will fix the problems in the following patches. Also attaching the
> > > crash for future reference.
> 
> > > [   16.950887] RIP: 0010:snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core]
> > > [   16.950889] RSP: 0000:ffff88015c807c08 EFLAGS: 00010003
> > > [   16.950891] RAX: 0000000000000101 RBX: 000000000000080c RCX: 1ffff10026822185
> 
> > Please try to rip off uninteresting hex values and other stuff there.
> 
> Right:
> 
> Please think hard before including complete backtraces in upstream
> reports, they are very large and contain almost no useful information
> relative to their size so often obscure the relevant content in your
> message. If part of the backtrace is usefully illustrative then it's
> usually better to pull out the relevant sections.

Thanks, will do in v2.

Re: [PATCH 2/3] sound: enable interrupt after dma buffer initialization

[Yu Zhao]

On Tue, Sep 11, 2018 at 08:06:49AM +0200, Takashi Iwai wrote:
> On Mon, 10 Sep 2018 23:21:50 +0200,
> Yu Zhao wrote:
> > 
> > In snd_hdac_bus_init_chip(), we enable interrupt before
> > snd_hdac_bus_init_cmd_io() initializing dma buffers. If irq has
> > been acquired and irq handler uses the dma buffer, kernel may crash
> > when interrupt comes in.
> > 
> > Fix the problem by postponing enabling irq after dma buffer
> > initialization. And warn once on null dma buffer pointer during the
> > initialization.
> > 
> > Signed-off-by: Yu Zhao <yuzhao@google.com>
> 
> Looks good to me.
> 
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
> 
> 
> BTW, the reason why this hasn't been hit on the legacy HD-audio driver
> is that we allocate usually with MSI, so the irq is isolated.
> 
> Any reason that Intel SKL driver doesn't use MST?

This I'm not sure. Vinod might have answer to it, according to
https://patchwork.kernel.org/patch/6375831/#13796611

[PATCH v2 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Yu Zhao]

This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.

The patch doesn't fix accessing memory with null pointer in
skl_interrupt().

There are two problems: 1) skl_init_chip() is called twice, before
and after dma buffer is allocate. The first call sets bus->chip_init
which prevents the second from initializing bus->corb.buf and
rirb.buf from bus->rb.area. 2) snd_hdac_bus_init_chip() enables
interrupt before snd_hdac_bus_init_cmd_io() initializing dma buffers.
There is a small window which skl_interrupt() can be called if irq
has been acquired. If so, it crashes when using null dma buffer
pointers.

Will fix the problems in the following patches. Also attaching the
crash for future reference.

[   16.949148] general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI
<snipped>
[   16.950903] Call Trace:
[   16.950906]  <IRQ>
[   16.950918]  skl_interrupt+0x19e/0x2d6 [snd_soc_skl]
[   16.950926]  ? dma_supported+0xb5/0xb5 [snd_soc_skl]
[   16.950933]  __handle_irq_event_percpu+0x27a/0x6c8
[   16.950937]  ? __irq_wake_thread+0x1d1/0x1d1
[   16.950942]  ? __do_softirq+0x57a/0x69e
[   16.950944]  handle_irq_event_percpu+0x95/0x1ba
[   16.950948]  ? _raw_spin_unlock+0x65/0xdc
[   16.950951]  ? __handle_irq_event_percpu+0x6c8/0x6c8
[   16.950953]  ? _raw_spin_unlock+0x65/0xdc
[   16.950957]  ? time_cpufreq_notifier+0x483/0x483
[   16.950959]  handle_irq_event+0x89/0x123
[   16.950962]  handle_fasteoi_irq+0x16f/0x425
[   16.950965]  handle_irq+0x1fe/0x28e
[   16.950969]  do_IRQ+0x6e/0x12e
[   16.950972]  common_interrupt+0x7a/0x7a
[   16.950974]  </IRQ>
<snipped>
[   16.951031] RIP: snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core] RSP: ffff88015c807c08
[   16.951036] ---[ end trace 58bf9ece1775bc92 ]---

Fixes: 2eeeb4f4733b ("ASoC: Intel: Skylake: Acquire irq after RIRB allocation")
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 sound/soc/intel/skylake/skl.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index e7fd14daeb4f..d174cbe35f7a 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -912,7 +912,11 @@ static int skl_first_init(struct hdac_bus *bus)
 
 	snd_hdac_bus_parse_capabilities(bus);
 
+	if (skl_acquire_irq(bus, 0) < 0)
+		return -EBUSY;
+
 	pci_set_master(pci);
+	synchronize_irq(bus->irq);
 
 	gcap = snd_hdac_chip_readw(bus, GCAP);
 	dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap);
@@ -945,12 +949,6 @@ static int skl_first_init(struct hdac_bus *bus)
 	if (err < 0)
 		return err;
 
-	err = skl_acquire_irq(bus, 0);
-	if (err < 0)
-		return err;
-
-	synchronize_irq(bus->irq);
-
 	/* initialize chip */
 	skl_init_pci(skl);
 
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

[PATCH v2 2/3] sound: enable interrupt after dma buffer initialization

[Yu Zhao]

In snd_hdac_bus_init_chip(), we enable interrupt before
snd_hdac_bus_init_cmd_io() initializing dma buffers. If irq has
been acquired and irq handler uses the dma buffer, kernel may crash
when interrupt comes in.

Fix the problem by postponing enabling irq after dma buffer
initialization. And warn once on null dma buffer pointer during the
initialization.

Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 sound/hda/hdac_controller.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 560ec0986e1a..11057d9f84ec 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -40,6 +40,8 @@ static void azx_clear_corbrp(struct hdac_bus *bus)
  */
 void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus)
 {
+	WARN_ON_ONCE(!bus->rb.area);
+
 	spin_lock_irq(&bus->reg_lock);
 	/* CORB set up */
 	bus->corb.addr = bus->rb.addr;
@@ -479,13 +481,15 @@ bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset)
 	/* reset controller */
 	azx_reset(bus, full_reset);
 
-	/* initialize interrupts */
+	/* clear interrupts */
 	azx_int_clear(bus);
-	azx_int_enable(bus);
 
 	/* initialize the codec command I/O */
 	snd_hdac_bus_init_cmd_io(bus);
 
+	/* enable interrupts after CORB/RIRB buffers are initialized above */
+	azx_int_enable(bus);
+
 	/* program the position buffer */
 	if (bus->use_posbuf && bus->posbuf.addr) {
 		snd_hdac_chip_writel(bus, DPLBASE, (u32)bus->posbuf.addr);
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

[PATCH v2 3/3] sound: don't call skl_init_chip() to reset intel skl soc

[Yu Zhao]

Internally, skl_init_chip() calls snd_hdac_bus_init_chip() which
1) sets bus->chip_init to prevent multiple entrances before device
is stopped; 2) enables interrupt.

We shouldn't use it for the purpose of resetting device only because
1) when we really want to initialize device, we won't be able to do
so; 2) we are ready to handle interrupt yet, and kernel crashes when
interrupt comes in.

Rename azx_reset() to snd_hdac_bus_reset_link(), and use it to reset
device properly.

Fixes: 60767abcea3d ("ASoC: Intel: Skylake: Reset the controller in probe")
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 include/sound/hdaudio.h       | 1 +
 sound/hda/hdac_controller.c   | 7 ++++---
 sound/soc/intel/skylake/skl.c | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 6f1e1f3b3063..cd1773d0e08f 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -412,6 +412,7 @@ void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus);
 void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus);
 void snd_hdac_bus_enter_link_reset(struct hdac_bus *bus);
 void snd_hdac_bus_exit_link_reset(struct hdac_bus *bus);
+int snd_hdac_bus_reset_link(struct hdac_bus *bus, bool full_reset);
 
 void snd_hdac_bus_update_rirb(struct hdac_bus *bus);
 int snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned int status,
diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 11057d9f84ec..74244d8e2909 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -385,7 +385,7 @@ void snd_hdac_bus_exit_link_reset(struct hdac_bus *bus)
 EXPORT_SYMBOL_GPL(snd_hdac_bus_exit_link_reset);
 
 /* reset codec link */
-static int azx_reset(struct hdac_bus *bus, bool full_reset)
+int snd_hdac_bus_reset_link(struct hdac_bus *bus, bool full_reset)
 {
 	if (!full_reset)
 		goto skip_reset;
@@ -410,7 +410,7 @@ static int azx_reset(struct hdac_bus *bus, bool full_reset)
  skip_reset:
 	/* check to see if controller is ready */
 	if (!snd_hdac_chip_readb(bus, GCTL)) {
-		dev_dbg(bus->dev, "azx_reset: controller not ready!\n");
+		dev_dbg(bus->dev, "controller not ready!\n");
 		return -EBUSY;
 	}
 
@@ -425,6 +425,7 @@ static int azx_reset(struct hdac_bus *bus, bool full_reset)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(snd_hdac_bus_reset_link);
 
 /* enable interrupts */
 static void azx_int_enable(struct hdac_bus *bus)
@@ -479,7 +480,7 @@ bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset)
 		return false;
 
 	/* reset controller */
-	azx_reset(bus, full_reset);
+	snd_hdac_bus_reset_link(bus, full_reset);
 
 	/* clear interrupts */
 	azx_int_clear(bus);
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index d174cbe35f7a..29225623b4b4 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -908,7 +908,7 @@ static int skl_first_init(struct hdac_bus *bus)
 		return -ENXIO;
 	}
 
-	skl_init_chip(bus, true);
+	snd_hdac_bus_reset_link(bus, true);
 
 	snd_hdac_bus_parse_capabilities(bus);
 
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH 2/3] sound: enable interrupt after dma buffer initialization

[Vinod]

On 11-09-18, 14:58, Yu Zhao wrote:
> On Tue, Sep 11, 2018 at 08:06:49AM +0200, Takashi Iwai wrote:
> > On Mon, 10 Sep 2018 23:21:50 +0200,
> > Yu Zhao wrote:
> > > 
> > > In snd_hdac_bus_init_chip(), we enable interrupt before
> > > snd_hdac_bus_init_cmd_io() initializing dma buffers. If irq has
> > > been acquired and irq handler uses the dma buffer, kernel may crash
> > > when interrupt comes in.
> > > 
> > > Fix the problem by postponing enabling irq after dma buffer
> > > initialization. And warn once on null dma buffer pointer during the
> > > initialization.
> > > 
> > > Signed-off-by: Yu Zhao <yuzhao@google.com>
> > 
> > Looks good to me.
> > 
> > Reviewed-by: Takashi Iwai <tiwai@suse.de>
> > 
> > 
> > BTW, the reason why this hasn't been hit on the legacy HD-audio driver
> > is that we allocate usually with MSI, so the irq is isolated.
> > 
> > Any reason that Intel SKL driver doesn't use MST?
> 
> This I'm not sure. Vinod might have answer to it, according to
> https://patchwork.kernel.org/patch/6375831/#13796611

IIRC (seemed quite some time back) we faced issues with using MSI on SKL
and didnt try afterwards. If Intel folks can try it and check. Pierre is
out, maybe Liam can help..?

-- 
~Vinod

Re: [PATCH v2 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]

On Tue, Sep 11, 2018 at 03:12:46PM -0600, Yu Zhao wrote:
> This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.
> 
> The patch doesn't fix accessing memory with null pointer in
> skl_interrupt().

Please use normal subject lines and so on even for revert patches -
they're patches just the same as any other.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 326 bytes --]

On Tue, Sep 11, 2018 at 03:12:46PM -0600, Yu Zhao wrote:
> This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.
> 
> The patch doesn't fix accessing memory with null pointer in
> skl_interrupt().

Please use normal subject lines and so on even for revert patches -
they're patches just the same as any other.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Yu Zhao]

On Wed, Sep 12, 2018 at 11:20:20AM +0100, Mark Brown wrote:
> On Tue, Sep 11, 2018 at 03:12:46PM -0600, Yu Zhao wrote:
> > This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.
> > 
> > The patch doesn't fix accessing memory with null pointer in
> > skl_interrupt().
> 
> Please use normal subject lines and so on even for revert patches -
> they're patches just the same as any other.

Well, they seem pretty normal to me, and have been used by many:

  commit 82ab86e82911107ead6fc7cd73568f75bc266a57
  Author: Mark Brown <broonie@kernel.org>
  Date:   Mon Oct 30 19:40:25 2017 +0000
  
      Revert "ASoC: rt5651: Enable jack detection on JD* pins"
      
      This reverts commit 60d5a1a47b9a8381c08d2263b11ac9c757c87746.
      
      Signed-off-by: Mark Brown <broonie@kernel.org>

Anyway, I'll update them in v3.

[PATCH v3 1/3] ASoC: Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Yu Zhao]

This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.

The patch claims it fixes accessing memory with null pointer on
skl_interrupt() and snd_hdac_bus_update_rirb() path, but in fact it
has no effect.

There are two problems: 1) skl_init_chip() is called twice, before
and after dma buffer is allocate. The first call sets bus->chip_init
which prevents the second from initializing bus->corb.buf and
rirb.buf from bus->rb.area. 2) snd_hdac_bus_init_chip() enables
interrupt before snd_hdac_bus_init_cmd_io() initializing dma buffers.
There is a small window which skl_interrupt() can be called if irq
has been acquired. If so, it crashes when using null dma buffer
pointers.

Will fix the problems in the following patches. Also attaching the
crash for future reference.

[   16.949148] general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI
<snipped>
[   16.950903] Call Trace:
[   16.950906]  <IRQ>
[   16.950918]  skl_interrupt+0x19e/0x2d6 [snd_soc_skl]
[   16.950926]  ? dma_supported+0xb5/0xb5 [snd_soc_skl]
[   16.950933]  __handle_irq_event_percpu+0x27a/0x6c8
[   16.950937]  ? __irq_wake_thread+0x1d1/0x1d1
[   16.950942]  ? __do_softirq+0x57a/0x69e
[   16.950944]  handle_irq_event_percpu+0x95/0x1ba
[   16.950948]  ? _raw_spin_unlock+0x65/0xdc
[   16.950951]  ? __handle_irq_event_percpu+0x6c8/0x6c8
[   16.950953]  ? _raw_spin_unlock+0x65/0xdc
[   16.950957]  ? time_cpufreq_notifier+0x483/0x483
[   16.950959]  handle_irq_event+0x89/0x123
[   16.950962]  handle_fasteoi_irq+0x16f/0x425
[   16.950965]  handle_irq+0x1fe/0x28e
[   16.950969]  do_IRQ+0x6e/0x12e
[   16.950972]  common_interrupt+0x7a/0x7a
[   16.950974]  </IRQ>
<snipped>
[   16.951031] RIP: snd_hdac_bus_update_rirb+0x19b/0x4cf [snd_hda_core] RSP: ffff88015c807c08
[   16.951036] ---[ end trace 58bf9ece1775bc92 ]---

Fixes: 2eeeb4f4733b ("ASoC: Intel: Skylake: Acquire irq after RIRB allocation")
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 sound/soc/intel/skylake/skl.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index e7fd14daeb4f..d174cbe35f7a 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -912,7 +912,11 @@ static int skl_first_init(struct hdac_bus *bus)
 
 	snd_hdac_bus_parse_capabilities(bus);
 
+	if (skl_acquire_irq(bus, 0) < 0)
+		return -EBUSY;
+
 	pci_set_master(pci);
+	synchronize_irq(bus->irq);
 
 	gcap = snd_hdac_chip_readw(bus, GCAP);
 	dev_dbg(bus->dev, "chipset global capabilities = 0x%x\n", gcap);
@@ -945,12 +949,6 @@ static int skl_first_init(struct hdac_bus *bus)
 	if (err < 0)
 		return err;
 
-	err = skl_acquire_irq(bus, 0);
-	if (err < 0)
-		return err;
-
-	synchronize_irq(bus->irq);
-
 	/* initialize chip */
 	skl_init_pci(skl);
 
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

[PATCH v3 2/3] ASoC: enable interrupt after dma buffer initialization

[Yu Zhao]

In snd_hdac_bus_init_chip(), we enable interrupt before
snd_hdac_bus_init_cmd_io() initializing dma buffers. If irq has
been acquired and irq handler uses the dma buffer, kernel may crash
when interrupt comes in.

Fix the problem by postponing enabling irq after dma buffer
initialization. And warn once on null dma buffer pointer during the
initialization.

Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 sound/hda/hdac_controller.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 560ec0986e1a..11057d9f84ec 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -40,6 +40,8 @@ static void azx_clear_corbrp(struct hdac_bus *bus)
  */
 void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus)
 {
+	WARN_ON_ONCE(!bus->rb.area);
+
 	spin_lock_irq(&bus->reg_lock);
 	/* CORB set up */
 	bus->corb.addr = bus->rb.addr;
@@ -479,13 +481,15 @@ bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset)
 	/* reset controller */
 	azx_reset(bus, full_reset);
 
-	/* initialize interrupts */
+	/* clear interrupts */
 	azx_int_clear(bus);
-	azx_int_enable(bus);
 
 	/* initialize the codec command I/O */
 	snd_hdac_bus_init_cmd_io(bus);
 
+	/* enable interrupts after CORB/RIRB buffers are initialized above */
+	azx_int_enable(bus);
+
 	/* program the position buffer */
 	if (bus->use_posbuf && bus->posbuf.addr) {
 		snd_hdac_chip_writel(bus, DPLBASE, (u32)bus->posbuf.addr);
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

[PATCH v3 3/3] ASoC: don't call skl_init_chip() to reset intel skl soc

[Yu Zhao]

Internally, skl_init_chip() calls snd_hdac_bus_init_chip() which
1) sets bus->chip_init to prevent multiple entrances before device
is stopped; 2) enables interrupt.

We shouldn't use it for the purpose of resetting device only because
1) when we really want to initialize device, we won't be able to do
so; 2) we are ready to handle interrupt yet, and kernel crashes when
interrupt comes in.

Rename azx_reset() to snd_hdac_bus_reset_link(), and use it to reset
device properly.

Fixes: 60767abcea3d ("ASoC: Intel: Skylake: Reset the controller in probe")
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 include/sound/hdaudio.h       | 1 +
 sound/hda/hdac_controller.c   | 7 ++++---
 sound/soc/intel/skylake/skl.c | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 6f1e1f3b3063..cd1773d0e08f 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -412,6 +412,7 @@ void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus);
 void snd_hdac_bus_stop_cmd_io(struct hdac_bus *bus);
 void snd_hdac_bus_enter_link_reset(struct hdac_bus *bus);
 void snd_hdac_bus_exit_link_reset(struct hdac_bus *bus);
+int snd_hdac_bus_reset_link(struct hdac_bus *bus, bool full_reset);
 
 void snd_hdac_bus_update_rirb(struct hdac_bus *bus);
 int snd_hdac_bus_handle_stream_irq(struct hdac_bus *bus, unsigned int status,
diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 11057d9f84ec..74244d8e2909 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -385,7 +385,7 @@ void snd_hdac_bus_exit_link_reset(struct hdac_bus *bus)
 EXPORT_SYMBOL_GPL(snd_hdac_bus_exit_link_reset);
 
 /* reset codec link */
-static int azx_reset(struct hdac_bus *bus, bool full_reset)
+int snd_hdac_bus_reset_link(struct hdac_bus *bus, bool full_reset)
 {
 	if (!full_reset)
 		goto skip_reset;
@@ -410,7 +410,7 @@ static int azx_reset(struct hdac_bus *bus, bool full_reset)
  skip_reset:
 	/* check to see if controller is ready */
 	if (!snd_hdac_chip_readb(bus, GCTL)) {
-		dev_dbg(bus->dev, "azx_reset: controller not ready!\n");
+		dev_dbg(bus->dev, "controller not ready!\n");
 		return -EBUSY;
 	}
 
@@ -425,6 +425,7 @@ static int azx_reset(struct hdac_bus *bus, bool full_reset)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(snd_hdac_bus_reset_link);
 
 /* enable interrupts */
 static void azx_int_enable(struct hdac_bus *bus)
@@ -479,7 +480,7 @@ bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset)
 		return false;
 
 	/* reset controller */
-	azx_reset(bus, full_reset);
+	snd_hdac_bus_reset_link(bus, full_reset);
 
 	/* clear interrupts */
 	azx_int_clear(bus);
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index d174cbe35f7a..29225623b4b4 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -908,7 +908,7 @@ static int skl_first_init(struct hdac_bus *bus)
 		return -ENXIO;
 	}
 
-	skl_init_chip(bus, true);
+	snd_hdac_bus_reset_link(bus, true);
 
 	snd_hdac_bus_parse_capabilities(bus);
 
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH v3 2/3] ASoC: enable interrupt after dma buffer initialization

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]

On Wed, Sep 12, 2018 at 01:44:41PM -0600, Yu Zhao wrote:
> In snd_hdac_bus_init_chip(), we enable interrupt before
> snd_hdac_bus_init_cmd_io() initializing dma buffers. If irq has
> been acquired and irq handler uses the dma buffer, kernel may crash
> when interrupt comes in.

> ---
>  sound/hda/hdac_controller.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

This is an ALSA commit, not an ASoC one.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/3] ASoC: enable interrupt after dma buffer initialization

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 422 bytes --]

On Wed, Sep 12, 2018 at 01:44:41PM -0600, Yu Zhao wrote:
> In snd_hdac_bus_init_chip(), we enable interrupt before
> snd_hdac_bus_init_cmd_io() initializing dma buffers. If irq has
> been acquired and irq handler uses the dma buffer, kernel may crash
> when interrupt comes in.

> ---
>  sound/hda/hdac_controller.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

This is an ALSA commit, not an ASoC one.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

On Wed, Sep 12, 2018 at 01:32:21PM -0600, Yu Zhao wrote:
> On Wed, Sep 12, 2018 at 11:20:20AM +0100, Mark Brown wrote:

> > Please use normal subject lines and so on even for revert patches -
> > they're patches just the same as any other.

> Well, they seem pretty normal to me, and have been used by many:

Many people do them but that doesn't mean they're good practice!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 374 bytes --]

On Wed, Sep 12, 2018 at 01:32:21PM -0600, Yu Zhao wrote:
> On Wed, Sep 12, 2018 at 11:20:20AM +0100, Mark Brown wrote:

> > Please use normal subject lines and so on even for revert patches -
> > they're patches just the same as any other.

> Well, they seem pretty normal to me, and have been used by many:

Many people do them but that doesn't mean they're good practice!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]