Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SKL+: need better split between HDA and non-HDA support #254

Closed
plbossart opened this issue Nov 8, 2018 · 9 comments
Closed

SKL+: need better split between HDA and non-HDA support #254

plbossart opened this issue Nov 8, 2018 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@plbossart
Copy link
Member

The current code includes too many parts and variables from the HDAC library when SND_SOC_SOF_HDA is not defined.

See below compile-tested code only to check what is really needed

diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c
index 759f1e61a030..50b0a55ce655 100644
--- a/sound/soc/sof/intel/hda-bus.c
+++ b/sound/soc/sof/intel/hda-bus.c
@@ -80,29 +80,30 @@ static const struct hdac_io_ops io_ops = {
 int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
 		     const struct hdac_ext_bus_ops *ext_ops)
 {
-	static int idx;
+	static int idx; // Is this function called multiple times?
 
 	memset(bus, 0, sizeof(*bus));
 	bus->dev = dev;
 
 	bus->io_ops = &io_ops;
 	INIT_LIST_HEAD(&bus->stream_list);
-	INIT_LIST_HEAD(&bus->codec_list);
 
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
-	bus->ops = &bus_ops;
-	INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
-#endif
-	spin_lock_init(&bus->reg_lock);
-	mutex_init(&bus->cmd_mutex);
+
 	bus->irq = -1;
 
 	bus->ext_ops = ext_ops;
-	INIT_LIST_HEAD(&bus->hlink_list);
 	bus->idx = idx++;
-
 	mutex_init(&bus->lock);
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
+	INIT_LIST_HEAD(&bus->codec_list);
+	spin_lock_init(&bus->reg_lock);
+	mutex_init(&bus->cmd_mutex);
+	INIT_LIST_HEAD(&bus->hlink_list);
+	bus->ops = &bus_ops;
+	INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
 	bus->cmd_dma_state = true;
+#endif
 
 	return 0;
 }
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index 42a986ce35a0..c8c0d405ef79 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -610,6 +610,7 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
 		return -ENOMEM;
 	}
 
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) // FIXME: where is this freed when HDA is enabled?
 	/* mem alloc for the CORB/RIRB ringbuffers */
 	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
 				  PAGE_SIZE, &bus->rb);
@@ -617,7 +618,7 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
 		dev_err(sdev->dev, "error: RB alloc failed\n");
 		return -ENOMEM;
 	}
-
+#endif
 	/* create capture streams */
 	for (i = 0; i < num_capture; i++) {
 

@keyonjie can you review and comment?

@keyonjie
Copy link

keyonjie commented Nov 8, 2018

The current code includes too many parts and variables from the HDAC library when SND_SOC_SOF_HDA is not defined.

Good revisit. We did this in a tight schedule, it's time to refine it now.

See below compile-tested code only to check what is really needed

diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c
index 759f1e61a030..50b0a55ce655 100644
--- a/sound/soc/sof/intel/hda-bus.c
+++ b/sound/soc/sof/intel/hda-bus.c
@@ -80,29 +80,30 @@ static const struct hdac_io_ops io_ops = {
 int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
 		     const struct hdac_ext_bus_ops *ext_ops)
 {
-	static int idx;
+	static int idx; // Is this function called multiple times?

I think this is for bus index, theoretically, it can be called multiple times then we have bus 0, bus 1, ...
But I never come across it yet. @mengdonglin please correct me if wrong.

memset(bus, 0, sizeof(*bus));
bus->dev = dev;

bus->io_ops = &io_ops;
INIT_LIST_HEAD(&bus->stream_list);

  • INIT_LIST_HEAD(&bus->codec_list);

-#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)

  • bus->ops = &bus_ops;
  • INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
    -#endif
  • spin_lock_init(&bus->reg_lock);
  • mutex_init(&bus->cmd_mutex);
  • bus->irq = -1;

    bus->ext_ops = ext_ops;

  • INIT_LIST_HEAD(&bus->hlink_list);
    bus->idx = idx++;
  • mutex_init(&bus->lock);

We can even move this into HDA defined block also.

+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)

  • INIT_LIST_HEAD(&bus->codec_list);
  • spin_lock_init(&bus->reg_lock);
  • mutex_init(&bus->cmd_mutex);
  • INIT_LIST_HEAD(&bus->hlink_list);
  • bus->ops = &bus_ops;
  • INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
    bus->cmd_dma_state = true;
    +#endif

Just went though all these members, agree we can handle them only when HDA defined.
We can even move 'mutex_init(&bus->lock);' into this block also.

return 0;
}
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index 42a986ce35a0..c8c0d405ef79 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -610,6 +610,7 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
return -ENOMEM;
}

