| Message ID | 87v996od2c.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| State | Accepted |
| Commit | f2138aed231c88d5c4fa8d06aa15ad19685087c2 |
| Headers | show |
| Series | ASoC: simple-card-utils: prepare for multi support | expand |
On Thu, Apr 01, 2021 at 01:15:23PM +0900, Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Current simple-card / audio-graph are assuming fixed > single-CPU/Codec/Platform. > This patch prepares multi-CPU/Codec/Platform support. > > Note is that it is not yet full-multi-support. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > include/sound/simple_card_utils.h | 8 +++ > sound/soc/generic/audio-graph-card.c | 22 ++++++++ > sound/soc/generic/simple-card-utils.c | 72 +++++++++++++++++++-------- > sound/soc/generic/simple-card.c | 30 +++++++++++ > 4 files changed, 110 insertions(+), 22 deletions(-) Hi, This seems to break display support on a Jetson TX2 board for me, though I admittedly don't quite understand how it would be related to display at all. Reverting basically the whole series (because subsequent patches depend on this on) on top of next-20210415, I get working display again. There's this in the log, which seems to be related: [ 14.671377] tegra-audio-graph-card sound: too many links [ 14.799645] tegra-audio-graph-card sound: too many links [ 14.845375] tegra-audio-graph-card sound: too many links [ 14.853635] tegra-audio-graph-card sound: too many links [ 14.860934] tegra-audio-graph-card sound: too many links [ 14.868781] tegra-audio-graph-card sound: too many links [ 14.875659] tegra-audio-graph-card sound: too many links [ 14.907874] tegra-audio-graph-card sound: too many links [ 14.917351] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010 [ 14.926255] Mem abort info: [ 14.929096] ESR = 0x96000047 [ 14.932208] EC = 0x25: DABT (current EL), IL = 32 bits [ 14.937559] SET = 0, FnV = 0 [ 14.940642] EA = 0, S1PTW = 0 [ 14.943867] Data abort info: [ 14.946753] ISV = 0, ISS = 0x00000047 [ 14.950611] CM = 0, WnR = 1 [ 14.953614] user pgtable: 64k pages, 48-bit VAs, pgdp=0000000100b94400 [ 14.960185] [0000000000000010] pgd=0800000102280003, p4d=0800000102280003, pud=0800000102280003, pmd=0800000101050003, pte=0000000000000000 [ 14.972774] Internal error: Oops: 96000047 [#1] PREEMPT SMP [ 14.978362] Modules linked in: drm_kms_helper snd_soc_tegra210_admaif snd_soc_tegra_pcm snd_soc_tegra210_dmic snd_soc_tegra210_i2s snd_soc_tegra186_dspk cfbfillrect cfbimgblt cfbcopyarea snd_soc_tegra210_ahub snd_soc_tegra_audio_graph_card snd_soc_audio_graph_card snd_soc_simple_card_utils crct10dif_ce at24 tegra_aconnect tegra_bpmp_thermal host1x drm fuse drm_panel_orientation_quirks ipv6 [ 15.012917] CPU: 2 PID: 69 Comm: kworker/u12:3 Tainted: G S 5.12.0-rc7-next-20210415 #108 [ 15.022390] Hardware name: NVIDIA Jetson TX2 Developer Kit (DT) [ 15.028303] Workqueue: events_unbound deferred_probe_work_func [ 15.034159] pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--) [ 15.040162] pc : snd_soc_get_dai_name+0x124/0x150 [ 15.044883] lr : snd_soc_get_dai_name+0xa4/0x150 [ 15.049521] sp : ffff800011fef780 [ 15.052849] x29: ffff800011fef780 x28: ffff00008bc14980 [ 15.058181] x27: 0000000000000000 x26: ffff000081aafc10 [ 15.063511] x25: ffff0001f7091680 x24: ffff800011538798 [ 15.068841] x23: 0000000000000010 x22: ffff800011538778 [ 15.074169] x21: ffff800011fef808 x20: 00000000fffffdf4 [ 15.079486] x19: ffff0000809c7880 x18: 0000000000000030 [ 15.084813] x17: 0000000000000000 x16: 0000000000000000 [ 15.090142] x15: ffffffffffffffff x14: ffffffff00000000 [ 15.095468] x13: ffffffffffffffff x12: 0000000000000020 [ 15.100804] x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f [ 15.106135] x9 : 7f7f7f7f7f7f7f7f x8 : feff303f73716e6f [ 15.111464] x7 : 00000000ffffffff x6 : 0000314000000000 [ 15.116791] x5 : ffff0001f7091d28 x4 : ffff00008c41a010 [ 15.122109] x3 : 0000000000000000 x2 : 0000000000000010 [ 15.127439] x1 : 0000000000000000 x0 : ffff8000094614d8 [ 15.132756] Call trace: [ 15.135201] snd_soc_get_dai_name+0x124/0x150 [ 15.139560] asoc_simple_parse_dai.part.0+0x70/0xd0 [snd_soc_audio_graph_card] [ 15.146784] graph_dai_link_of_dpcm+0x100/0x38c [snd_soc_audio_graph_card] [ 15.153672] __graph_for_each_link+0x1dc/0x204 [snd_soc_audio_graph_card] [ 15.160471] audio_graph_parse_of+0x148/0x20c [snd_soc_audio_graph_card] [ 15.167178] tegra_audio_graph_probe+0x6c/0x8c [snd_soc_tegra_audio_graph_card] [ 15.174499] platform_probe+0x68/0xe0 [ 15.178164] really_probe+0xe4/0x50c [ 15.181754] driver_probe_device+0x64/0xc4 [ 15.185861] __device_attach_driver+0xb4/0x110 [ 15.190314] bus_for_each_drv+0x78/0xd0 [ 15.194149] __device_attach+0xd8/0x180 [ 15.197985] device_initial_probe+0x14/0x20 [ 15.202168] bus_probe_device+0x9c/0xa4 [ 15.206003] deferred_probe_work_func+0x88/0xc0 [ 15.210532] process_one_work+0x1cc/0x350 [ 15.214542] worker_thread+0x68/0x3ac [ 15.218203] kthread+0x128/0x134 [ 15.221431] ret_from_fork+0x10/0x34 [ 15.225045] Code: fa531004 54ffff81 f9400c00 f9400000 (f90002e0) [ 15.231134] ---[ end trace 671a11645631ca2f ]--- Any ideas what could be wrong? Adding Jon and linux-tegra for visibility. Thierry
On Thu, Apr 15, 2021 at 08:01:07PM +0200, Thierry Reding wrote: > This seems to break display support on a Jetson TX2 board for me, though > I admittedly don't quite understand how it would be related to display > at all. Reverting basically the whole series (because subsequent patches > depend on this on) on top of next-20210415, I get working display again. Given that we got an oops it's probably just memory corruption somewhere. > There's this in the log, which seems to be related: > [ 14.671377] tegra-audio-graph-card sound: too many links > [ 14.799645] tegra-audio-graph-card sound: too many links This looks like an issue? Or perhaps it's just DPCM triggered... > [ 15.106135] x9 : 7f7f7f7f7f7f7f7f x8 : feff303f73716e6f > [ 15.111464] x7 : 00000000ffffffff x6 : 0000314000000000 > [ 15.116791] x5 : ffff0001f7091d28 x4 : ffff00008c41a010 > [ 15.122109] x3 : 0000000000000000 x2 : 0000000000000010 > [ 15.127439] x1 : 0000000000000000 x0 : ffff8000094614d8 > [ 15.132756] Call trace: > [ 15.135201] snd_soc_get_dai_name+0x124/0x150 Can you check where that is in the function?
On Thu, Apr 15, 2021 at 07:14:50PM +0100, Mark Brown wrote: > On Thu, Apr 15, 2021 at 08:01:07PM +0200, Thierry Reding wrote: > > > This seems to break display support on a Jetson TX2 board for me, though > > I admittedly don't quite understand how it would be related to display > > at all. Reverting basically the whole series (because subsequent patches > > depend on this on) on top of next-20210415, I get working display again. > > Given that we got an oops it's probably just memory corruption > somewhere. > > > There's this in the log, which seems to be related: > > > [ 14.671377] tegra-audio-graph-card sound: too many links > > [ 14.799645] tegra-audio-graph-card sound: too many links > > This looks like an issue? Or perhaps it's just DPCM triggered... Yeah, as I was looking into this a bit, I noticed that on Tegra186 and later the number of links can go up to 72. I'm not sure why this is wreaking havoc, since presumably the check is there to prevent the array from being overwritten, but apparently it's not. I suspect that the same check might be missing somewhere else. In any case, I came up with the attached. I don't know how good it is because now the number of links exceeds SNDRV_MINOR_DEVICES, but perhaps that's just irrelevant and that constant was used merely because it was conveniently there. The patch restores display on Jetson TX2. I can look around a bit if I can find where the boundary checks might be missing so that we gracefully fail rather than corrupting everything. Thierry From ba07d30380492661c8fc2677155c9c6230bae2fe Mon Sep 17 00:00:00 2001 From: Thierry Reding <treding@nvidia.com> Date: Thu, 15 Apr 2021 20:16:09 +0200 Subject: [PATCH] ASoC: simple-card-utils: Increase maximum number of links to 128 On Tegra186 and later, the number of links can go up to 72, so bump the maximum number of links to the next power of two (128). Fixes: f2138aed231c ("ASoC: simple-card-utils: enable flexible CPU/Codec/Platform") Signed-off-by: Thierry Reding <treding@nvidia.com> --- include/sound/simple_card_utils.h | 4 +++- sound/soc/generic/audio-graph-card.c | 4 ++-- sound/soc/generic/simple-card.c | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index fac3b832d982..e318a2d4ac44 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -115,10 +115,12 @@ struct asoc_simple_priv { ((codec) = simple_props_to_dai_codec(props, i)); \ (i)++) +#define SNDRV_MAX_LINKS 128 + struct link_info { int link; /* number of link */ int cpu; /* turn for CPU / Codec */ - struct prop_nums num[SNDRV_MINOR_DEVICES]; + struct prop_nums num[SNDRV_MAX_LINKS]; }; int asoc_simple_parse_daifmt(struct device *dev, diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 0582fe296471..80d065935d9a 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -613,7 +613,7 @@ static int graph_count_noml(struct asoc_simple_priv *priv, { struct device *dev = simple_priv_to_dev(priv); - if (li->link >= SNDRV_MINOR_DEVICES) { + if (li->link >= SNDRV_MAX_LINKS) { dev_err(dev, "too many links\n"); return -EINVAL; } @@ -636,7 +636,7 @@ static int graph_count_dpcm(struct asoc_simple_priv *priv, { struct device *dev = simple_priv_to_dev(priv); - if (li->link >= SNDRV_MINOR_DEVICES) { + if (li->link >= SNDRV_MAX_LINKS) { dev_err(dev, "too many links\n"); return -EINVAL; } diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index bf5ddf1ea65f..7ac64fef73c9 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -484,7 +484,7 @@ static int simple_count_noml(struct asoc_simple_priv *priv, struct device_node *codec, struct link_info *li, bool is_top) { - if (li->link >= SNDRV_MINOR_DEVICES) { + if (li->link >= SNDRV_MAX_LINKS) { struct device *dev = simple_priv_to_dev(priv); dev_err(dev, "too many links\n"); @@ -505,7 +505,7 @@ static int simple_count_dpcm(struct asoc_simple_priv *priv, struct device_node *codec, struct link_info *li, bool is_top) { - if (li->link >= SNDRV_MINOR_DEVICES) { + if (li->link >= SNDRV_MAX_LINKS) { struct device *dev = simple_priv_to_dev(priv); dev_err(dev, "too many links\n");
On Thu, Apr 15, 2021 at 08:25:21PM +0200, Thierry Reding wrote: > In any case, I came up with the attached. I don't know how good it is > because now the number of links exceeds SNDRV_MINOR_DEVICES, but perhaps > that's just irrelevant and that constant was used merely because it was > conveniently there. We shouldn't actually end up creating that many devices, a lot of those should be DPCM links which are internal only. > The patch restores display on Jetson TX2. I can look around a bit if I > can find where the boundary checks might be missing so that we > gracefully fail rather than corrupting everything. That'd be good, thanks.
diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index 86e46cbf9e14..475f8cb14492 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -38,6 +38,12 @@ struct asoc_simple_jack { struct snd_soc_jack_gpio gpio; }; +struct prop_nums { + int cpus; + int codecs; + int platforms; +}; + struct asoc_simple_priv { struct snd_soc_card snd_card; struct simple_dai_props { @@ -48,6 +54,7 @@ struct asoc_simple_priv { struct snd_soc_dai_link_component *platforms; struct asoc_simple_data adata; struct snd_soc_codec_conf *codec_conf; + struct prop_nums num; unsigned int mclk_fs; } *dai_props; struct asoc_simple_jack hp_jack; @@ -71,6 +78,7 @@ struct link_info { int link; /* number of link */ int conf; /* number of codec_conf */ int cpu; /* turn for CPU / Codec */ + struct prop_nums num[SNDRV_MINOR_DEVICES]; }; int asoc_simple_parse_daifmt(struct device *dev, diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index ee1d924d68e5..a3ca9a99fccd 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -628,6 +628,15 @@ static int graph_count_noml(struct asoc_simple_priv *priv, { struct device *dev = simple_priv_to_dev(priv); + if (li->link >= SNDRV_MINOR_DEVICES) { + dev_err(dev, "too many links\n"); + return -EINVAL; + } + + li->num[li->link].cpus = 1; + li->num[li->link].codecs = 1; + li->num[li->link].platforms = 1; + li->link += 1; /* 1xCPU-Codec */ li->dais += 2; /* 1xCPU + 1xCodec */ @@ -643,10 +652,23 @@ static int graph_count_dpcm(struct asoc_simple_priv *priv, { struct device *dev = simple_priv_to_dev(priv); + if (li->link >= SNDRV_MINOR_DEVICES) { + dev_err(dev, "too many links\n"); + return -EINVAL; + } + if (li->cpu) { + li->num[li->link].cpus = 1; + li->num[li->link].codecs = 1; + li->num[li->link].platforms = 1; + li->link++; /* 1xCPU-dummy */ li->dais++; /* 1xCPU */ } else { + li->num[li->link].cpus = 1; + li->num[li->link].codecs = 1; + li->num[li->link].platforms = 1; + li->link++; /* 1xdummy-Codec */ li->conf++; /* 1xdummy-Codec */ li->dais++; /* 1xCodec */ diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c index 3010ff63c71d..1606b9bc6b71 100644 --- a/sound/soc/generic/simple-card-utils.c +++ b/sound/soc/generic/simple-card-utils.c @@ -604,13 +604,27 @@ int asoc_simple_init_priv(struct asoc_simple_priv *priv, struct asoc_simple_dai *dais; struct snd_soc_dai_link_component *dlcs; struct snd_soc_codec_conf *cconf = NULL; - int i; + int i, dai_num = 0, dlc_num = 0; dai_props = devm_kcalloc(dev, li->link, sizeof(*dai_props), GFP_KERNEL); dai_link = devm_kcalloc(dev, li->link, sizeof(*dai_link), GFP_KERNEL); - dais = devm_kcalloc(dev, li->dais, sizeof(*dais), GFP_KERNEL); - dlcs = devm_kcalloc(dev, li->link * 3, sizeof(*dai_props), GFP_KERNEL); - if (!dai_props || !dai_link || !dais || !dlcs) + if (!dai_props || !dai_link) + return -ENOMEM; + + /* + * dais (= CPU+Codec) + * dlcs (= CPU+Codec+Platform) + */ + for (i = 0; i < li->link; i++) { + int cc = li->num[i].cpus + li->num[i].codecs; + + dai_num += cc; + dlc_num += cc + li->num[i].platforms; + } + + dais = devm_kcalloc(dev, dai_num, sizeof(*dais), GFP_KERNEL); + dlcs = devm_kcalloc(dev, dlc_num, sizeof(*dai_props), GFP_KERNEL); + if (!dais || !dlcs) return -ENOMEM; if (li->conf) { @@ -619,24 +633,6 @@ int asoc_simple_init_priv(struct asoc_simple_priv *priv, return -ENOMEM; } - /* - * "platform" might be removed - * see - * simple-card-utils.c :: asoc_simple_canonicalize_platform() - */ - for (i = 0; i < li->link; i++) { - dai_props[i].cpus = dlcs + (3 * i) + 0; - dai_props[i].codecs = dlcs + (3 * i) + 1; - dai_props[i].platforms = dlcs + (3 * i) + 2; - - dai_link[i].cpus = dai_props[i].cpus; - dai_link[i].num_cpus = 1; - dai_link[i].codecs = dai_props[i].codecs; - dai_link[i].num_codecs = 1; - dai_link[i].platforms = dai_props[i].platforms; - dai_link[i].num_platforms = 1; - } - priv->dai_props = dai_props; priv->dai_link = dai_link; priv->dais = dais; @@ -648,6 +644,38 @@ int asoc_simple_init_priv(struct asoc_simple_priv *priv, card->codec_conf = cconf; card->num_configs = li->conf; + for (i = 0; i < li->link; i++) { + if (li->num[i].cpus) { + /* Normal CPU */ + dai_props[i].cpus = + dai_link[i].cpus = dlcs; + dai_props[i].num.cpus = + dai_link[i].num_cpus = li->num[i].cpus; + + dlcs += li->num[i].cpus; + } + + if (li->num[i].codecs) { + /* Normal Codec */ + dai_props[i].codecs = + dai_link[i].codecs = dlcs; + dai_props[i].num.codecs = + dai_link[i].num_codecs = li->num[i].codecs; + + dlcs += li->num[i].codecs; + } + + if (li->num[i].platforms) { + /* Have Platform */ + dai_props[i].platforms = + dai_link[i].platforms = dlcs; + dai_props[i].num.platforms = + dai_link[i].num_platforms = li->num[i].platforms; + + dlcs += li->num[i].platforms; + } + } + return 0; } EXPORT_SYMBOL_GPL(asoc_simple_init_priv); diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 41aa40765a8d..f60e809d723b 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -499,6 +499,17 @@ static int simple_count_noml(struct asoc_simple_priv *priv, struct device_node *codec, struct link_info *li, bool is_top) { + if (li->link >= SNDRV_MINOR_DEVICES) { + struct device *dev = simple_priv_to_dev(priv); + + dev_err(dev, "too many links\n"); + return -EINVAL; + } + + li->num[li->link].cpus = 1; + li->num[li->link].codecs = 1; + li->num[li->link].platforms = 1; + li->link += 1; li->dais += 2; @@ -510,10 +521,25 @@ static int simple_count_dpcm(struct asoc_simple_priv *priv, struct device_node *codec, struct link_info *li, bool is_top) { + if (li->link >= SNDRV_MINOR_DEVICES) { + struct device *dev = simple_priv_to_dev(priv); + + dev_err(dev, "too many links\n"); + return -EINVAL; + } + if (li->cpu) { + li->num[li->link].cpus = 1; + li->num[li->link].codecs = 1; + li->num[li->link].platforms = 1; + li->link++; /* CPU-dummy */ li->dais++; } else { + li->num[li->link].cpus = 1; + li->num[li->link].codecs = 1; + li->num[li->link].platforms = 1; + li->link++; /* dummy-Codec */ li->dais++; li->conf++; @@ -575,6 +601,10 @@ static void simple_get_dais_count(struct asoc_simple_priv *priv, * => 1 ccnf = 1xdummy-Codec */ if (!top) { + li->num[0].cpus = 1; + li->num[0].codecs = 1; + li->num[0].platforms = 1; + li->link = 1; li->dais = 2; li->conf = 0;