ASoC: Intel: hdac_hdmi: add Icelake support by bardliao · Pull Request #256 · thesofproject/linux

bardliao · 2018-11-08T03:22:48Z

Add Icelake device id. Also, Icelake's pin2port mapping table is
complicated. So we use a mapping table to do the pin2port mapping.

Signed-off-by: Bard liao bard.liao@intel.com

libinyang · 2018-11-08T06:17:43Z

NID 2 is vendor node is very strange based on my old knowledge. Where do you find the information? I need to have a check.

@libinyang I will send you the link of the spec.
Thanks Bard. After checking with SPEC, I think the HW registers are different from previous ones and the mapping are changed. The legacy HDA audio driver need changes. I will do it on the legacy driver later when I have time.

I think we need to revisit this. This 0x2 NID is correct but not limited to ICL, so we should have the NID information defined in less-ICL specific terms, e.g. INTEL_VENDOR_NID_0x2 - same for GLK.

@plbossart How about using magic number directly? The define is only used in the _dra_data struct and we can know it is a vendor nid from the field's name (.vendor_nid).

we want to keep the info that this is an INTEL VENDOR thingy. This is lost if you use 0x2, people might assume it's a standard value instead of a vendor one. So yes while the define brings no value from a compilation perspective, it's semantically useful for the code reviews.

bardliao · 2018-11-08T06:31:03Z

@plbossart @lgirdwood CI complains
CHECK: Alignment should match open parenthesis
#108: FILE: sound/soc/codecs/hdac_hdmi.c:2300:

