On 5/13/20 10:36 AM, Keyon Jie wrote:
On 5/13/20 3:40 AM, Pierre-Louis Bossart wrote:
On 5/12/20 1:23 PM, Keyon Jie wrote:
As we have check the 'pass' in the soc_elem_pass_load(), so no need to check it again in each specific elem_load function, at the same time, the tplg->pos will be reset to the next header base when the pass is mismatched, so the increasing of the tplg->pos in these cases made no sense. Here remove all of them.
Sorry Keyon, I am not comfortable with the changes. This started as a removal of a debug log and we ended-up changing the execution flow in one of the more complex parts of the ASoC framework. I spent 30mn looking at the code and I don't have a feeling all corner cases are covered. The bar for core changes is higher, it'd help to have additional explanations or evidence that your proposal was tested extensively. You may be right but we should be careful with topology changes.
Maybe I should send these 2 patches separately, this 2nd one is actually fix for a long existed issue and it is exposed to me when writing the 1st commit:
- the check of "if (tplg->pass != SOC_TPLG_PASS_xxx" was good and it
acted as the correct filter of the pass, but 2. the increment "tplg->pos += le32_to_cpu(hdr->size) + le32_to_cpu(hdr->payload_size);" was actually wrong when it the pass is unmatched in #1, it should be "tplg->pos += le32_to_cpu(hdr->payload_size);" as the "hdr->size" was already calculated in the upper layer in soc_tplg_load_header(): "tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);". 3. the exited mistake described in #2 didn't triggered any real problems as the "tplg->pos" will be reset to new value in soc_tplg_load_header() when the next header comes, so increasing the "tplg->pos" here at the end of the current header check actually make no sense, we can just remove them.
The code changes here might make you nervous but the logic here is not that complicated, if you rule out the issue I mentioned above in #2 and #3, the remained changes here is only removing the checks "if (tplg->pass != SOC_TPLG_PASS_xxx" as they are already covered by the 1st commit.
I have tested these changes with 3 typical SOF topology files (nocodec, tgl i2s + dsm, hda-generic) and it works all fine there and the correct header information are logged with the changes, agree we should test more extensively and widely, so tests on SST driver are very welcomed.
+ Cezary from the SST driver side.
Hi Cezary, can you help to check this from the SST driver side?
Thanks, ~Keyon
So in short, I did these changes together since I am already here, but if you and Mark preferred, I can split out the fix for the "tplg->pos" increment and send it ahead.
Thanks, ~Keyon
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
sound/soc/soc-topology.c | 46 ++-------------------------------------- 1 file changed, 2 insertions(+), 44 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index b3dae319c108..682ffaa85e9e 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -246,8 +246,8 @@ static inline void soc_control_err(struct soc_tplg *tplg, } /* pass vendor data to component driver for processing */ -static int soc_tplg_vendor_load_(struct soc_tplg *tplg, - struct snd_soc_tplg_hdr *hdr) +static int soc_tplg_vendor_load(struct soc_tplg *tplg, + struct snd_soc_tplg_hdr *hdr) { int ret = 0; @@ -268,16 +268,6 @@ static int soc_tplg_vendor_load_(struct soc_tplg *tplg, return ret; } -/* pass vendor data to component driver for processing */ -static int soc_tplg_vendor_load(struct soc_tplg *tplg, - struct snd_soc_tplg_hdr *hdr) -{ - if (tplg->pass != SOC_TPLG_PASS_VENDOR) - return 0;
- return soc_tplg_vendor_load_(tplg, hdr); -}
/* optionally pass new dynamic widget to component driver. This is mainly for * external widgets where we can assign private data/ops */ static int soc_tplg_widget_load(struct soc_tplg *tplg, @@ -1127,12 +1117,6 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, int ret; int i; - if (tplg->pass != SOC_TPLG_PASS_MIXER) { - tplg->pos += le32_to_cpu(hdr->size) + - le32_to_cpu(hdr->payload_size); - return 0; - }
dev_dbg(tplg->dev, "ASoC: adding %d kcontrols at 0x%lx\n", hdr->count, soc_tplg_get_offset(tplg)); @@ -1204,14 +1188,6 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg, count = le32_to_cpu(hdr->count); - if (tplg->pass != SOC_TPLG_PASS_GRAPH) { - tplg->pos += - le32_to_cpu(hdr->size) + - le32_to_cpu(hdr->payload_size);
- return 0; - }
if (soc_tplg_check_elem_count(tplg, sizeof(struct snd_soc_tplg_dapm_graph_elem), count, le32_to_cpu(hdr->payload_size), "graph")) { @@ -1741,9 +1717,6 @@ static int soc_tplg_dapm_widget_elems_load(struct soc_tplg *tplg, count = le32_to_cpu(hdr->count); - if (tplg->pass != SOC_TPLG_PASS_WIDGET) - return 0;
dev_dbg(tplg->dev, "ASoC: adding %d DAPM widgets\n", count); for (i = 0; i < count; i++) { @@ -2101,9 +2074,6 @@ static int soc_tplg_pcm_elems_load(struct soc_tplg *tplg, count = le32_to_cpu(hdr->count); - if (tplg->pass != SOC_TPLG_PASS_PCM_DAI) - return 0;
/* check the element size and count */ pcm = (struct snd_soc_tplg_pcm *)tplg->pos; size = le32_to_cpu(pcm->size); @@ -2386,12 +2356,6 @@ static int soc_tplg_link_elems_load(struct soc_tplg *tplg, count = le32_to_cpu(hdr->count); - if (tplg->pass != SOC_TPLG_PASS_LINK) { - tplg->pos += le32_to_cpu(hdr->size) + - le32_to_cpu(hdr->payload_size); - return 0; - }
/* check the element size and count */ link = (struct snd_soc_tplg_link_config *)tplg->pos; size = le32_to_cpu(link->size); @@ -2528,9 +2492,6 @@ static int soc_tplg_dai_elems_load(struct soc_tplg *tplg, count = le32_to_cpu(hdr->count); - if (tplg->pass != SOC_TPLG_PASS_BE_DAI) - return 0;
/* config the existing BE DAIs */ for (i = 0; i < count; i++) { dai = (struct snd_soc_tplg_dai *)tplg->pos; @@ -2610,9 +2571,6 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg, bool abi_match; int ret = 0; - if (tplg->pass != SOC_TPLG_PASS_MANIFEST) - return 0;
manifest = (struct snd_soc_tplg_manifest *)tplg->pos; /* check ABI version by size, create a new manifest if abi not match */