+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) // FIXME: where is this freed when HDA is enabled?

good catch, let's do freeing it in hda_dsp_stream_free().

/* mem alloc for the CORB/RIRB ringbuffers */
ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
PAGE_SIZE, &bus->rb);
@@ -617,7 +618,7 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
dev_err(sdev->dev, "error: RB alloc failed\n");
return -ENOMEM;
}

+#endif
/* create capture streams */
for (i = 0; i < num_capture; i++) {


@keyonjie can you review and comment?

@plbossart Thanks for reviewing this part. let me do a patch to fix it.

@keyonjie
Copy link

keyonjie commented Nov 8, 2018

@plbossart PR #257 is for this, please help review.

@mengdonglin mengdonglin added the enhancement New feature or request label Nov 22, 2018
@mengdonglin
Copy link
Collaborator

mengdonglin commented Nov 22, 2018

@plbossart @keyonjie It seems this feature can be closed since PR #257 has been merged.

@plbossart
Copy link
Member Author

No, I would still like to see how we remove the static variable for the initialization of bus->idx

@plbossart
Copy link
Member Author

@keyonjie can you look into this bus->idx initialization, this is the only part that needs to be cleaned.

@wenqingfu
Copy link

@keyonjie do you agree with #300 ? Can you please follow up to finish up this one?

@keyonjie
Copy link

keyonjie commented Apr 2, 2019

just checked and found the bus->idx initialization is already supported(static and increased for each x_init() calling), the decent decreasing(freeing) of this is needed, let me do a patch for it.

@keyonjie
Copy link

keyonjie commented Apr 2, 2019

@plbossart how do you think about something like this:

diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c
index 62cc9921bb55..d924d4e3a28e 100644
--- a/sound/soc/sof/intel/hda-bus.c
+++ b/sound/soc/sof/intel/hda-bus.c
@@ -74,14 +74,34 @@ static const struct hdac_io_ops io_ops = {
        .dma_free_pages = sof_hda_dma_free_pages,
 };
 
+static u64 bus_idx_bits = 0;
+
+static int get_free_index(void)
+{
+       int i = 0;
+
+       while ((bus_idx_bits >> i) & 0x1) {
+               if (i++ == 63)
+                       return -EINVAL;
+       }
+
+       bus_idx_bits |= 1 << i;
+       return i;
+}
+
+static void put_index(int i)
+{
+       if (i < 0 || i > 63)
+               return;
+       bus_idx_bits &= ~((u64)1 << i);
+}
+
 /*
  * This can be used for both with/without hda link support.
  */
 void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
                      const struct hdac_ext_bus_ops *ext_ops)
 {
-       static int idx;
-
        memset(bus, 0, sizeof(*bus));
        bus->dev = dev;
 
@@ -90,7 +110,7 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
 
        bus->irq = -1;
        bus->ext_ops = ext_ops;
-       bus->idx = idx++;
+       bus->idx = get_free_index();
 
        spin_lock_init(&bus->reg_lock);
 
@@ -106,3 +126,12 @@ void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
 #endif
 
 }
+
+/*
+ * This can be used for both with/without hda link support.
+ */
+void sof_hda_bus_deinit(struct hdac_bus *bus)
+{
+       if (bus)
+               put_index(bus->idx);
+}
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index a57374be06e6..66405ad0a6e9 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -666,6 +666,8 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
 #endif
        hda_codec_i915_exit(sdev);
 
+       sof_hda_bus_deinit();
+
        return 0;
 }

@plbossart
Copy link
Member Author

closing, this was fixed by PR#862

plbossart pushed a commit that referenced this issue Jan 13, 2020
Both drd and gadget interrupt handler use the struct cdns3 pointer as
dev_id, it causes devm_free_irq at cdns3_gadget_exit doesn't free
gadget's interrupt handler, it freed drd's handler. So, when the
host interrupt occurs, the gadget's interrupt hanlder is still
called, and causes below oops. To fix it, we use gadget's private
data priv_dev as interrupt dev_id for gadget.

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000380
Mem abort info:
  ESR = 0x96000006
  EC = 0x25: DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
Data abort info:
  ISV = 0, ISS = 0x00000006
  CM = 0, WnR = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=0000000971d79000
