[alsa-devel] [PATCH v1 0/3] ASoC: last minute fixes
This series comes with some last minutes fixes and clean-up.
Changes in v1: - Split from the Tegra series as suggested by Mark. - Fix issue in soc-core rather than working around it in tegra_sgtl5000.
Marcel Ziswiler (3): ASoC: tegra_sgtl5000: fix device_node refcounting ASoC: soc-core: fix trivial checkpatch issues ASoC: soc-core: fix platform name vs. of_node assignement
sound/soc/soc-core.c | 162 ++++++++++++++++++++++++--------------- sound/soc/tegra/tegra_sgtl5000.c | 17 +++- 2 files changed, 116 insertions(+), 63 deletions(-)
From: Marcel Ziswiler marcel.ziswiler@toradex.com
Similar to the following:
commit 4321723648b0 ("ASoC: tegra_alc5632: fix device_node refcounting")
commit 7c5dfd549617 ("ASoC: tegra: fix device_node refcounting")
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com Acked-by: Jon Hunter jonathanh@nvidia.com
---
Changes in v1: None
sound/soc/tegra/tegra_sgtl5000.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/sound/soc/tegra/tegra_sgtl5000.c b/sound/soc/tegra/tegra_sgtl5000.c index 45a4aa9d2a47..901457da25ec 100644 --- a/sound/soc/tegra/tegra_sgtl5000.c +++ b/sound/soc/tegra/tegra_sgtl5000.c @@ -149,14 +149,14 @@ static int tegra_sgtl5000_driver_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Property 'nvidia,i2s-controller' missing/invalid\n"); ret = -EINVAL; - goto err; + goto err_put_codec_of_node; }
tegra_sgtl5000_dai.platform_of_node = tegra_sgtl5000_dai.cpu_of_node;
ret = tegra_asoc_utils_init(&machine->util_data, &pdev->dev); if (ret) - goto err; + goto err_put_cpu_of_node;
ret = snd_soc_register_card(card); if (ret) { @@ -169,6 +169,13 @@ static int tegra_sgtl5000_driver_probe(struct platform_device *pdev)
err_fini_utils: tegra_asoc_utils_fini(&machine->util_data); +err_put_cpu_of_node: + of_node_put(tegra_sgtl5000_dai.cpu_of_node); + tegra_sgtl5000_dai.cpu_of_node = NULL; + tegra_sgtl5000_dai.platform_of_node = NULL; +err_put_codec_of_node: + of_node_put(tegra_sgtl5000_dai.codec_of_node); + tegra_sgtl5000_dai.codec_of_node = NULL; err: return ret; } @@ -183,6 +190,12 @@ static int tegra_sgtl5000_driver_remove(struct platform_device *pdev)
tegra_asoc_utils_fini(&machine->util_data);
+ of_node_put(tegra_sgtl5000_dai.cpu_of_node); + tegra_sgtl5000_dai.cpu_of_node = NULL; + tegra_sgtl5000_dai.platform_of_node = NULL; + of_node_put(tegra_sgtl5000_dai.codec_of_node); + tegra_sgtl5000_dai.codec_of_node = NULL; + return ret; }
From: Marcel Ziswiler marcel.ziswiler@toradex.com
Fix a few trivial aka cosmetic only checkpatch issues like long lines, wrong indentations, spurious blanks and newlines, missing newlines, multi-line comments etc.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
---
Changes in v1: None
sound/soc/soc-core.c | 146 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 88 insertions(+), 58 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 62e8e36062df..6ddcf12bc030 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -66,8 +66,9 @@ static int pmdown_time = 5000; module_param(pmdown_time, int, 0); MODULE_PARM_DESC(pmdown_time, "DAPM stream powerdown time (msecs)");
-/* If a DMI filed contain strings in this blacklist (e.g. - * "Type2 - Board Manufacturer" or "Type1 - TBD by OEM"), it will be taken +/* + * If a DMI filed contain strings in this blacklist (e.g. + * "Type2 - Board Manufacturer" or "Type1 - TBD by OEM"), it will be taken * as invalid and dropped when setting the card long name from DMI info. */ static const char * const dmi_blacklist[] = { @@ -222,7 +223,7 @@ static void soc_init_card_debugfs(struct snd_soc_card *card) &card->pop_time); if (!card->debugfs_pop_time) dev_warn(card->dev, - "ASoC: Failed to create pop time debugfs file\n"); + "ASoC: Failed to create pop time debugfs file\n"); }
static void soc_cleanup_card_debugfs(struct snd_soc_card *card) @@ -426,7 +427,8 @@ EXPORT_SYMBOL_GPL(snd_soc_get_pcm_runtime);
static void codec2codec_close_delayed_work(struct work_struct *work) { - /* Currently nothing to do for c2c links + /* + * Currently nothing to do for c2c links * Since c2c links are internal nodes in the DAPM graph and * don't interface with the outside world or application layer * we don't have to do any special handling on close. @@ -446,8 +448,9 @@ int snd_soc_suspend(struct device *dev) if (!card->instantiated) return 0;
- /* Due to the resume being scheduled into a workqueue we could - * suspend before that's finished - wait for it to complete. + /* + * Due to the resume being scheduled into a workqueue we could + * suspend before that's finished - wait for it to complete. */ snd_power_wait(card->snd_card, SNDRV_CTL_POWER_D0);
@@ -514,10 +517,13 @@ int snd_soc_suspend(struct device *dev)
/* suspend all COMPONENTs */ for_each_card_components(card, component) { - struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); + struct snd_soc_dapm_context *dapm = + snd_soc_component_get_dapm(component);
- /* If there are paths active then the COMPONENT will be held with - * bias _ON and should not be suspended. */ + /* + * If there are paths active then the COMPONENT will be held + * with bias _ON and should not be suspended. + */ if (!component->suspended) { switch (snd_soc_dapm_get_bias_level(dapm)) { case SND_SOC_BIAS_STANDBY: @@ -571,18 +577,21 @@ int snd_soc_suspend(struct device *dev) } EXPORT_SYMBOL_GPL(snd_soc_suspend);
-/* deferred resume work, so resume can complete before we finished +/* + * deferred resume work, so resume can complete before we finished * setting our codec back up, which can be very slow on I2C */ static void soc_resume_deferred(struct work_struct *work) { struct snd_soc_card *card = - container_of(work, struct snd_soc_card, deferred_resume_work); + container_of(work, struct snd_soc_card, + deferred_resume_work); struct snd_soc_pcm_runtime *rtd; struct snd_soc_component *component; int i;
- /* our power state is still SNDRV_CTL_POWER_D3hot from suspend time, + /* + * our power state is still SNDRV_CTL_POWER_D3hot from suspend time, * so userspace apps are blocked from touching us */
@@ -699,6 +708,7 @@ int snd_soc_resume(struct device *dev) */ for_each_card_rtds(card, rtd) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + bus_control |= cpu_dai->driver->bus_control; } if (bus_control) { @@ -777,7 +787,7 @@ struct snd_soc_dai *snd_soc_find_dai(
lockdep_assert_held(&client_mutex);
- /* Find CPU DAI from registered DAIs*/ + /* Find CPU DAI from registered DAIs */ for_each_component(component) { if (!snd_soc_is_matching_component(dlc, component)) continue; @@ -795,7 +805,6 @@ struct snd_soc_dai *snd_soc_find_dai( } EXPORT_SYMBOL_GPL(snd_soc_find_dai);
- /** * snd_soc_find_dai_link - Find a DAI link * @@ -918,7 +927,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
_err_defer: soc_free_pcm_runtime(rtd); - return -EPROBE_DEFER; + return -EPROBE_DEFER; }
static void soc_remove_component(struct snd_soc_component *component) @@ -1074,7 +1083,7 @@ static int snd_soc_init_multicodec(struct snd_soc_card *card, }
static int soc_init_dai_link(struct snd_soc_card *card, - struct snd_soc_dai_link *link) + struct snd_soc_dai_link *link) { int i, ret; struct snd_soc_dai_link_component *codec; @@ -1148,7 +1157,8 @@ static int soc_init_dai_link(struct snd_soc_card *card,
void snd_soc_disconnect_sync(struct device *dev) { - struct snd_soc_component *component = snd_soc_lookup_component(dev, NULL); + struct snd_soc_component *component = + snd_soc_lookup_component(dev, NULL);
if (!component || !component->card) return; @@ -1179,7 +1189,8 @@ int snd_soc_add_dai_link(struct snd_soc_card *card, }
lockdep_assert_held(&client_mutex); - /* Notify the machine driver for extra initialization + /* + * Notify the machine driver for extra initialization * on the link created by topology. */ if (dai_link->dobj.type && card->add_dai_link) @@ -1214,7 +1225,8 @@ void snd_soc_remove_dai_link(struct snd_soc_card *card, }
lockdep_assert_held(&client_mutex); - /* Notify the machine driver for extra destruction + /* + * Notify the machine driver for extra destruction * on the link created by topology. */ if (dai_link->dobj.type && card->remove_dai_link) @@ -1274,7 +1286,8 @@ static void soc_set_name_prefix(struct snd_soc_card *card, static int soc_probe_component(struct snd_soc_card *card, struct snd_soc_component *component) { - struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); + struct snd_soc_dapm_context *dapm = + snd_soc_component_get_dapm(component); struct snd_soc_dai *dai; int ret;
@@ -1406,8 +1419,7 @@ static int soc_post_component_init(struct snd_soc_pcm_runtime *rtd, }
static int soc_probe_link_components(struct snd_soc_card *card, - struct snd_soc_pcm_runtime *rtd, - int order) + struct snd_soc_pcm_runtime *rtd, int order) { struct snd_soc_component *component; struct snd_soc_rtdcom_list *rtdcom; @@ -1434,6 +1446,7 @@ static int soc_probe_dai(struct snd_soc_dai *dai, int order)
if (dai->driver->probe) { int ret = dai->driver->probe(dai); + if (ret < 0) { dev_err(dai->dev, "ASoC: failed to probe DAI %s: %d\n", dai->name, ret); @@ -1541,7 +1554,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, }
if (cpu_dai->driver->compress_new) { - /*create compress_device"*/ + /* create compress_device" */ ret = cpu_dai->driver->compress_new(rtd, num); if (ret < 0) { dev_err(card->dev, "ASoC: can't create compress %s\n", @@ -1555,7 +1568,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, ret = soc_new_pcm(rtd, num); if (ret < 0) { dev_err(card->dev, "ASoC: can't create pcm %s :%d\n", - dai_link->stream_name, ret); + dai_link->stream_name, ret); return ret; } ret = soc_link_dai_pcm_new(&cpu_dai, 1, rtd); @@ -1683,8 +1696,10 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd, } }
- /* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */ - /* the component which has non_legacy_dai_naming is Codec */ + /* + * Flip the polarity for the "CPU" end of a CODEC<->CODEC link + * the component which has non_legacy_dai_naming is Codec + */ if (cpu_dai->component->driver->non_legacy_dai_naming) { unsigned int inv_dai_fmt;
@@ -1718,9 +1733,9 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd, } EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
- #ifdef CONFIG_DMI -/* Trim special characters, and replace '-' with '_' since '-' is used to +/* + * Trim special characters, and replace '-' with '_' since '-' is used to * separate different DMI fields in the card long name. Only number and * alphabet characters and a few separator characters are kept. */ @@ -1739,7 +1754,8 @@ static void cleanup_dmi_name(char *name) name[j] = '\0'; }
-/* Check if a DMI field is valid, i.e. not containing any string +/* + * Check if a DMI field is valid, i.e. not containing any string * in the black list. */ static int is_dmi_valid(const char *field) @@ -1802,7 +1818,6 @@ int snd_soc_set_dmi_name(struct snd_soc_card *card, const char *flavour) return 0; }
- snprintf(card->dmi_longname, sizeof(card->snd_card->longname), "%s", vendor); cleanup_dmi_name(card->dmi_longname); @@ -1818,7 +1833,8 @@ int snd_soc_set_dmi_name(struct snd_soc_card *card, const char *flavour) if (len < longname_buf_size) cleanup_dmi_name(card->dmi_longname + len);
- /* some vendors like Lenovo may only put a self-explanatory + /* + * some vendors like Lenovo may only put a self-explanatory * name in the product version field */ product_version = dmi_get_system_info(DMI_PRODUCT_VERSION); @@ -1914,7 +1930,8 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) dai_link->be_hw_params_fixup = component->driver->be_hw_params_fixup;
- /* most BE links don't set stream name, so set it to + /* + * most BE links don't set stream name, so set it to * dai link name if it's NULL to help bind widgets. */ if (!dai_link->stream_name) @@ -1924,7 +1941,7 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) /* Inform userspace we are using alternate topology */ if (component->driver->topology_name_prefix) {
- /* topology shortname created ? */ + /* topology shortname created? */ if (!card->topology_shortname_created) { comp_drv = component->driver;
@@ -2029,7 +2046,8 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) if (ret < 0) goto probe_dai_err;
- /* Find new DAI links added during probing components and bind them. + /* + * Find new DAI links added during probing components and bind them. * Components with topology may bring new DAIs and DAI links. */ for_each_card_links(card, dai_link) { @@ -2061,7 +2079,8 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) snd_soc_dapm_connect_dai_link_widgets(card);
if (card->controls) - snd_soc_add_card_controls(card, card->controls, card->num_controls); + snd_soc_add_card_controls(card, card->controls, + card->num_controls);
if (card->dapm_routes) snd_soc_dapm_add_routes(&card->dapm, card->dapm_routes, @@ -2207,8 +2226,10 @@ int snd_soc_poweroff(struct device *dev) if (!card->instantiated) return 0;
- /* Flush out pmdown_time work - we actually do want to run it - * now, we're shutting down so no imminent restart. */ + /* + * Flush out pmdown_time work - we actually do want to run it + * now, we're shutting down so no imminent restart. + */ for_each_card_rtds(card, rtd) flush_delayed_work(&rtd->delayed_work);
@@ -2301,6 +2322,7 @@ static int snd_soc_add_controls(struct snd_card *card, struct device *dev,
for (i = 0; i < num_controls; i++) { const struct snd_kcontrol_new *control = &controls[i]; + err = snd_ctl_add(card, snd_soc_cnew(control, data, control->name, prefix)); if (err < 0) { @@ -2418,8 +2440,9 @@ EXPORT_SYMBOL_GPL(snd_soc_dai_set_sysclk); * * Configures the CODEC master (MCLK) or system (SYSCLK) clocking. */ -int snd_soc_component_set_sysclk(struct snd_soc_component *component, int clk_id, - int source, unsigned int freq, int dir) +int snd_soc_component_set_sysclk(struct snd_soc_component *component, + int clk_id, int source, unsigned int freq, + int dir) { if (component->driver->set_sysclk) return component->driver->set_sysclk(component, clk_id, source, @@ -2487,7 +2510,7 @@ int snd_soc_component_set_pll(struct snd_soc_component *component, int pll_id, { if (component->driver->set_pll) return component->driver->set_pll(component, pll_id, source, - freq_in, freq_out); + freq_in, freq_out);
return -EINVAL; } @@ -2533,8 +2556,8 @@ EXPORT_SYMBOL_GPL(snd_soc_dai_set_fmt); * Generates the TDM tx and rx slot default masks for DAI. */ static int snd_soc_xlate_tdm_slot_mask(unsigned int slots, - unsigned int *tx_mask, - unsigned int *rx_mask) + unsigned int *tx_mask, + unsigned int *rx_mask) { if (*tx_mask || *rx_mask) return 0; @@ -2684,7 +2707,7 @@ static int snd_soc_bind_card(struct snd_soc_card *card) return ret;
/* deactivate pins to sleep state */ - for_each_card_rtds(card, rtd) { + for_each_card_rtds(card, rtd) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dai *codec_dai; int j; @@ -2799,7 +2822,7 @@ static char *fmt_single_name(struct device *dev, int *id) }
} else { - /* I2C component devices are named "bus-addr" */ + /* I2C component devices are named "bus-addr" */ if (sscanf(name, "%x-%x", &id1, &id2) == 2) { char tmp[NAME_SIZE];
@@ -2807,7 +2830,8 @@ static char *fmt_single_name(struct device *dev, int *id) *id = ((id1 & 0xffff) << 16) + id2;
/* sanitize component name for DAI link creation */ - snprintf(tmp, NAME_SIZE, "%s.%s", dev->driver->name, name); + snprintf(tmp, NAME_SIZE, "%s.%s", dev->driver->name, + name); strlcpy(name, tmp, NAME_SIZE); } else *id = 0; @@ -2874,7 +2898,7 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, * component-less anymore. */ if (legacy_dai_naming && - (dai_drv->id == 0 || dai_drv->name == NULL)) { + (dai_drv->id == 0 || dai_drv->name == NULL)) { dai->name = fmt_single_name(dev, &dai->id); } else { dai->name = fmt_multiple_name(dev, dai_drv); @@ -2910,7 +2934,8 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, * @count: Number of DAIs */ static int snd_soc_register_dais(struct snd_soc_component *component, - struct snd_soc_dai_driver *dai_drv, size_t count) + struct snd_soc_dai_driver *dai_drv, + size_t count) { struct device *dev = component->dev; struct snd_soc_dai *dai; @@ -2921,8 +2946,8 @@ static int snd_soc_register_dais(struct snd_soc_component *component,
for (i = 0; i < count; i++) {
- dai = soc_add_dai(component, dai_drv + i, - count == 1 && !component->driver->non_legacy_dai_naming); + dai = soc_add_dai(component, dai_drv + i, count == 1 && + !component->driver->non_legacy_dai_naming); if (dai == NULL) { ret = -ENOMEM; goto err; @@ -2966,7 +2991,8 @@ int snd_soc_register_dai(struct snd_soc_component *component, if (!dai) return -ENOMEM;
- /* Create the DAI widgets here. After adding DAIs, topology may + /* + * Create the DAI widgets here. After adding DAIs, topology may * also add routes that need these widgets as source or sink. */ ret = snd_soc_dapm_new_dai_widgets(dapm, dai); @@ -3048,7 +3074,8 @@ static void snd_soc_component_setup_regmap(struct snd_soc_component *component) #ifdef CONFIG_REGMAP
/** - * snd_soc_component_init_regmap() - Initialize regmap instance for the component + * snd_soc_component_init_regmap() - Initialize regmap instance for the + * component * @component: The component for which to initialize the regmap instance * @regmap: The regmap instance that should be used by the component * @@ -3066,7 +3093,8 @@ void snd_soc_component_init_regmap(struct snd_soc_component *component, EXPORT_SYMBOL_GPL(snd_soc_component_init_regmap);
/** - * snd_soc_component_exit_regmap() - De-initialize regmap instance for the component + * snd_soc_component_exit_regmap() - De-initialize regmap instance for the + * component * @component: The component for which to de-initialize the regmap instance * * Calls regmap_exit() on the regmap instance associated to the component and @@ -3090,7 +3118,8 @@ static void snd_soc_component_add(struct snd_soc_component *component)
if (!component->driver->write && !component->driver->read) { if (!component->regmap) - component->regmap = dev_get_regmap(component->dev, NULL); + component->regmap = dev_get_regmap(component->dev, + NULL); if (component->regmap) snd_soc_component_setup_regmap(component); } @@ -3235,23 +3264,24 @@ static int __snd_soc_unregister_component(struct device *dev) if (dev != component->dev) continue;
- snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL); + snd_soc_tplg_component_remove(component, + SND_SOC_TPLG_INDEX_ALL); snd_soc_component_del_unlocked(component); found = 1; break; } mutex_unlock(&client_mutex);
- if (found) { + if (found) snd_soc_component_cleanup(component); - }
return found; }
void snd_soc_unregister_component(struct device *dev) { - while (__snd_soc_unregister_component(dev)); + while (__snd_soc_unregister_component(dev)) + ; } EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
@@ -3832,7 +3862,7 @@ int snd_soc_of_get_dai_link_codecs(struct device *dev, for_each_link_codecs(dai_link, index, component) { ret = of_parse_phandle_with_args(of_node, name, "#sound-dai-cells", - index, &args); + index, &args); if (ret) goto err; component->of_node = args.np;
The patch
ASoC: soc-core: fix trivial checkpatch issues
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 2c7b696a7589ab14854c132dc732973fbd498d5a Mon Sep 17 00:00:00 2001
From: Marcel Ziswiler marcel.ziswiler@toradex.com Date: Thu, 18 Oct 2018 13:18:28 +0200 Subject: [PATCH] ASoC: soc-core: fix trivial checkpatch issues
Fix a few trivial aka cosmetic only checkpatch issues like long lines, wrong indentations, spurious blanks and newlines, missing newlines, multi-line comments etc.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-core.c | 146 ++++++++++++++++++++++++++----------------- 1 file changed, 88 insertions(+), 58 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 62e8e36062df..6ddcf12bc030 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -66,8 +66,9 @@ static int pmdown_time = 5000; module_param(pmdown_time, int, 0); MODULE_PARM_DESC(pmdown_time, "DAPM stream powerdown time (msecs)");
-/* If a DMI filed contain strings in this blacklist (e.g. - * "Type2 - Board Manufacturer" or "Type1 - TBD by OEM"), it will be taken +/* + * If a DMI filed contain strings in this blacklist (e.g. + * "Type2 - Board Manufacturer" or "Type1 - TBD by OEM"), it will be taken * as invalid and dropped when setting the card long name from DMI info. */ static const char * const dmi_blacklist[] = { @@ -222,7 +223,7 @@ static void soc_init_card_debugfs(struct snd_soc_card *card) &card->pop_time); if (!card->debugfs_pop_time) dev_warn(card->dev, - "ASoC: Failed to create pop time debugfs file\n"); + "ASoC: Failed to create pop time debugfs file\n"); }
static void soc_cleanup_card_debugfs(struct snd_soc_card *card) @@ -426,7 +427,8 @@ EXPORT_SYMBOL_GPL(snd_soc_get_pcm_runtime);
static void codec2codec_close_delayed_work(struct work_struct *work) { - /* Currently nothing to do for c2c links + /* + * Currently nothing to do for c2c links * Since c2c links are internal nodes in the DAPM graph and * don't interface with the outside world or application layer * we don't have to do any special handling on close. @@ -446,8 +448,9 @@ int snd_soc_suspend(struct device *dev) if (!card->instantiated) return 0;
- /* Due to the resume being scheduled into a workqueue we could - * suspend before that's finished - wait for it to complete. + /* + * Due to the resume being scheduled into a workqueue we could + * suspend before that's finished - wait for it to complete. */ snd_power_wait(card->snd_card, SNDRV_CTL_POWER_D0);
@@ -514,10 +517,13 @@ int snd_soc_suspend(struct device *dev)
/* suspend all COMPONENTs */ for_each_card_components(card, component) { - struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); + struct snd_soc_dapm_context *dapm = + snd_soc_component_get_dapm(component);
- /* If there are paths active then the COMPONENT will be held with - * bias _ON and should not be suspended. */ + /* + * If there are paths active then the COMPONENT will be held + * with bias _ON and should not be suspended. + */ if (!component->suspended) { switch (snd_soc_dapm_get_bias_level(dapm)) { case SND_SOC_BIAS_STANDBY: @@ -571,18 +577,21 @@ int snd_soc_suspend(struct device *dev) } EXPORT_SYMBOL_GPL(snd_soc_suspend);
-/* deferred resume work, so resume can complete before we finished +/* + * deferred resume work, so resume can complete before we finished * setting our codec back up, which can be very slow on I2C */ static void soc_resume_deferred(struct work_struct *work) { struct snd_soc_card *card = - container_of(work, struct snd_soc_card, deferred_resume_work); + container_of(work, struct snd_soc_card, + deferred_resume_work); struct snd_soc_pcm_runtime *rtd; struct snd_soc_component *component; int i;
- /* our power state is still SNDRV_CTL_POWER_D3hot from suspend time, + /* + * our power state is still SNDRV_CTL_POWER_D3hot from suspend time, * so userspace apps are blocked from touching us */
@@ -699,6 +708,7 @@ int snd_soc_resume(struct device *dev) */ for_each_card_rtds(card, rtd) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + bus_control |= cpu_dai->driver->bus_control; } if (bus_control) { @@ -777,7 +787,7 @@ struct snd_soc_dai *snd_soc_find_dai(
lockdep_assert_held(&client_mutex);
- /* Find CPU DAI from registered DAIs*/ + /* Find CPU DAI from registered DAIs */ for_each_component(component) { if (!snd_soc_is_matching_component(dlc, component)) continue; @@ -795,7 +805,6 @@ struct snd_soc_dai *snd_soc_find_dai( } EXPORT_SYMBOL_GPL(snd_soc_find_dai);
- /** * snd_soc_find_dai_link - Find a DAI link * @@ -918,7 +927,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
_err_defer: soc_free_pcm_runtime(rtd); - return -EPROBE_DEFER; + return -EPROBE_DEFER; }
static void soc_remove_component(struct snd_soc_component *component) @@ -1074,7 +1083,7 @@ static int snd_soc_init_multicodec(struct snd_soc_card *card, }
static int soc_init_dai_link(struct snd_soc_card *card, - struct snd_soc_dai_link *link) + struct snd_soc_dai_link *link) { int i, ret; struct snd_soc_dai_link_component *codec; @@ -1148,7 +1157,8 @@ static int soc_init_dai_link(struct snd_soc_card *card,
void snd_soc_disconnect_sync(struct device *dev) { - struct snd_soc_component *component = snd_soc_lookup_component(dev, NULL); + struct snd_soc_component *component = + snd_soc_lookup_component(dev, NULL);
if (!component || !component->card) return; @@ -1179,7 +1189,8 @@ int snd_soc_add_dai_link(struct snd_soc_card *card, }
lockdep_assert_held(&client_mutex); - /* Notify the machine driver for extra initialization + /* + * Notify the machine driver for extra initialization * on the link created by topology. */ if (dai_link->dobj.type && card->add_dai_link) @@ -1214,7 +1225,8 @@ void snd_soc_remove_dai_link(struct snd_soc_card *card, }
lockdep_assert_held(&client_mutex); - /* Notify the machine driver for extra destruction + /* + * Notify the machine driver for extra destruction * on the link created by topology. */ if (dai_link->dobj.type && card->remove_dai_link) @@ -1274,7 +1286,8 @@ static void soc_set_name_prefix(struct snd_soc_card *card, static int soc_probe_component(struct snd_soc_card *card, struct snd_soc_component *component) { - struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); + struct snd_soc_dapm_context *dapm = + snd_soc_component_get_dapm(component); struct snd_soc_dai *dai; int ret;
@@ -1406,8 +1419,7 @@ static int soc_post_component_init(struct snd_soc_pcm_runtime *rtd, }
static int soc_probe_link_components(struct snd_soc_card *card, - struct snd_soc_pcm_runtime *rtd, - int order) + struct snd_soc_pcm_runtime *rtd, int order) { struct snd_soc_component *component; struct snd_soc_rtdcom_list *rtdcom; @@ -1434,6 +1446,7 @@ static int soc_probe_dai(struct snd_soc_dai *dai, int order)
if (dai->driver->probe) { int ret = dai->driver->probe(dai); + if (ret < 0) { dev_err(dai->dev, "ASoC: failed to probe DAI %s: %d\n", dai->name, ret); @@ -1541,7 +1554,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, }
if (cpu_dai->driver->compress_new) { - /*create compress_device"*/ + /* create compress_device" */ ret = cpu_dai->driver->compress_new(rtd, num); if (ret < 0) { dev_err(card->dev, "ASoC: can't create compress %s\n", @@ -1555,7 +1568,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, ret = soc_new_pcm(rtd, num); if (ret < 0) { dev_err(card->dev, "ASoC: can't create pcm %s :%d\n", - dai_link->stream_name, ret); + dai_link->stream_name, ret); return ret; } ret = soc_link_dai_pcm_new(&cpu_dai, 1, rtd); @@ -1683,8 +1696,10 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd, } }
- /* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */ - /* the component which has non_legacy_dai_naming is Codec */ + /* + * Flip the polarity for the "CPU" end of a CODEC<->CODEC link + * the component which has non_legacy_dai_naming is Codec + */ if (cpu_dai->component->driver->non_legacy_dai_naming) { unsigned int inv_dai_fmt;
@@ -1718,9 +1733,9 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd, } EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
- #ifdef CONFIG_DMI -/* Trim special characters, and replace '-' with '_' since '-' is used to +/* + * Trim special characters, and replace '-' with '_' since '-' is used to * separate different DMI fields in the card long name. Only number and * alphabet characters and a few separator characters are kept. */ @@ -1739,7 +1754,8 @@ static void cleanup_dmi_name(char *name) name[j] = '\0'; }
-/* Check if a DMI field is valid, i.e. not containing any string +/* + * Check if a DMI field is valid, i.e. not containing any string * in the black list. */ static int is_dmi_valid(const char *field) @@ -1802,7 +1818,6 @@ int snd_soc_set_dmi_name(struct snd_soc_card *card, const char *flavour) return 0; }
- snprintf(card->dmi_longname, sizeof(card->snd_card->longname), "%s", vendor); cleanup_dmi_name(card->dmi_longname); @@ -1818,7 +1833,8 @@ int snd_soc_set_dmi_name(struct snd_soc_card *card, const char *flavour) if (len < longname_buf_size) cleanup_dmi_name(card->dmi_longname + len);
- /* some vendors like Lenovo may only put a self-explanatory + /* + * some vendors like Lenovo may only put a self-explanatory * name in the product version field */ product_version = dmi_get_system_info(DMI_PRODUCT_VERSION); @@ -1914,7 +1930,8 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) dai_link->be_hw_params_fixup = component->driver->be_hw_params_fixup;
- /* most BE links don't set stream name, so set it to + /* + * most BE links don't set stream name, so set it to * dai link name if it's NULL to help bind widgets. */ if (!dai_link->stream_name) @@ -1924,7 +1941,7 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) /* Inform userspace we are using alternate topology */ if (component->driver->topology_name_prefix) {
- /* topology shortname created ? */ + /* topology shortname created? */ if (!card->topology_shortname_created) { comp_drv = component->driver;
@@ -2029,7 +2046,8 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) if (ret < 0) goto probe_dai_err;
- /* Find new DAI links added during probing components and bind them. + /* + * Find new DAI links added during probing components and bind them. * Components with topology may bring new DAIs and DAI links. */ for_each_card_links(card, dai_link) { @@ -2061,7 +2079,8 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) snd_soc_dapm_connect_dai_link_widgets(card);
if (card->controls) - snd_soc_add_card_controls(card, card->controls, card->num_controls); + snd_soc_add_card_controls(card, card->controls, + card->num_controls);
if (card->dapm_routes) snd_soc_dapm_add_routes(&card->dapm, card->dapm_routes, @@ -2207,8 +2226,10 @@ int snd_soc_poweroff(struct device *dev) if (!card->instantiated) return 0;
- /* Flush out pmdown_time work - we actually do want to run it - * now, we're shutting down so no imminent restart. */ + /* + * Flush out pmdown_time work - we actually do want to run it + * now, we're shutting down so no imminent restart. + */ for_each_card_rtds(card, rtd) flush_delayed_work(&rtd->delayed_work);
@@ -2301,6 +2322,7 @@ static int snd_soc_add_controls(struct snd_card *card, struct device *dev,
for (i = 0; i < num_controls; i++) { const struct snd_kcontrol_new *control = &controls[i]; + err = snd_ctl_add(card, snd_soc_cnew(control, data, control->name, prefix)); if (err < 0) { @@ -2418,8 +2440,9 @@ EXPORT_SYMBOL_GPL(snd_soc_dai_set_sysclk); * * Configures the CODEC master (MCLK) or system (SYSCLK) clocking. */ -int snd_soc_component_set_sysclk(struct snd_soc_component *component, int clk_id, - int source, unsigned int freq, int dir) +int snd_soc_component_set_sysclk(struct snd_soc_component *component, + int clk_id, int source, unsigned int freq, + int dir) { if (component->driver->set_sysclk) return component->driver->set_sysclk(component, clk_id, source, @@ -2487,7 +2510,7 @@ int snd_soc_component_set_pll(struct snd_soc_component *component, int pll_id, { if (component->driver->set_pll) return component->driver->set_pll(component, pll_id, source, - freq_in, freq_out); + freq_in, freq_out);
return -EINVAL; } @@ -2533,8 +2556,8 @@ EXPORT_SYMBOL_GPL(snd_soc_dai_set_fmt); * Generates the TDM tx and rx slot default masks for DAI. */ static int snd_soc_xlate_tdm_slot_mask(unsigned int slots, - unsigned int *tx_mask, - unsigned int *rx_mask) + unsigned int *tx_mask, + unsigned int *rx_mask) { if (*tx_mask || *rx_mask) return 0; @@ -2684,7 +2707,7 @@ static int snd_soc_bind_card(struct snd_soc_card *card) return ret;
/* deactivate pins to sleep state */ - for_each_card_rtds(card, rtd) { + for_each_card_rtds(card, rtd) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dai *codec_dai; int j; @@ -2799,7 +2822,7 @@ static char *fmt_single_name(struct device *dev, int *id) }
} else { - /* I2C component devices are named "bus-addr" */ + /* I2C component devices are named "bus-addr" */ if (sscanf(name, "%x-%x", &id1, &id2) == 2) { char tmp[NAME_SIZE];
@@ -2807,7 +2830,8 @@ static char *fmt_single_name(struct device *dev, int *id) *id = ((id1 & 0xffff) << 16) + id2;
/* sanitize component name for DAI link creation */ - snprintf(tmp, NAME_SIZE, "%s.%s", dev->driver->name, name); + snprintf(tmp, NAME_SIZE, "%s.%s", dev->driver->name, + name); strlcpy(name, tmp, NAME_SIZE); } else *id = 0; @@ -2874,7 +2898,7 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, * component-less anymore. */ if (legacy_dai_naming && - (dai_drv->id == 0 || dai_drv->name == NULL)) { + (dai_drv->id == 0 || dai_drv->name == NULL)) { dai->name = fmt_single_name(dev, &dai->id); } else { dai->name = fmt_multiple_name(dev, dai_drv); @@ -2910,7 +2934,8 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, * @count: Number of DAIs */ static int snd_soc_register_dais(struct snd_soc_component *component, - struct snd_soc_dai_driver *dai_drv, size_t count) + struct snd_soc_dai_driver *dai_drv, + size_t count) { struct device *dev = component->dev; struct snd_soc_dai *dai; @@ -2921,8 +2946,8 @@ static int snd_soc_register_dais(struct snd_soc_component *component,
for (i = 0; i < count; i++) {
- dai = soc_add_dai(component, dai_drv + i, - count == 1 && !component->driver->non_legacy_dai_naming); + dai = soc_add_dai(component, dai_drv + i, count == 1 && + !component->driver->non_legacy_dai_naming); if (dai == NULL) { ret = -ENOMEM; goto err; @@ -2966,7 +2991,8 @@ int snd_soc_register_dai(struct snd_soc_component *component, if (!dai) return -ENOMEM;
- /* Create the DAI widgets here. After adding DAIs, topology may + /* + * Create the DAI widgets here. After adding DAIs, topology may * also add routes that need these widgets as source or sink. */ ret = snd_soc_dapm_new_dai_widgets(dapm, dai); @@ -3048,7 +3074,8 @@ static void snd_soc_component_setup_regmap(struct snd_soc_component *component) #ifdef CONFIG_REGMAP
/** - * snd_soc_component_init_regmap() - Initialize regmap instance for the component + * snd_soc_component_init_regmap() - Initialize regmap instance for the + * component * @component: The component for which to initialize the regmap instance * @regmap: The regmap instance that should be used by the component * @@ -3066,7 +3093,8 @@ void snd_soc_component_init_regmap(struct snd_soc_component *component, EXPORT_SYMBOL_GPL(snd_soc_component_init_regmap);
/** - * snd_soc_component_exit_regmap() - De-initialize regmap instance for the component + * snd_soc_component_exit_regmap() - De-initialize regmap instance for the + * component * @component: The component for which to de-initialize the regmap instance * * Calls regmap_exit() on the regmap instance associated to the component and @@ -3090,7 +3118,8 @@ static void snd_soc_component_add(struct snd_soc_component *component)
if (!component->driver->write && !component->driver->read) { if (!component->regmap) - component->regmap = dev_get_regmap(component->dev, NULL); + component->regmap = dev_get_regmap(component->dev, + NULL); if (component->regmap) snd_soc_component_setup_regmap(component); } @@ -3235,23 +3264,24 @@ static int __snd_soc_unregister_component(struct device *dev) if (dev != component->dev) continue;
- snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL); + snd_soc_tplg_component_remove(component, + SND_SOC_TPLG_INDEX_ALL); snd_soc_component_del_unlocked(component); found = 1; break; } mutex_unlock(&client_mutex);
- if (found) { + if (found) snd_soc_component_cleanup(component); - }
return found; }
void snd_soc_unregister_component(struct device *dev) { - while (__snd_soc_unregister_component(dev)); + while (__snd_soc_unregister_component(dev)) + ; } EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
@@ -3832,7 +3862,7 @@ int snd_soc_of_get_dai_link_codecs(struct device *dev, for_each_link_codecs(dai_link, index, component) { ret = of_parse_phandle_with_args(of_node, name, "#sound-dai-cells", - index, &args); + index, &args); if (ret) goto err; component->of_node = args.np;
The patch
ASoC: soc-core: fix trivial checkpatch issues
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 2c7b696a7589ab14854c132dc732973fbd498d5a Mon Sep 17 00:00:00 2001
From: Marcel Ziswiler marcel.ziswiler@toradex.com Date: Thu, 18 Oct 2018 13:18:28 +0200 Subject: [PATCH] ASoC: soc-core: fix trivial checkpatch issues
Fix a few trivial aka cosmetic only checkpatch issues like long lines, wrong indentations, spurious blanks and newlines, missing newlines, multi-line comments etc.
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-core.c | 146 ++++++++++++++++++++++++++----------------- 1 file changed, 88 insertions(+), 58 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 62e8e36062df..6ddcf12bc030 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -66,8 +66,9 @@ static int pmdown_time = 5000; module_param(pmdown_time, int, 0); MODULE_PARM_DESC(pmdown_time, "DAPM stream powerdown time (msecs)");
-/* If a DMI filed contain strings in this blacklist (e.g. - * "Type2 - Board Manufacturer" or "Type1 - TBD by OEM"), it will be taken +/* + * If a DMI filed contain strings in this blacklist (e.g. + * "Type2 - Board Manufacturer" or "Type1 - TBD by OEM"), it will be taken * as invalid and dropped when setting the card long name from DMI info. */ static const char * const dmi_blacklist[] = { @@ -222,7 +223,7 @@ static void soc_init_card_debugfs(struct snd_soc_card *card) &card->pop_time); if (!card->debugfs_pop_time) dev_warn(card->dev, - "ASoC: Failed to create pop time debugfs file\n"); + "ASoC: Failed to create pop time debugfs file\n"); }
static void soc_cleanup_card_debugfs(struct snd_soc_card *card) @@ -426,7 +427,8 @@ EXPORT_SYMBOL_GPL(snd_soc_get_pcm_runtime);
static void codec2codec_close_delayed_work(struct work_struct *work) { - /* Currently nothing to do for c2c links + /* + * Currently nothing to do for c2c links * Since c2c links are internal nodes in the DAPM graph and * don't interface with the outside world or application layer * we don't have to do any special handling on close. @@ -446,8 +448,9 @@ int snd_soc_suspend(struct device *dev) if (!card->instantiated) return 0;
- /* Due to the resume being scheduled into a workqueue we could - * suspend before that's finished - wait for it to complete. + /* + * Due to the resume being scheduled into a workqueue we could + * suspend before that's finished - wait for it to complete. */ snd_power_wait(card->snd_card, SNDRV_CTL_POWER_D0);
@@ -514,10 +517,13 @@ int snd_soc_suspend(struct device *dev)
/* suspend all COMPONENTs */ for_each_card_components(card, component) { - struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); + struct snd_soc_dapm_context *dapm = + snd_soc_component_get_dapm(component);
- /* If there are paths active then the COMPONENT will be held with - * bias _ON and should not be suspended. */ + /* + * If there are paths active then the COMPONENT will be held + * with bias _ON and should not be suspended. + */ if (!component->suspended) { switch (snd_soc_dapm_get_bias_level(dapm)) { case SND_SOC_BIAS_STANDBY: @@ -571,18 +577,21 @@ int snd_soc_suspend(struct device *dev) } EXPORT_SYMBOL_GPL(snd_soc_suspend);
-/* deferred resume work, so resume can complete before we finished +/* + * deferred resume work, so resume can complete before we finished * setting our codec back up, which can be very slow on I2C */ static void soc_resume_deferred(struct work_struct *work) { struct snd_soc_card *card = - container_of(work, struct snd_soc_card, deferred_resume_work); + container_of(work, struct snd_soc_card, + deferred_resume_work); struct snd_soc_pcm_runtime *rtd; struct snd_soc_component *component; int i;
- /* our power state is still SNDRV_CTL_POWER_D3hot from suspend time, + /* + * our power state is still SNDRV_CTL_POWER_D3hot from suspend time, * so userspace apps are blocked from touching us */
@@ -699,6 +708,7 @@ int snd_soc_resume(struct device *dev) */ for_each_card_rtds(card, rtd) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + bus_control |= cpu_dai->driver->bus_control; } if (bus_control) { @@ -777,7 +787,7 @@ struct snd_soc_dai *snd_soc_find_dai(
lockdep_assert_held(&client_mutex);
- /* Find CPU DAI from registered DAIs*/ + /* Find CPU DAI from registered DAIs */ for_each_component(component) { if (!snd_soc_is_matching_component(dlc, component)) continue; @@ -795,7 +805,6 @@ struct snd_soc_dai *snd_soc_find_dai( } EXPORT_SYMBOL_GPL(snd_soc_find_dai);
- /** * snd_soc_find_dai_link - Find a DAI link * @@ -918,7 +927,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
_err_defer: soc_free_pcm_runtime(rtd); - return -EPROBE_DEFER; + return -EPROBE_DEFER; }
static void soc_remove_component(struct snd_soc_component *component) @@ -1074,7 +1083,7 @@ static int snd_soc_init_multicodec(struct snd_soc_card *card, }
static int soc_init_dai_link(struct snd_soc_card *card, - struct snd_soc_dai_link *link) + struct snd_soc_dai_link *link) { int i, ret; struct snd_soc_dai_link_component *codec; @@ -1148,7 +1157,8 @@ static int soc_init_dai_link(struct snd_soc_card *card,
void snd_soc_disconnect_sync(struct device *dev) { - struct snd_soc_component *component = snd_soc_lookup_component(dev, NULL); + struct snd_soc_component *component = + snd_soc_lookup_component(dev, NULL);
if (!component || !component->card) return; @@ -1179,7 +1189,8 @@ int snd_soc_add_dai_link(struct snd_soc_card *card, }
lockdep_assert_held(&client_mutex); - /* Notify the machine driver for extra initialization + /* + * Notify the machine driver for extra initialization * on the link created by topology. */ if (dai_link->dobj.type && card->add_dai_link) @@ -1214,7 +1225,8 @@ void snd_soc_remove_dai_link(struct snd_soc_card *card, }
lockdep_assert_held(&client_mutex); - /* Notify the machine driver for extra destruction + /* + * Notify the machine driver for extra destruction * on the link created by topology. */ if (dai_link->dobj.type && card->remove_dai_link) @@ -1274,7 +1286,8 @@ static void soc_set_name_prefix(struct snd_soc_card *card, static int soc_probe_component(struct snd_soc_card *card, struct snd_soc_component *component) { - struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); + struct snd_soc_dapm_context *dapm = + snd_soc_component_get_dapm(component); struct snd_soc_dai *dai; int ret;
@@ -1406,8 +1419,7 @@ static int soc_post_component_init(struct snd_soc_pcm_runtime *rtd, }
static int soc_probe_link_components(struct snd_soc_card *card, - struct snd_soc_pcm_runtime *rtd, - int order) + struct snd_soc_pcm_runtime *rtd, int order) { struct snd_soc_component *component; struct snd_soc_rtdcom_list *rtdcom; @@ -1434,6 +1446,7 @@ static int soc_probe_dai(struct snd_soc_dai *dai, int order)
if (dai->driver->probe) { int ret = dai->driver->probe(dai); + if (ret < 0) { dev_err(dai->dev, "ASoC: failed to probe DAI %s: %d\n", dai->name, ret); @@ -1541,7 +1554,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, }
if (cpu_dai->driver->compress_new) { - /*create compress_device"*/ + /* create compress_device" */ ret = cpu_dai->driver->compress_new(rtd, num); if (ret < 0) { dev_err(card->dev, "ASoC: can't create compress %s\n", @@ -1555,7 +1568,7 @@ static int soc_probe_link_dais(struct snd_soc_card *card, ret = soc_new_pcm(rtd, num); if (ret < 0) { dev_err(card->dev, "ASoC: can't create pcm %s :%d\n", - dai_link->stream_name, ret); + dai_link->stream_name, ret); return ret; } ret = soc_link_dai_pcm_new(&cpu_dai, 1, rtd); @@ -1683,8 +1696,10 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd, } }
- /* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */ - /* the component which has non_legacy_dai_naming is Codec */ + /* + * Flip the polarity for the "CPU" end of a CODEC<->CODEC link + * the component which has non_legacy_dai_naming is Codec + */ if (cpu_dai->component->driver->non_legacy_dai_naming) { unsigned int inv_dai_fmt;
@@ -1718,9 +1733,9 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd, } EXPORT_SYMBOL_GPL(snd_soc_runtime_set_dai_fmt);
- #ifdef CONFIG_DMI -/* Trim special characters, and replace '-' with '_' since '-' is used to +/* + * Trim special characters, and replace '-' with '_' since '-' is used to * separate different DMI fields in the card long name. Only number and * alphabet characters and a few separator characters are kept. */ @@ -1739,7 +1754,8 @@ static void cleanup_dmi_name(char *name) name[j] = '\0'; }
-/* Check if a DMI field is valid, i.e. not containing any string +/* + * Check if a DMI field is valid, i.e. not containing any string * in the black list. */ static int is_dmi_valid(const char *field) @@ -1802,7 +1818,6 @@ int snd_soc_set_dmi_name(struct snd_soc_card *card, const char *flavour) return 0; }
- snprintf(card->dmi_longname, sizeof(card->snd_card->longname), "%s", vendor); cleanup_dmi_name(card->dmi_longname); @@ -1818,7 +1833,8 @@ int snd_soc_set_dmi_name(struct snd_soc_card *card, const char *flavour) if (len < longname_buf_size) cleanup_dmi_name(card->dmi_longname + len);
- /* some vendors like Lenovo may only put a self-explanatory + /* + * some vendors like Lenovo may only put a self-explanatory * name in the product version field */ product_version = dmi_get_system_info(DMI_PRODUCT_VERSION); @@ -1914,7 +1930,8 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) dai_link->be_hw_params_fixup = component->driver->be_hw_params_fixup;
- /* most BE links don't set stream name, so set it to + /* + * most BE links don't set stream name, so set it to * dai link name if it's NULL to help bind widgets. */ if (!dai_link->stream_name) @@ -1924,7 +1941,7 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) /* Inform userspace we are using alternate topology */ if (component->driver->topology_name_prefix) {
- /* topology shortname created ? */ + /* topology shortname created? */ if (!card->topology_shortname_created) { comp_drv = component->driver;
@@ -2029,7 +2046,8 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) if (ret < 0) goto probe_dai_err;
- /* Find new DAI links added during probing components and bind them. + /* + * Find new DAI links added during probing components and bind them. * Components with topology may bring new DAIs and DAI links. */ for_each_card_links(card, dai_link) { @@ -2061,7 +2079,8 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) snd_soc_dapm_connect_dai_link_widgets(card);
if (card->controls) - snd_soc_add_card_controls(card, card->controls, card->num_controls); + snd_soc_add_card_controls(card, card->controls, + card->num_controls);
if (card->dapm_routes) snd_soc_dapm_add_routes(&card->dapm, card->dapm_routes, @@ -2207,8 +2226,10 @@ int snd_soc_poweroff(struct device *dev) if (!card->instantiated) return 0;
- /* Flush out pmdown_time work - we actually do want to run it - * now, we're shutting down so no imminent restart. */ + /* + * Flush out pmdown_time work - we actually do want to run it + * now, we're shutting down so no imminent restart. + */ for_each_card_rtds(card, rtd) flush_delayed_work(&rtd->delayed_work);
@@ -2301,6 +2322,7 @@ static int snd_soc_add_controls(struct snd_card *card, struct device *dev,
for (i = 0; i < num_controls; i++) { const struct snd_kcontrol_new *control = &controls[i]; + err = snd_ctl_add(card, snd_soc_cnew(control, data, control->name, prefix)); if (err < 0) { @@ -2418,8 +2440,9 @@ EXPORT_SYMBOL_GPL(snd_soc_dai_set_sysclk); * * Configures the CODEC master (MCLK) or system (SYSCLK) clocking. */ -int snd_soc_component_set_sysclk(struct snd_soc_component *component, int clk_id, - int source, unsigned int freq, int dir) +int snd_soc_component_set_sysclk(struct snd_soc_component *component, + int clk_id, int source, unsigned int freq, + int dir) { if (component->driver->set_sysclk) return component->driver->set_sysclk(component, clk_id, source, @@ -2487,7 +2510,7 @@ int snd_soc_component_set_pll(struct snd_soc_component *component, int pll_id, { if (component->driver->set_pll) return component->driver->set_pll(component, pll_id, source, - freq_in, freq_out); + freq_in, freq_out);
return -EINVAL; } @@ -2533,8 +2556,8 @@ EXPORT_SYMBOL_GPL(snd_soc_dai_set_fmt); * Generates the TDM tx and rx slot default masks for DAI. */ static int snd_soc_xlate_tdm_slot_mask(unsigned int slots, - unsigned int *tx_mask, - unsigned int *rx_mask) + unsigned int *tx_mask, + unsigned int *rx_mask) { if (*tx_mask || *rx_mask) return 0; @@ -2684,7 +2707,7 @@ static int snd_soc_bind_card(struct snd_soc_card *card) return ret;
/* deactivate pins to sleep state */ - for_each_card_rtds(card, rtd) { + for_each_card_rtds(card, rtd) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dai *codec_dai; int j; @@ -2799,7 +2822,7 @@ static char *fmt_single_name(struct device *dev, int *id) }
} else { - /* I2C component devices are named "bus-addr" */ + /* I2C component devices are named "bus-addr" */ if (sscanf(name, "%x-%x", &id1, &id2) == 2) { char tmp[NAME_SIZE];
@@ -2807,7 +2830,8 @@ static char *fmt_single_name(struct device *dev, int *id) *id = ((id1 & 0xffff) << 16) + id2;
/* sanitize component name for DAI link creation */ - snprintf(tmp, NAME_SIZE, "%s.%s", dev->driver->name, name); + snprintf(tmp, NAME_SIZE, "%s.%s", dev->driver->name, + name); strlcpy(name, tmp, NAME_SIZE); } else *id = 0; @@ -2874,7 +2898,7 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, * component-less anymore. */ if (legacy_dai_naming && - (dai_drv->id == 0 || dai_drv->name == NULL)) { + (dai_drv->id == 0 || dai_drv->name == NULL)) { dai->name = fmt_single_name(dev, &dai->id); } else { dai->name = fmt_multiple_name(dev, dai_drv); @@ -2910,7 +2934,8 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, * @count: Number of DAIs */ static int snd_soc_register_dais(struct snd_soc_component *component, - struct snd_soc_dai_driver *dai_drv, size_t count) + struct snd_soc_dai_driver *dai_drv, + size_t count) { struct device *dev = component->dev; struct snd_soc_dai *dai; @@ -2921,8 +2946,8 @@ static int snd_soc_register_dais(struct snd_soc_component *component,
for (i = 0; i < count; i++) {
- dai = soc_add_dai(component, dai_drv + i, - count == 1 && !component->driver->non_legacy_dai_naming); + dai = soc_add_dai(component, dai_drv + i, count == 1 && + !component->driver->non_legacy_dai_naming); if (dai == NULL) { ret = -ENOMEM; goto err; @@ -2966,7 +2991,8 @@ int snd_soc_register_dai(struct snd_soc_component *component, if (!dai) return -ENOMEM;
- /* Create the DAI widgets here. After adding DAIs, topology may + /* + * Create the DAI widgets here. After adding DAIs, topology may * also add routes that need these widgets as source or sink. */ ret = snd_soc_dapm_new_dai_widgets(dapm, dai); @@ -3048,7 +3074,8 @@ static void snd_soc_component_setup_regmap(struct snd_soc_component *component) #ifdef CONFIG_REGMAP
/** - * snd_soc_component_init_regmap() - Initialize regmap instance for the component + * snd_soc_component_init_regmap() - Initialize regmap instance for the + * component * @component: The component for which to initialize the regmap instance * @regmap: The regmap instance that should be used by the component * @@ -3066,7 +3093,8 @@ void snd_soc_component_init_regmap(struct snd_soc_component *component, EXPORT_SYMBOL_GPL(snd_soc_component_init_regmap);
/** - * snd_soc_component_exit_regmap() - De-initialize regmap instance for the component + * snd_soc_component_exit_regmap() - De-initialize regmap instance for the + * component * @component: The component for which to de-initialize the regmap instance * * Calls regmap_exit() on the regmap instance associated to the component and @@ -3090,7 +3118,8 @@ static void snd_soc_component_add(struct snd_soc_component *component)
if (!component->driver->write && !component->driver->read) { if (!component->regmap) - component->regmap = dev_get_regmap(component->dev, NULL); + component->regmap = dev_get_regmap(component->dev, + NULL); if (component->regmap) snd_soc_component_setup_regmap(component); } @@ -3235,23 +3264,24 @@ static int __snd_soc_unregister_component(struct device *dev) if (dev != component->dev) continue;
- snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL); + snd_soc_tplg_component_remove(component, + SND_SOC_TPLG_INDEX_ALL); snd_soc_component_del_unlocked(component); found = 1; break; } mutex_unlock(&client_mutex);
- if (found) { + if (found) snd_soc_component_cleanup(component); - }
return found; }
void snd_soc_unregister_component(struct device *dev) { - while (__snd_soc_unregister_component(dev)); + while (__snd_soc_unregister_component(dev)) + ; } EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
@@ -3832,7 +3862,7 @@ int snd_soc_of_get_dai_link_codecs(struct device *dev, for_each_link_codecs(dai_link, index, component) { ret = of_parse_phandle_with_args(of_node, name, "#sound-dai-cells", - index, &args); + index, &args); if (ret) goto err; component->of_node = args.np;
From: Marcel Ziswiler marcel.ziswiler@toradex.com
This fixes the following error as seen post commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform") on Apalis TK1 after initial probe deferral:
tegra-snd-sgtl5000 sound: ASoC: Both platform name/of_node are set for sgtl5000 tegra-snd-sgtl5000 sound: ASoC: failed to init link sgtl5000 tegra-snd-sgtl5000 sound: snd_soc_register_card failed (-22) tegra-snd-sgtl5000: probe of sound failed with error -22
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
---
Changes in v1: - Split from the Tegra series as suggested by Mark. - Fix issue in soc-core rather than working around it in tegra_sgtl5000.
sound/soc/soc-core.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 6ddcf12bc030..b97624005976 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2733,7 +2733,7 @@ static int snd_soc_bind_card(struct snd_soc_card *card) int snd_soc_register_card(struct snd_soc_card *card) { int i, ret; - struct snd_soc_dai_link *link; + struct snd_soc_dai_link *link = NULL;
if (!card->name || !card->dev) return -EINVAL; @@ -2744,7 +2744,7 @@ int snd_soc_register_card(struct snd_soc_card *card) if (ret) { dev_err(card->dev, "ASoC: failed to init link %s\n", link->name); - return ret; + goto err; } }
@@ -2763,7 +2763,17 @@ int snd_soc_register_card(struct snd_soc_card *card) mutex_init(&card->mutex); mutex_init(&card->dapm_mutex);
- return snd_soc_bind_card(card); + ret = snd_soc_bind_card(card); + if (ret) + goto err; + + return 0; + +err: + if (link && link->platform) + link->platform = NULL; + + return ret; } EXPORT_SYMBOL_GPL(snd_soc_register_card);
On 18/10/2018 12:18, Marcel Ziswiler wrote:
From: Marcel Ziswiler marcel.ziswiler@toradex.com
This fixes the following error as seen post commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform") on Apalis TK1 after initial probe deferral:
tegra-snd-sgtl5000 sound: ASoC: Both platform name/of_node are set for sgtl5000 tegra-snd-sgtl5000 sound: ASoC: failed to init link sgtl5000 tegra-snd-sgtl5000 sound: snd_soc_register_card failed (-22) tegra-snd-sgtl5000: probe of sound failed with error -22
Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
Changes in v1:
- Split from the Tegra series as suggested by Mark.
- Fix issue in soc-core rather than working around it in tegra_sgtl5000.
sound/soc/soc-core.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 6ddcf12bc030..b97624005976 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2733,7 +2733,7 @@ static int snd_soc_bind_card(struct snd_soc_card *card) int snd_soc_register_card(struct snd_soc_card *card) { int i, ret;
- struct snd_soc_dai_link *link;
struct snd_soc_dai_link *link = NULL;
if (!card->name || !card->dev) return -EINVAL;
@@ -2744,7 +2744,7 @@ int snd_soc_register_card(struct snd_soc_card *card) if (ret) { dev_err(card->dev, "ASoC: failed to init link %s\n", link->name);
return ret;
goto err;@@ -2763,7 +2763,17 @@ int snd_soc_register_card(struct snd_soc_card *card) mutex_init(&card->mutex); mutex_init(&card->dapm_mutex);
- return snd_soc_bind_card(card);
- ret = snd_soc_bind_card(card);
- if (ret)
goto err;- return 0;
+err:
- if (link && link->platform)
link->platform = NULL;
Looking at snd_soc_init_platform(), it seems that the platform pointer can be allocated by the machine driver and so if it is not allocated by the core, then I don't think we should clear it here. Seems we need a way to determine if this was allocated by the core.
Furthermore, it seems that it is possible that there is more than one link that might be to be cleared.
Cheers Jon
On Fri, Oct 19, 2018 at 11:22:46AM +0100, Jon Hunter wrote:
Looking at snd_soc_init_platform(), it seems that the platform pointer can be allocated by the machine driver and so if it is not allocated by the core, then I don't think we should clear it here. Seems we need a way to determine if this was allocated by the core.
Indeed, this is a bit of a mess. We probably shouldn't be modifying the data that the drivers passed in, otherwise we get into trouble like this. That suggests that we should copy the data, probably all of it. I will try to have a proper look at this next week.
Furthermore, it seems that it is possible that there is more than one link that might be to be cleared.
Yes, that's an issue as well.
Hi Mark,
On Sun, Oct 21, 2018 at 12:23:01PM +0100, Mark Brown wrote:
On Fri, Oct 19, 2018 at 11:22:46AM +0100, Jon Hunter wrote:
Looking at snd_soc_init_platform(), it seems that the platform pointer can be allocated by the machine driver and so if it is not allocated by the core, then I don't think we should clear it here. Seems we need a way to determine if this was allocated by the core.
Indeed, this is a bit of a mess. We probably shouldn't be modifying the data that the drivers passed in, otherwise we get into trouble like this. That suggests that we should copy the data, probably all of it. I will try to have a proper look at this next week.
did you find the time to look into this?
The downstream Raspberry Pi kernel contains a bunch of machine drivers that are implemented in a similar way as the tegra_sgtl5000 driver (static card and dai link structs, dai_link->platform_of_node filled in from device tree) which are breaking in 4.20 on deferred probing.
Switching these drivers to dynamically allocated dai link structs, like 76836fd35492 "ASoC: omap-abe-twl6040: Fix missing audio card caused by deferred probing" would be a possibility, but if there's some solution on the horizon that doesn't require changes to the driver code it'd be easier to wait for that.
so long,
Hias
Furthermore, it seems that it is possible that there is more than one link that might be to be cleared.
Yes, that's an issue as well.
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Mark, Kuninori
On 18/12/2018 17:40, Matthias Reichl wrote:
Hi Mark,
On Sun, Oct 21, 2018 at 12:23:01PM +0100, Mark Brown wrote:
On Fri, Oct 19, 2018 at 11:22:46AM +0100, Jon Hunter wrote:
Looking at snd_soc_init_platform(), it seems that the platform pointer can be allocated by the machine driver and so if it is not allocated by the core, then I don't think we should clear it here. Seems we need a way to determine if this was allocated by the core.
Indeed, this is a bit of a mess. We probably shouldn't be modifying the data that the drivers passed in, otherwise we get into trouble like this. That suggests that we should copy the data, probably all of it. I will try to have a proper look at this next week.
did you find the time to look into this?
The downstream Raspberry Pi kernel contains a bunch of machine drivers that are implemented in a similar way as the tegra_sgtl5000 driver (static card and dai link structs, dai_link->platform_of_node filled in from device tree) which are breaking in 4.20 on deferred probing.
Switching these drivers to dynamically allocated dai link structs, like 76836fd35492 "ASoC: omap-abe-twl6040: Fix missing audio card caused by deferred probing" would be a possibility, but if there's some solution on the horizon that doesn't require changes to the driver code it'd be easier to wait for that.
so long,
Hias
Furthermore, it seems that it is possible that there is more than one link that might be to be cleared.
Yes, that's an issue as well.
I have been looking at this again recently. I see this issue occurring all the time when the sound drivers are built as kernel modules and probing the sound card is deferred until the codec driver has been loaded.
Commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform") appears to introduce the problem because now we allocate the 'snd_soc_dai_link_component' structure for the platform we attempt to register the soundcard but we never clear the freed pointer on failure. Therefore, we only actually allocate it the first time. There is no easy way to clear this pointer for the memory allocated because this is done before the dai-links have been added to the list of dai-links for the soundcard.
I don't see an easy solution that will be 100% robust unless you do opt for copying all the dai-link info from the platform (but this is probably not a trivial fix).
Do you envision a fix any time soon, or should we be updating all the machine drivers to populate the platform snd_soc_dai_link_component so that it is handled by the machine drivers are not the core?
Cheers Jon
Hi Jon
I have been looking at this again recently. I see this issue occurring all the time when the sound drivers are built as kernel modules and probing the sound card is deferred until the codec driver has been loaded.
Commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform") appears to introduce the problem because now we allocate the 'snd_soc_dai_link_component' structure for the platform we attempt to register the soundcard but we never clear the freed pointer on failure. Therefore, we only actually allocate it the first time. There is no easy way to clear this pointer for the memory allocated because this is done before the dai-links have been added to the list of dai-links for the soundcard.
I don't see an easy solution that will be 100% robust unless you do opt for copying all the dai-link info from the platform (but this is probably not a trivial fix).
Do you envision a fix any time soon, or should we be updating all the machine drivers to populate the platform snd_soc_dai_link_component so that it is handled by the machine drivers are not the core?
Thank you for pointing it. Indeed it is mess. I think coping info is nice idea, but it is not easy so far, and it uses much memory...
I didn't test this, but can below patch solve your issue ? I think same issue happen on codec side too, so it cares it too.
--------------- diff --git a/include/sound/soc.h b/include/sound/soc.h index 8ec1de8..49ac5a8 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -985,6 +985,10 @@ struct snd_soc_dai_link { /* Do not create a PCM for this DAI link (Backend link) */ unsigned int ignore:1;
+ /* allocated dai_link_comonent. These should be removed in the future */ + unsigned int allocated_platform:1; + unsigned int allocated_codecs:1; + struct list_head list; /* DAI link list of the soc card */ struct snd_soc_dobj dobj; /* For topology */ }; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 0462b3e..49ccea3 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1023,6 +1023,25 @@ static void soc_remove_dai_links(struct snd_soc_card *card) } }
+static void snd_soc_init_dai_link_component(struct snd_soc_card *card) +{ + struct snd_soc_dai_link *dai_link; + int i; + + /* + * FIXME + * + * this function should be removed in the future + */ + for_each_card_prelinks(card, i, dai_link) { + /* see snd_soc_init_platform */ + if (dai_link->allocated_platform) + dai_link->platform = NULL; + if (dai_link->allocated_codecs) + dai_link->codecs = NULL; + } +} + static int snd_soc_init_platform(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { @@ -1042,6 +1061,8 @@ static int snd_soc_init_platform(struct snd_soc_card *card, return -ENOMEM;
dai_link->platform = platform; + dai_link->allocated_platform = 1; + platform->name = dai_link->platform_name; platform->of_node = dai_link->platform_of_node; platform->dai_name = NULL; @@ -1069,6 +1090,8 @@ static int snd_soc_init_multicodec(struct snd_soc_card *card, if (!dai_link->codecs) return -ENOMEM;
+ dai_link->allocated_codecs = 1; + dai_link->codecs[0].name = dai_link->codec_name; dai_link->codecs[0].of_node = dai_link->codec_of_node; dai_link->codecs[0].dai_name = dai_link->codec_dai_name; @@ -2739,6 +2762,8 @@ int snd_soc_register_card(struct snd_soc_card *card) if (!card->name || !card->dev) return -EINVAL;
+ snd_soc_init_dai_link_component(card); + for_each_card_prelinks(card, i, link) {
ret = soc_init_dai_link(card, link); ---------------
Best regards --- Kuninori Morimoto
Hi Kuninori,
On 08/01/2019 02:25, Kuninori Morimoto wrote:
Hi Jon
I have been looking at this again recently. I see this issue occurring all the time when the sound drivers are built as kernel modules and probing the sound card is deferred until the codec driver has been loaded.
Commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform") appears to introduce the problem because now we allocate the 'snd_soc_dai_link_component' structure for the platform we attempt to register the soundcard but we never clear the freed pointer on failure. Therefore, we only actually allocate it the first time. There is no easy way to clear this pointer for the memory allocated because this is done before the dai-links have been added to the list of dai-links for the soundcard.
I don't see an easy solution that will be 100% robust unless you do opt for copying all the dai-link info from the platform (but this is probably not a trivial fix).
Do you envision a fix any time soon, or should we be updating all the machine drivers to populate the platform snd_soc_dai_link_component so that it is handled by the machine drivers are not the core?
Thank you for pointing it. Indeed it is mess. I think coping info is nice idea, but it is not easy so far, and it uses much memory...
I didn't test this, but can below patch solve your issue ?
I will give it a try and let you know.
I think same issue happen on codec side too, so it cares it too.
diff --git a/include/sound/soc.h b/include/sound/soc.h index 8ec1de8..49ac5a8 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -985,6 +985,10 @@ struct snd_soc_dai_link { /* Do not create a PCM for this DAI link (Backend link) */ unsigned int ignore:1;
- /* allocated dai_link_comonent. These should be removed in the future */
- unsigned int allocated_platform:1;
- unsigned int allocated_codecs:1;
You should add a comment here to state that these should not be modified by the machine driver and are private to the sound core.
struct list_head list; /* DAI link list of the soc card */ struct snd_soc_dobj dobj; /* For topology */ }; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 0462b3e..49ccea3 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1023,6 +1023,25 @@ static void soc_remove_dai_links(struct snd_soc_card *card) } }
+static void snd_soc_init_dai_link_component(struct snd_soc_card *card) +{
- struct snd_soc_dai_link *dai_link;
- int i;
- /*
* FIXME** this function should be removed in the future*/- for_each_card_prelinks(card, i, dai_link) {
/* see snd_soc_init_platform */if (dai_link->allocated_platform)dai_link->platform = NULL;if (dai_link->allocated_codecs)dai_link->codecs = NULL;- }
+}
It is still a little fragile because there is nothing to prevent a machine driver doing the wrong thing and setting these when they should not.
static int snd_soc_init_platform(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { @@ -1042,6 +1061,8 @@ static int snd_soc_init_platform(struct snd_soc_card *card, return -ENOMEM;
dai_link->platform = platform;
dai_link->allocated_platform = 1;- platform->name = dai_link->platform_name; platform->of_node = dai_link->platform_of_node; platform->dai_name = NULL;
@@ -1069,6 +1090,8 @@ static int snd_soc_init_multicodec(struct snd_soc_card *card, if (!dai_link->codecs) return -ENOMEM;
dai_link->allocated_codecs = 1;- dai_link->codecs[0].name = dai_link->codec_name; dai_link->codecs[0].of_node = dai_link->codec_of_node; dai_link->codecs[0].dai_name = dai_link->codec_dai_name;
@@ -2739,6 +2762,8 @@ int snd_soc_register_card(struct snd_soc_card *card) if (!card->name || !card->dev) return -EINVAL;
snd_soc_init_dai_link_component(card);
for_each_card_prelinks(card, i, link) {
ret = soc_init_dai_link(card, link);
I still question if the platform link component needs to be allocated and why it cannot be in the DAI link structure? If it was static, then ...
1. If the dai_link->platform_name or dai_link->platform_of_node are populated and the platform->name/of_node are not populated then use platform_name/of_node as the platform->name/of_node.
2. If the dai_link->platform_name or dai_link->platform_of_node are not populated and the platform->name/of_node are populated then there is nothing to do, just use platform->name/of_node.
3. If the dai_link->platform_name or dai_link->platform_of_node are populated and the platform->name/of_node are populated then WARN and default to the platform_name/of_node as the platform->name/of_node.
Could this work?
Cheers Jon
On 08/01/2019 10:50, Jon Hunter wrote:
Hi Kuninori,
On 08/01/2019 02:25, Kuninori Morimoto wrote:
Hi Jon
I have been looking at this again recently. I see this issue occurring all the time when the sound drivers are built as kernel modules and probing the sound card is deferred until the codec driver has been loaded.
Commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform") appears to introduce the problem because now we allocate the 'snd_soc_dai_link_component' structure for the platform we attempt to register the soundcard but we never clear the freed pointer on failure. Therefore, we only actually allocate it the first time. There is no easy way to clear this pointer for the memory allocated because this is done before the dai-links have been added to the list of dai-links for the soundcard.
I don't see an easy solution that will be 100% robust unless you do opt for copying all the dai-link info from the platform (but this is probably not a trivial fix).
Do you envision a fix any time soon, or should we be updating all the machine drivers to populate the platform snd_soc_dai_link_component so that it is handled by the machine drivers are not the core?
Thank you for pointing it. Indeed it is mess. I think coping info is nice idea, but it is not easy so far, and it uses much memory...
I didn't test this, but can below patch solve your issue ?
I will give it a try and let you know.
Yes so this does workaround the problem. However, per my previous comments, I would like to explore whether it is necessary to allocate the platform link component or if it can be static.
Cheers Jon
On 08/01/2019 12:03, Jon Hunter wrote:
On 08/01/2019 10:50, Jon Hunter wrote:
Hi Kuninori,
On 08/01/2019 02:25, Kuninori Morimoto wrote:
Hi Jon
I have been looking at this again recently. I see this issue occurring all the time when the sound drivers are built as kernel modules and probing the sound card is deferred until the codec driver has been loaded.
Commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for platform") appears to introduce the problem because now we allocate the 'snd_soc_dai_link_component' structure for the platform we attempt to register the soundcard but we never clear the freed pointer on failure. Therefore, we only actually allocate it the first time. There is no easy way to clear this pointer for the memory allocated because this is done before the dai-links have been added to the list of dai-links for the soundcard.
I don't see an easy solution that will be 100% robust unless you do opt for copying all the dai-link info from the platform (but this is probably not a trivial fix).
Do you envision a fix any time soon, or should we be updating all the machine drivers to populate the platform snd_soc_dai_link_component so that it is handled by the machine drivers are not the core?
Thank you for pointing it. Indeed it is mess. I think coping info is nice idea, but it is not easy so far, and it uses much memory...
I didn't test this, but can below patch solve your issue ?
I will give it a try and let you know.
Yes so this does workaround the problem. However, per my previous comments, I would like to explore whether it is necessary to allocate the platform link component or if it can be static.
To be specific, the following also works ...
--- include/sound/simple_card_utils.h | 2 +- include/sound/soc.h | 2 +- sound/soc/generic/audio-graph-card.c | 4 +++- sound/soc/generic/simple-card-utils.c | 4 ++-- sound/soc/generic/simple-card.c | 6 ++++-- sound/soc/soc-core.c | 18 +++++++----------- 6 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index 6d69ed2bd7b1..78273b81ef82 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -75,7 +75,7 @@ void asoc_simple_card_clk_disable(struct asoc_simple_dai *dai); &dai_link->codec_dai_name, \ list_name, cells_name, NULL) #define asoc_simple_card_parse_platform(node, dai_link, list_name, cells_name) \ - asoc_simple_card_parse_dai(node, dai_link->platform, \ + asoc_simple_card_parse_dai(node, &dai_link->platform, \ &dai_link->platform_of_node, \ NULL, list_name, cells_name, NULL) int asoc_simple_card_parse_dai(struct device_node *node, diff --git a/include/sound/soc.h b/include/sound/soc.h index 8ec1de856ee7..8b7ffc60006a 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -925,7 +925,7 @@ struct snd_soc_dai_link { */ const char *platform_name; struct device_node *platform_of_node; - struct snd_soc_dai_link_component *platform; + struct snd_soc_dai_link_component platform;
int id; /* optional ID for machine driver link identification */
diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 3ec96cdc683b..e961d45ce141 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -687,7 +687,9 @@ static int graph_probe(struct platform_device *pdev) for (i = 0; i < li.link; i++) { dai_link[i].codecs = &dai_props[i].codecs; dai_link[i].num_codecs = 1; - dai_link[i].platform = &dai_props[i].platform; + dai_link[i].platform.name = dai_props[i].platform.name; + dai_link[i].platform.of_node = dai_props[i].platform.of_node; + dai_link[i].platform.dai_name = dai_props[i].platform.dai_name; }
priv->pa_gpio = devm_gpiod_get_optional(dev, "pa", GPIOD_OUT_LOW); diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c index 336895f7fd1e..74910c7841ec 100644 --- a/sound/soc/generic/simple-card-utils.c +++ b/sound/soc/generic/simple-card-utils.c @@ -397,8 +397,8 @@ EXPORT_SYMBOL_GPL(asoc_simple_card_init_dai); int asoc_simple_card_canonicalize_dailink(struct snd_soc_dai_link *dai_link) { /* Assumes platform == cpu */ - if (!dai_link->platform->of_node) - dai_link->platform->of_node = dai_link->cpu_of_node; + if (!dai_link->platform.of_node) + dai_link->platform.of_node = dai_link->cpu_of_node;
return 0;
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 479de236e694..b6402e09bba2 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -732,7 +732,9 @@ static int simple_probe(struct platform_device *pdev) for (i = 0; i < li.link; i++) { dai_link[i].codecs = &dai_props[i].codecs; dai_link[i].num_codecs = 1; - dai_link[i].platform = &dai_props[i].platform; + dai_link[i].platform.name = dai_props[i].platform.name; + dai_link[i].platform.of_node = dai_props[i].platform.of_node; + dai_link[i].platform.dai_name = dai_props[i].platform.dai_name; }
priv->dai_props = dai_props; @@ -782,7 +784,7 @@ static int simple_probe(struct platform_device *pdev) codecs->name = cinfo->codec; codecs->dai_name = cinfo->codec_dai.name;
- platform = dai_link->platform; + platform = &dai_link->platform; platform->name = cinfo->platform;
card->name = (cinfo->card) ? cinfo->card : cinfo->name; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 0462b3ec977a..466099995e44 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -915,7 +915,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
/* find one from the set of registered platforms */ for_each_component(component) { - if (!snd_soc_is_matching_component(dai_link->platform, + if (!snd_soc_is_matching_component(&dai_link->platform, component)) continue;
@@ -1026,7 +1026,7 @@ static void soc_remove_dai_links(struct snd_soc_card *card) static int snd_soc_init_platform(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { - struct snd_soc_dai_link_component *platform = dai_link->platform; + struct snd_soc_dai_link_component *platform = &dai_link->platform;
/* * FIXME @@ -1034,14 +1034,10 @@ static int snd_soc_init_platform(struct snd_soc_card *card, * this function should be removed in the future */ /* convert Legacy platform link */ - if (!platform) { - platform = devm_kzalloc(card->dev, - sizeof(struct snd_soc_dai_link_component), - GFP_KERNEL); - if (!platform) - return -ENOMEM; + if (dai_link->platform_name || dai_link->platform_of_node) { + dev_dbg(card->dev, + "ASoC: Defaulting to legacy platform data!\n");
- dai_link->platform = platform; platform->name = dai_link->platform_name; platform->of_node = dai_link->platform_of_node; platform->dai_name = NULL; @@ -1123,7 +1119,7 @@ static int soc_init_dai_link(struct snd_soc_card *card, * Platform may be specified by either name or OF node, but * can be left unspecified, and a dummy platform will be used. */ - if (link->platform->name && link->platform->of_node) { + if (link->platform.name && link->platform.of_node) { dev_err(card->dev, "ASoC: Both platform name/of_node are set for %s\n", link->name); @@ -1921,7 +1917,7 @@ static void soc_check_tplg_fes(struct snd_soc_card *card) dev_err(card->dev, "init platform error"); continue; } - dai_link->platform->name = component->name; + dai_link->platform.name = component->name;
/* convert non BE into BE */ dai_link->no_pcm = 1;
On Tue, Jan 08, 2019 at 03:48:27PM +0000, Jon Hunter wrote:
Yes so this does workaround the problem. However, per my previous comments, I would like to explore whether it is necessary to allocate the platform link component or if it can be static.
To be specific, the following also works ...
Thanks for working on this, that looks nice and simple - can you send it as a real patch please (not reviewed properly yet but...)?
Hi Jon
Thank you for your help
Yes so this does workaround the problem. However, per my previous comments, I would like to explore whether it is necessary to allocate the platform link component or if it can be static.
OK, thanks. It *will* be static, but not yet. Thank you for your patch. I guess you worry about allocated memory leak when failed case ? It is using devm_kzalloc(), so all allocated memory will be automatically freed when card was detached. But indeed if driver gots -EPROBE_DEFER many times, it will allocate many unused platform.
Here is the background of snd_soc_init_platform.
Legacy dai_link was xxx_name/xxx_of_node style, but multi codec support starts to use snd_soc_dai_link_component style. OTOH Lars-Petter is thinking that current ALSA SoC is not good match for modern sound device. I guess, we need "multi xxx" support as 1st step for modern sound device. "multi codec" is already supported, "multi cpu" patch was posted, but not yet accepted (or rejected ??). "multi platform" is no plan (?). These want to use snd_soc_dai_link_component style, because it is nice for multi xxx support style, I think. I think no one is planing for "multi platform" so far, thus, I posted snd_soc_dai_link_component style for it. # Maybe it should have num_platform, too. # all driver will have .num_platform = 1, thus I didn't added. # maybe it was my fault...
The reason why platform/codec is allocating/copying by snd_soc_init_xxx so far is that it is glue for xxx_name/xxx_of_node (legacy style) <-> snd_soc_init_platform (modern style).
I want to which to modern style immediately and remove legacy style. But as you know, we have too many ALSA SoC drivers now. So, if I posted "switch legacy style to modern style" patch for each (= for codec, for platform, for cpu), it will be patch-bomb, and Lars-Petter/Mark don't like it. Thus, I'm waiting "multi CPU" support patch.
If CPU/Codec/Platform can be snd_soc_init_platform style, then, we can switch to modern style for all drivers. Then, all driver will have *static* platform.
# So, I guess if your driver can switch to use # snd_soc_init_platform style directly, your problem can gone ?
Mark
If there is no plan for "multi CPU" so far, I can post snd_soc_dai_link_component style for CPU. and post switch modern style for all drivers. Then, this issue can be solved ?
Best regards --- Kuninori Morimoto
On 09/01/2019 01:51, Kuninori Morimoto wrote:
Hi Jon
Thank you for your help
Yes so this does workaround the problem. However, per my previous comments, I would like to explore whether it is necessary to allocate the platform link component or if it can be static.
OK, thanks. It *will* be static, but not yet. Thank you for your patch. I guess you worry about allocated memory leak when failed case ? It is using devm_kzalloc(), so all allocated memory will be automatically freed when card was detached. But indeed if driver gots -EPROBE_DEFER many times, it will allocate many unused platform.
No. Offline you had suggested using kmalloc and not devm_kzalloc and so I was worried in that case about a memory leak. Right now I am only concerned about an invalid pointer that is not being handled correctly.
Here is the background of snd_soc_init_platform.
Legacy dai_link was xxx_name/xxx_of_node style, but multi codec support starts to use snd_soc_dai_link_component style. OTOH Lars-Petter is thinking that current ALSA SoC is not good match for modern sound device. I guess, we need "multi xxx" support as 1st step for modern sound device. "multi codec" is already supported, "multi cpu" patch was posted, but not yet accepted (or rejected ??). "multi platform" is no plan (?).
I would like someone to explain what multi-platform means? Even if a soundcard supports multiple platforms, there is only one platform you are using at any time so ...
These want to use snd_soc_dai_link_component style, because it is nice for multi xxx support style, I think. I think no one is planing for "multi platform" so far, thus, I posted snd_soc_dai_link_component style for it. # Maybe it should have num_platform, too. # all driver will have .num_platform = 1, thus I didn't added. # maybe it was my fault...
... I don't understand why you would ever need a 'num_platform' as the machine driver just needs to understand which platform is using it at any given time. Right?
The reason why platform/codec is allocating/copying by snd_soc_init_xxx so far is that it is glue for xxx_name/xxx_of_node (legacy style) <-> snd_soc_init_platform (modern style).
I want to which to modern style immediately and remove legacy style. But as you know, we have too many ALSA SoC drivers now. So, if I posted "switch legacy style to modern style" patch for each (= for codec, for platform, for cpu), it will be patch-bomb, and Lars-Petter/Mark don't like it. Thus, I'm waiting "multi CPU" support patch.
Sorry, I still don't understand the dependency on the multi CPU and why we need to wait.
If CPU/Codec/Platform can be snd_soc_init_platform style, then, we can switch to modern style for all drivers. Then, all driver will have *static* platform.
# So, I guess if your driver can switch to use # snd_soc_init_platform style directly, your problem can gone ?
Yes that is an alternative and I can convert all the Tegra machine drivers to use this now. However, that will not solve the problem for non-Tegra devices and everyone will have to do this.
Cheers Jon
On Wed, Jan 09, 2019 at 11:03:44AM +0000, Jon Hunter wrote:
On 09/01/2019 01:51, Kuninori Morimoto wrote:
"multi platform" is no plan (?).
I would like someone to explain what multi-platform means? Even if a soundcard supports multiple platforms, there is only one platform you are using at any time so ...
Platform in this case means DMA driver (for historical reasons I've never entirely understood the DMA drivers got called the platform drivers). It is possible that a system might have multiple DMA implementations in a single sound card but fortunately we don't seem to run into that.
I want to which to modern style immediately and remove legacy style. But as you know, we have too many ALSA SoC drivers now. So, if I posted "switch legacy style to modern style" patch for each (= for codec, for platform, for cpu), it will be patch-bomb, and Lars-Petter/Mark don't like it. Thus, I'm waiting "multi CPU" support patch.
Sorry, I still don't understand the dependency on the multi CPU and why we need to wait.
I believe Morimoto-san's concern is to minimize the number of refactorings of the drivers as that gets disruptive. That's definitely a valid concern but we can't postpone fixing bugs over releases, we need things to work for people.
If CPU/Codec/Platform can be snd_soc_init_platform style, then, we can switch to modern style for all drivers. Then, all driver will have *static* platform.
# So, I guess if your driver can switch to use # snd_soc_init_platform style directly, your problem can gone ?
Yes that is an alternative and I can convert all the Tegra machine drivers to use this now. However, that will not solve the problem for non-Tegra devices and everyone will have to do this.
We're going to have to go through another round of conversions that touch everything at some point no matter what :/
On 09/01/2019 12:53, Mark Brown wrote:
...
Yes that is an alternative and I can convert all the Tegra machine drivers to use this now. However, that will not solve the problem for non-Tegra devices and everyone will have to do this.
We're going to have to go through another round of conversions that touch everything at some point no matter what :/
Do you have a preference here? Do you think that we can fix-up the soc-core or should I go ahead and migrate the Tegra machine driver to workaround this issue now?
Cheers Jon
On Wed, Jan 09, 2019 at 02:11:58PM +0000, Jon Hunter wrote:
On 09/01/2019 12:53, Mark Brown wrote:
Yes that is an alternative and I can convert all the Tegra machine drivers to use this now. However, that will not solve the problem for non-Tegra devices and everyone will have to do this.
We're going to have to go through another round of conversions that touch everything at some point no matter what :/
Do you have a preference here? Do you think that we can fix-up the soc-core or should I go ahead and migrate the Tegra machine driver to workaround this issue now?
We're going to need to migrate Tegra regardless so it'd be good to do that whatever happens, I'm intending to try to properly review the patch today.
Hi Mark, Jon
No. Offline you had suggested using kmalloc and not devm_kzalloc and so I was worried in that case about a memory leak. Right now I am only concerned about an invalid pointer that is not being handled correctly.
I'm sorry I was confused/misunderstood, kmalloc idea was wrong.
I would like someone to explain what multi-platform means? Even if a soundcard supports multiple platforms, there is only one platform you are using at any time so ...
(snip)
... I don't understand why you would ever need a 'num_platform' as the machine driver just needs to understand which platform is using it at any given time. Right?
As Mark explained, "platform" on ALSA SoC means "DMA", and we might have multiple DMA sound sysytem (= multi-platform) in the future. Currently, all driver/sysytem is using single DMA for 1 sound card.
# So, I guess if your driver can switch to use # snd_soc_init_platform style directly, your problem can gone ?
Yes that is an alternative and I can convert all the Tegra machine drivers to use this now. However, that will not solve the problem for non-Tegra devices and everyone will have to do this.
Yeah I agree. But my concern is that the same problem happen on codec side too, by same logic, because snd_soc_init_multicodec() is overwriting dai_link too. Your posted patch solved platform side only, I think.
Yes that is an alternative and I can convert all the Tegra machine drivers to use this now. However, that will not solve the problem for non-Tegra devices and everyone will have to do this.
We're going to have to go through another round of conversions that touch everything at some point no matter what :/
Do you have a preference here? Do you think that we can fix-up the soc-core or should I go ahead and migrate the Tegra machine driver to workaround this issue now?
We're going to need to migrate Tegra regardless so it'd be good to do that whatever happens, I'm intending to try to properly review the patch today.
As I mentioned above, I think we have same issue on codec side too. exchanging *platform to platform doesn't solve all issues. And we need to exchange all driver again if we had multi-platform support in the future (I don't know when it can happen though...)
My posted quick-patch can solve "dirty pointer" issue, but it can't solve "memory leak" issue. This issue will be solved if all driver can switch to modern style, but it needs more time. Are these correct ?
So, how about this ?
I will try to add snd_soc_dai_link_component support for CPU, and switch all driver to use modern style for v5.1 (or v5.2 ?). Until then, as temporary solution, we can use above quick-patch style.
And to avoid "memory leak crash" attach, it temporary have bind dai_link limitation (max 5time?). If it goes to max limitation, ALSA SoC doesn't allow to try again. In such case, all related CPU/Codec driver need to rmmod/unbind, and insmod/bind again. Then, the limitation will be 0 cleared. You can try bind again.
It can solve "dirty pointer" issue, "memory leak" issue, and "memory leak attack" issue. The problem is that code can be dirty temporary. But it will be removed if all driver can be swtich to modern style.
Best regards --- Kuninori Morimoto
Hi Mark, Jon again
My posted quick-patch can solve "dirty pointer" issue, but it can't solve "memory leak" issue. This issue will be solved if all driver can switch to modern style, but it needs more time. Are these correct ?
Sorry I was very confused. I think the issue is only "dirty pointer", there is no "memory leak" issue (= devm_kzalloc()).
And the things Jon is worry about is that why we need to have platform pointer. And the answer is we need multi platform support in the future (pointer is prepare for it). Are these correct ?
If so my posted patch can solve all issues ?
Best regards --- Kuninori Morimoto
On 10/01/2019 01:16, Kuninori Morimoto wrote:
...
As I mentioned above, I think we have same issue on codec side too.
Actually no. Looking at snd_soc_init_multicodec() it always allocates memory if any of the 'legacy' codec members (codec_name/codec_of_node/codec_dai_name) are populated. However, this looks quite fragile to me and is susceptible to leaking memory if the user/machine driver already incorrectly allocated the memory as well as populating these legacy codec members.
My concern about all of this is it is not fool proof and hard to detect if a machine driver is doing something bad that it should not.
exchanging *platform to platform doesn't solve all issues. And we need to exchange all driver again if we had multi-platform support in the future (I don't know when it can happen though...)
My posted quick-patch can solve "dirty pointer" issue, but it can't solve "memory leak" issue. This issue will be solved if all driver can switch to modern style, but it needs more time. Are these correct ?
So, how about this ?
It is still fragile. Again the machine driver could have incorrectly set these 'allocated_platform/codecs' members as they are exposed to the machine driver. You have no way to determine if the machine driver is doing the correct thing or not. The problem becomes more complex with probe deferral.
Cheers Jon
Hi Jon
Actually no. Looking at snd_soc_init_multicodec() it always allocates memory if any of the 'legacy' codec members (codec_name/codec_of_node/codec_dai_name) are populated. However, this looks quite fragile to me and is susceptible to leaking memory if the user/machine driver already incorrectly allocated the memory as well as populating these legacy codec members.
Yeah, sorry I was 100% misunderstood. I thought there is a case that defered card connected to unbind_card_list, and try snd_soc_init_platform/codec again without freeing memory... very mess
My concern about all of this is it is not fool proof and hard to detect if a machine driver is doing something bad that it should not.
Yeah, agree. Best solution is removing legacy style, I think.
It is still fragile. Again the machine driver could have incorrectly set these 'allocated_platform/codecs' members as they are exposed to the machine driver. You have no way to determine if the machine driver is doing the correct thing or not. The problem becomes more complex with probe deferral.
Indeed there is such case so far, but my understanding is that current driver should select "legacy style" or "modern style". If driver setup it as "legacy", but access to "modern" member, it is driver side bug, right ?
Best regards --- Kuninori Morimoto
On 11/01/2019 00:52, Kuninori Morimoto wrote:
...
It is still fragile. Again the machine driver could have incorrectly set these 'allocated_platform/codecs' members as they are exposed to the machine driver. You have no way to determine if the machine driver is doing the correct thing or not. The problem becomes more complex with probe deferral.
Indeed there is such case so far, but my understanding is that current driver should select "legacy style" or "modern style". If driver setup it as "legacy", but access to "modern" member, it is driver side bug, right ?
Yes absolutely it is a driver bug, but looking at the snd_soc_dai_link structure today it is not clear what the driver should be setting and what is 'modern' and what is 'legacy'. You need to dig through the git history and code to figure this out. So you could say it is not very well documented/commented from a soc-core perspective and could be easy for a driver writer to get themselves in a pickle/mess. Anyway, that is easy to fix and we could add some comments to clear it up.
Cheers Jon
Hi Jon
Indeed there is such case so far, but my understanding is that current driver should select "legacy style" or "modern style". If driver setup it as "legacy", but access to "modern" member, it is driver side bug, right ?
Yes absolutely it is a driver bug, but looking at the snd_soc_dai_link structure today it is not clear what the driver should be setting and what is 'modern' and what is 'legacy'. You need to dig through the git history and code to figure this out. So you could say it is not very well documented/commented from a soc-core perspective and could be easy for a driver writer to get themselves in a pickle/mess. Anyway, that is easy to fix and we could add some comments to clear it up.
Thank you for your feedback. Yes, indeed there is no enough information/documentation about legacy/modern style, and its plan (= all driver will be switched to modern, legacy will be removed, etc, etc..).
So, can you agree about these ? 1) Add enough information/documentation about legacy/modern style and its plan. 2) Add dirty pointer fixup patch as workaround 3) switch to modern style as much as possible
1) and 2) are needed immediately. 3) needs more time, but we can try
Best regards --- Kuninori Morimoto
On 11/01/2019 08:51, Kuninori Morimoto wrote:
Indeed there is such case so far, but my understanding is that current driver should select "legacy style" or "modern style". If driver setup it as "legacy", but access to "modern" member, it is driver side bug, right ?
Yes absolutely it is a driver bug, but looking at the snd_soc_dai_link structure today it is not clear what the driver should be setting and what is 'modern' and what is 'legacy'. You need to dig through the git history and code to figure this out. So you could say it is not very well documented/commented from a soc-core perspective and could be easy for a driver writer to get themselves in a pickle/mess. Anyway, that is easy to fix and we could add some comments to clear it up.
Thank you for your feedback. Yes, indeed there is no enough information/documentation about legacy/modern style, and its plan (= all driver will be switched to modern, legacy will be removed, etc, etc..).
So, can you agree about these ?
- Add enough information/documentation about legacy/modern style and its plan.
- Add dirty pointer fixup patch as workaround
- switch to modern style as much as possible
I think that Mark needs to decided on whether use your 'dirty pointer' fix or not.
Jon
On Fri, Jan 11, 2019 at 09:15:42AM +0000, Jon Hunter wrote:
On 11/01/2019 08:51, Kuninori Morimoto wrote:
So, can you agree about these ?
- Add enough information/documentation about legacy/modern style and its plan.
- Add dirty pointer fixup patch as workaround
- switch to modern style as much as possible
I think that Mark needs to decided on whether use your 'dirty pointer' fix or not.
I've gone ahead with Curtis' version of Morimoto-san's patch just now - hopefully that's fine for v5.0. Like we've been saying neither approach is ideal, thanks Jon and Morimoto-san for your efforts analyzing this and your proposals.
On 14/01/2019 23:02, Mark Brown wrote:
On Fri, Jan 11, 2019 at 09:15:42AM +0000, Jon Hunter wrote:
On 11/01/2019 08:51, Kuninori Morimoto wrote:
So, can you agree about these ?
- Add enough information/documentation about legacy/modern style and its plan.
- Add dirty pointer fixup patch as workaround
- switch to modern style as much as possible
I think that Mark needs to decided on whether use your 'dirty pointer' fix or not.
I've gone ahead with Curtis' version of Morimoto-san's patch just now - hopefully that's fine for v5.0. Like we've been saying neither approach is ideal, thanks Jon and Morimoto-san for your efforts analyzing this and your proposals.
Thanks! Works for me.
Jon
On Tue, Jan 15, 2019 at 03:26:42PM +0000, Jon Hunter wrote:
On 14/01/2019 23:02, Mark Brown wrote:
On Fri, Jan 11, 2019 at 09:15:42AM +0000, Jon Hunter wrote:
On 11/01/2019 08:51, Kuninori Morimoto wrote:
So, can you agree about these ?
- Add enough information/documentation about legacy/modern style and its plan.
- Add dirty pointer fixup patch as workaround
- switch to modern style as much as possible
I think that Mark needs to decided on whether use your 'dirty pointer' fix or not.
I've gone ahead with Curtis' version of Morimoto-san's patch just now - hopefully that's fine for v5.0. Like we've been saying neither approach is ideal, thanks Jon and Morimoto-san for your efforts analyzing this and your proposals.
Thanks! Works for me.
Thanks alot, works here, too on 4.20 and 5.0-rc2!
so long,
Hias
On Tue, Dec 18, 2018 at 06:40:40PM +0100, Matthias Reichl wrote:
On Sun, Oct 21, 2018 at 12:23:01PM +0100, Mark Brown wrote:
On Fri, Oct 19, 2018 at 11:22:46AM +0100, Jon Hunter wrote:
Indeed, this is a bit of a mess. We probably shouldn't be modifying the data that the drivers passed in, otherwise we get into trouble like this. That suggests that we should copy the data, probably all of it. I will try to have a proper look at this next week.
did you find the time to look into this?
No, the end of the year turned out to be really busy unfortunately.
participants (5)
-
Jon Hunter -
Kuninori Morimoto -
Marcel Ziswiler -
Mark Brown -
Matthias Reichl