HDA_CODEC_EXT_ENTRY(0x8086280f, 0x100000, "Icelake HDMI",
```
 &intel_icl_drv_data),
```

But that is how the existing code does. So I was wondering should we fix all of these or just leave as it is?

keyonjie · 2018-11-08T07:04:22Z

@bardliao I think we can change the alignment for glk and cnl together in a shot, add some comments about that if you feel it helpful for upstreaming.

bardliao · 2018-11-08T09:56:03Z

@keyonjie Actually, the upstreaming code has the same style. So I don't think that will be blocked by the reason.

lgirdwood · 2018-11-08T10:48:46Z

can you comment here what you are searching for

lgirdwood · 2018-11-08T10:49:19Z

comment here, what is port_map ?

i would move the static table just above this to help the reviewer connect the dots

const int *port_map?

lgirdwood · 2018-11-08T10:49:56Z

should this be const ?

I wasn't able to figure out how those values were determined, can you share a private email on what parts of the hardware spec you are looking at?

plbossart

looks mostly good but we can make the patch cleaner and more self-explanatory - this will be needed to share upstream. Thanks!

plbossart · 2018-11-08T14:16:26Z

i would move the static table just above this to help the reviewer connect the dots

plbossart · 2018-11-08T14:24:18Z

I wasn't able to figure out how those values were determined, can you share a private email on what parts of the hardware spec you are looking at?

plbossart · 2018-11-08T14:28:46Z

I think we need to revisit this. This 0x2 NID is correct but not limited to ICL, so we should have the NID information defined in less-ICL specific terms, e.g. INTEL_VENDOR_NID_0x2 - same for GLK.

plbossart · 2018-11-08T14:29:08Z

const int *port_map?

plbossart · 2018-11-08T14:54:21Z

@bardliao don't worry about the checkpatch report, it's better to avoid changing the alignment in existing code already upstream. It's more for new code that I care.

bardliao · 2018-11-09T02:53:26Z

@plbossart @lgirdwood I just pushed a new version. Could you review it?

Add Icelake device id. Also, Icelake's pin2port mapping table is complicated. So we use a mapping table to do the pin2port mapping. Signed-off-by: Bard liao <bard.liao@intel.com>

plbossart

Looks good @bardliao! Can you send upstream please?

bardliao · 2018-11-10T08:13:32Z

@plbossart Sure. I will do it.

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

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>

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>

In case of error when adding a new rule that refers to an anonymous set, deactivate expressions via NFT_TRANS_PREPARE state, not NFT_TRANS_RELEASE. Thus, the lookup expression marks anonymous sets as inactive in the next generation to ensure it is not reachable in this transaction anymore and decrement the set refcount as introduced by c1592a8 ("netfilter: nf_tables: deactivate anonymous set from preparation phase"). The abort step takes care of undoing the anonymous set. This is also consistent with rule deletion, where NFT_TRANS_PREPARE is used. Note that this error path is exercised in the preparation step of the commit protocol. This patch replaces nf_tables_rule_release() by the deactivate and destroy calls, this time with NFT_TRANS_PREPARE. Due to this incorrect error handling, it is possible to access a dangling pointer to the anonymous set that remains in the transaction list. [1009.379054] BUG: KASAN: use-after-free in nft_set_lookup_global+0x147/0x1a0 [nf_tables] [1009.379106] Read of size 8 at addr ffff88816c4c8020 by task nft-rule-add/137110 [1009.379116] CPU: 7 PID: 137110 Comm: nft-rule-add Not tainted 6.4.0-rc4+ thesofproject#256 [1009.379128] Call Trace: [1009.379132] <TASK> [1009.379135] dump_stack_lvl+0x33/0x50 [1009.379146] ? nft_set_lookup_global+0x147/0x1a0 [nf_tables] [1009.379191] print_address_description.constprop.0+0x27/0x300 [1009.379201] kasan_report+0x107/0x120 [1009.379210] ? nft_set_lookup_global+0x147/0x1a0 [nf_tables] [1009.379255] nft_set_lookup_global+0x147/0x1a0 [nf_tables] [1009.379302] nft_lookup_init+0xa5/0x270 [nf_tables] [1009.379350] nf_tables_newrule+0x698/0xe50 [nf_tables] [1009.379397] ? nf_tables_rule_release+0xe0/0xe0 [nf_tables] [1009.379441] ? kasan_unpoison+0x23/0x50 [1009.379450] nfnetlink_rcv_batch+0x97c/0xd90 [nfnetlink] [1009.379470] ? nfnetlink_rcv_msg+0x480/0x480 [nfnetlink] [1009.379485] ? __alloc_skb+0xb8/0x1e0 [1009.379493] ? __alloc_skb+0xb8/0x1e0 [1009.379502] ? entry_SYSCALL_64_after_hwframe+0x46/0xb0 [1009.379509] ? unwind_get_return_address+0x2a/0x40 [1009.379517] ? write_profile+0xc0/0xc0 [1009.379524] ? avc_lookup+0x8f/0xc0 [1009.379532] ? __rcu_read_unlock+0x43/0x60 Fixes: 958bee1 ("netfilter: nf_tables: use new transaction infrastructure to handle sets") Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Add a detachment test case with miniq present to assert that with and without the miniq we get the same error. # ./test_progs -t tc_opts thesofproject#244 tc_opts_after:OK thesofproject#245 tc_opts_append:OK thesofproject#246 tc_opts_basic:OK thesofproject#247 tc_opts_before:OK thesofproject#248 tc_opts_chain_classic:OK thesofproject#249 tc_opts_delete_empty:OK thesofproject#250 tc_opts_demixed:OK thesofproject#251 tc_opts_detach:OK thesofproject#252 tc_opts_detach_after:OK thesofproject#253 tc_opts_detach_before:OK thesofproject#254 tc_opts_dev_cleanup:OK thesofproject#255 tc_opts_invalid:OK thesofproject#256 tc_opts_mixed:OK thesofproject#257 tc_opts_prepend:OK thesofproject#258 tc_opts_replace:OK thesofproject#259 tc_opts_revision:OK Summary: 16/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Link: https://lore.kernel.org/r/20230804131112.11012-2-daniel@iogearbox.net Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>

Add several new tcx test cases to improve test coverage. This also includes a few new tests with ingress instead of clsact qdisc, to cover the fix from commit dc644b5 ("tcx: Fix splat in ingress_destroy upon tcx_entry_free"). # ./test_progs -t tc [...] thesofproject#234 tc_links_after:OK thesofproject#235 tc_links_append:OK thesofproject#236 tc_links_basic:OK thesofproject#237 tc_links_before:OK thesofproject#238 tc_links_chain_classic:OK thesofproject#239 tc_links_chain_mixed:OK thesofproject#240 tc_links_dev_cleanup:OK thesofproject#241 tc_links_dev_mixed:OK thesofproject#242 tc_links_ingress:OK thesofproject#243 tc_links_invalid:OK thesofproject#244 tc_links_prepend:OK thesofproject#245 tc_links_replace:OK thesofproject#246 tc_links_revision:OK thesofproject#247 tc_opts_after:OK thesofproject#248 tc_opts_append:OK thesofproject#249 tc_opts_basic:OK thesofproject#250 tc_opts_before:OK thesofproject#251 tc_opts_chain_classic:OK thesofproject#252 tc_opts_chain_mixed:OK thesofproject#253 tc_opts_delete_empty:OK thesofproject#254 tc_opts_demixed:OK thesofproject#255 tc_opts_detach:OK thesofproject#256 tc_opts_detach_after:OK thesofproject#257 tc_opts_detach_before:OK thesofproject#258 tc_opts_dev_cleanup:OK thesofproject#259 tc_opts_invalid:OK thesofproject#260 tc_opts_mixed:OK thesofproject#261 tc_opts_prepend:OK thesofproject#262 tc_opts_replace:OK thesofproject#263 tc_opts_revision:OK [...] Summary: 44/38 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Link: https://lore.kernel.org/r/8699efc284b75ccdc51ddf7062fa2370330dc6c0.1692029283.git.daniel@iogearbox.net Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>

Add several new test cases which assert corner cases on the mprog query mechanism, for example, around passing in a too small or a larger array than the current count. ./test_progs -t tc_opts thesofproject#252 tc_opts_after:OK thesofproject#253 tc_opts_append:OK thesofproject#254 tc_opts_basic:OK thesofproject#255 tc_opts_before:OK thesofproject#256 tc_opts_chain_classic:OK thesofproject#257 tc_opts_chain_mixed:OK thesofproject#258 tc_opts_delete_empty:OK thesofproject#259 tc_opts_demixed:OK thesofproject#260 tc_opts_detach:OK thesofproject#261 tc_opts_detach_after:OK thesofproject#262 tc_opts_detach_before:OK thesofproject#263 tc_opts_dev_cleanup:OK thesofproject#264 tc_opts_invalid:OK thesofproject#265 tc_opts_max:OK thesofproject#266 tc_opts_mixed:OK thesofproject#267 tc_opts_prepend:OK thesofproject#268 tc_opts_query:OK thesofproject#269 tc_opts_query_attach:OK thesofproject#270 tc_opts_replace:OK thesofproject#271 tc_opts_revision:OK Summary: 20/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Reviewed-by: Alan Maguire <alan.maguire@oracle.com> Link: https://lore.kernel.org/bpf/20231017081728.24769-1-daniel@iogearbox.net

bardliao requested review from lgirdwood and plbossart November 8, 2018 03:22

bardliao force-pushed the topic/hdac_hdmi branch from b9219e3 to bc78fc6Compare November 8, 2018 05:36

libinyang reviewed Nov 8, 2018
View reviewed changes

lgirdwood requested changes Nov 8, 2018
View reviewed changes

plbossart requested changes Nov 8, 2018
View reviewed changes

bardliao force-pushed the topic/hdac_hdmi branch 4 times, most recently from 1a6d2fe to 68650a2Compare November 9, 2018 02:45

bardliao force-pushed the topic/hdac_hdmi branch from 68650a2 to 2c3e967Compare November 9, 2018 04:40

ASoC: Intel: hdac_hdmi: add Icelake support
2c3e967
Add Icelake device id. Also, Icelake's pin2port mapping table is complicated. So we use a mapping table to do the pin2port mapping. Signed-off-by: Bard liao <bard.liao@intel.com>

plbossart approved these changes Nov 9, 2018
View reviewed changes

plbossart merged commit d8cd6e2 into thesofproject:topic/sof-devNov 9, 2018

Uh oh!

Conversation

bardliao commented Nov 8, 2018

Uh oh!

libinyangNov 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliaoNov 8, 2018• edited by libinyang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao commented Nov 8, 2018

Uh oh!

keyonjie commented Nov 8, 2018

Uh oh!

bardliao commented Nov 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Nov 8, 2018

Uh oh!

bardliao commented Nov 9, 2018

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

bardliao commented Nov 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

libinyangNov 8, 2018•
edited
Loading

bardliaoNov 8, 2018•
edited by libinyang
Loading