[0000000000000380] pgd=0000000971d6f003, pud=0000000971d6e003, pmd=0000000000000000
Internal error: Oops: 96000006 [#1] PREEMPT SMP
Modules linked in: mxc_jpeg_encdec crct10dif_ce fsl_imx8_ddr_perf
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.4.0-03486-g69f4e7d9c54a-dirty #254
Hardware name: Freescale i.MX8QM MEK (DT)
pstate: 00000085 (nzcv daIf -PAN -UAO)
pc : cdns3_device_irq_handler+0x1c/0xb8
lr : __handle_irq_event_percpu+0x78/0x2c0
sp : ffff800010003e30
x29: ffff800010003e30 x28: ffff8000129bb000
x27: ffff8000126e9000 x26: ffff0008f61b5600
x25: ffff800011fe1018 x24: ffff8000126ea120
x23: ffff800010003f04 x22: 0000000000000000
x21: 0000000000000093 x20: ffff0008f61b5600
x19: ffff0008f5061a80 x18: 0000000000000000
x17: 0000000000000000 x16: 0000000000000000
x15: 0000000000000000 x14: 003d090000000000
x13: 00003d0900000000 x12: 0000000000000000
x11: 00003d0900000000 x10: 0000000000000040
x9 : ffff800012708cb8 x8 : ffff800012708cb0
x7 : ffff0008f7c7a9d0 x6 : 0000000000000000
x5 : ffff0008f7c7a910 x4 : ffff8008ed359000
x3 : ffff800010003f40 x2 : 0000000000000000
x1 : ffff0008f5061a80 x0 : ffff800010161a60
Call trace:
 cdns3_device_irq_handler+0x1c/0xb8
 __handle_irq_event_percpu+0x78/0x2c0
 handle_irq_event_percpu+0x40/0x98
 handle_irq_event+0x4c/0xd0
 handle_fasteoi_irq+0xbc/0x168
 generic_handle_irq+0x34/0x50
 __handle_domain_irq+0x6c/0xc0
 gic_handle_irq+0xd4/0x174
 el1_irq+0xb8/0x180
 arch_cpu_idle+0x3c/0x230
 default_idle_call+0x38/0x40
 do_idle+0x20c/0x298
 cpu_startup_entry+0x28/0x48
 rest_init+0xdc/0xe8
 arch_call_rest_init+0x14/0x1c
 start_kernel+0x48c/0x4b8
Code: aa0103f3 aa1e03e0 d503201f f9409662 (f941c040)
---[ end trace 091dcf4dee011b0e ]---
Kernel panic - not syncing: Fatal exception in interrupt
SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x0002,2100600c
Memory Limit: none
---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---

Fixes: 7733f6c ("usb: cdns3: Add Cadence USB3 DRD Driver")
Cc: <stable@vger.kernel.org> #v5.4
Signed-off-by: Peter Chen <peter.chen@nxp.com>
Link: https://lore.kernel.org/r/1577437804-18146-1-git-send-email-peter.chen@nxp.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
wenliangwu pushed a commit that referenced this issue Oct 30, 2023
Add various tests to check maximum number of supported programs
being attached:

  # ./vmtest.sh -- ./test_progs -t tc_opts
  [...]
  ./test_progs -t tc_opts
  [    1.185325] bpf_testmod: loading out-of-tree module taints kernel.
  [    1.186826] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel
  [    1.270123] tsc: Refined TSC clocksource calibration: 3407.988 MHz
  [    1.272428] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x311fc932722, max_idle_ns: 440795381586 ns
  [    1.276408] clocksource: Switched to clocksource tsc
  #252     tc_opts_after:OK
  #253     tc_opts_append:OK
  #254     tc_opts_basic:OK
  #255     tc_opts_before:OK
  #256     tc_opts_chain_classic:OK
  #257     tc_opts_chain_mixed:OK
  #258     tc_opts_delete_empty:OK
  #259     tc_opts_demixed:OK
  #260     tc_opts_detach:OK
  #261     tc_opts_detach_after:OK
  #262     tc_opts_detach_before:OK
  #263     tc_opts_dev_cleanup:OK
  #264     tc_opts_invalid:OK
  #265     tc_opts_max:OK              <--- (new test)
  #266     tc_opts_mixed:OK
  #267     tc_opts_prepend:OK
  #268     tc_opts_replace:OK
  #269     tc_opts_revision:OK
  Summary: 18/0 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Link: https://lore.kernel.org/bpf/20230929204121.20305-2-daniel@iogearbox.net
wenliangwu pushed a commit that referenced this issue Oct 30, 2023
Add a new test case which performs double query of the bpf_mprog through
libbpf API, but also via raw bpf(2) syscall. This is testing to gather
first the count and then in a subsequent probe the full information with
the program array without clearing passed structs in between.

  # ./vmtest.sh -- ./test_progs -t tc_opts
  [...]
  ./test_progs -t tc_opts
  [    1.398818] tsc: Refined TSC clocksource calibration: 3407.999 MHz
  [    1.400263] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x311fd336761, max_idle_ns: 440795243819 ns
  [    1.402734] clocksource: Switched to clocksource tsc
  [    1.426639] bpf_testmod: loading out-of-tree module taints kernel.
  [    1.428112] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel
  #252     tc_opts_after:OK
  #253     tc_opts_append:OK
  #254     tc_opts_basic:OK
  #255     tc_opts_before:OK
  #256     tc_opts_chain_classic:OK
  #257     tc_opts_chain_mixed:OK
  #258     tc_opts_delete_empty:OK
  #259     tc_opts_demixed:OK
  #260     tc_opts_detach:OK
  #261     tc_opts_detach_after:OK
  #262     tc_opts_detach_before:OK
  #263     tc_opts_dev_cleanup:OK
  #264     tc_opts_invalid:OK
  #265     tc_opts_max:OK
  #266     tc_opts_mixed:OK
  #267     tc_opts_prepend:OK
  #268     tc_opts_query:OK            <--- (new test)
  #269     tc_opts_replace:OK
  #270     tc_opts_revision:OK
  Summary: 19/0 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: https://lore.kernel.org/r/20231006220655.1653-4-daniel@iogearbox.net
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
wenliangwu pushed a commit that referenced this issue Oct 30, 2023
Add a new test case to query on an empty bpf_mprog and pass the revision
directly into expected_revision for attachment to assert that this does
succeed.

  ./test_progs -t tc_opts
  [    1.406778] tsc: Refined TSC clocksource calibration: 3407.990 MHz
  [    1.408863] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x311fcaf6eb0, max_idle_ns: 440795321766 ns
  [    1.412419] clocksource: Switched to clocksource tsc
  [    1.428671] bpf_testmod: loading out-of-tree module taints kernel.
  [    1.430260] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel
  #252     tc_opts_after:OK
  #253     tc_opts_append:OK
  #254     tc_opts_basic:OK
  #255     tc_opts_before:OK
  #256     tc_opts_chain_classic:OK
  #257     tc_opts_chain_mixed:OK
  #258     tc_opts_delete_empty:OK
  #259     tc_opts_demixed:OK
  #260     tc_opts_detach:OK
  #261     tc_opts_detach_after:OK
  #262     tc_opts_detach_before:OK
  #263     tc_opts_dev_cleanup:OK
  #264     tc_opts_invalid:OK
  #265     tc_opts_max:OK
  #266     tc_opts_mixed:OK
  #267     tc_opts_prepend:OK
  #268     tc_opts_query:OK
  #269     tc_opts_query_attach:OK     <--- (new test)
  #270     tc_opts_replace:OK
  #271     tc_opts_revision:OK
  Summary: 20/0 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: https://lore.kernel.org/r/20231006220655.1653-6-daniel@iogearbox.net
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
plbossart pushed a commit that referenced this issue Jun 12, 2024
… rules

rx_create no longer allocates a modify_hdr instance that needs to be
cleaned up. The mlx5_modify_header_dealloc call will lead to a NULL pointer
dereference. A leak in the rules also previously occurred since there are
now two rules populated related to status.

  BUG: kernel NULL pointer dereference, address: 0000000000000000
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  PGD 109907067 P4D 109907067 PUD 116890067 PMD 0
  Oops: 0000 [#1] SMP
  CPU: 1 PID: 484 Comm: ip Not tainted 6.9.0-rc2-rrameshbabu+ #254
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS Arch Linux 1.16.3-1-1 04/01/2014
  RIP: 0010:mlx5_modify_header_dealloc+0xd/0x70
  <snip>
  Call Trace:
   <TASK>
   ? show_regs+0x60/0x70
   ? __die+0x24/0x70
   ? page_fault_oops+0x15f/0x430
   ? free_to_partial_list.constprop.0+0x79/0x150
   ? do_user_addr_fault+0x2c9/0x5c0
   ? exc_page_fault+0x63/0x110
   ? asm_exc_page_fault+0x27/0x30
   ? mlx5_modify_header_dealloc+0xd/0x70
   rx_create+0x374/0x590
   rx_add_rule+0x3ad/0x500
   ? rx_add_rule+0x3ad/0x500
   ? mlx5_cmd_exec+0x2c/0x40
   ? mlx5_create_ipsec_obj+0xd6/0x200
   mlx5e_accel_ipsec_fs_add_rule+0x31/0xf0
   mlx5e_xfrm_add_state+0x426/0xc00
  <snip>

Fixes: 94af50c ("net/mlx5e: Unify esw and normal IPsec status table creation/destruction")
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants