| Message ID | 20200509082248.2757-1-yang.jie@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: topology: log all headers being parsed | expand |
Kind reminder to always CC: maintainers.... On 5/9/20 3:22 AM, Keyon Jie wrote: > The check (tplg->pass == le32_to_cpu(hdr->type)) makes no sense as it is > comparing two different enums, remove the check and log all headers that > being parsed. It's true that the TYPE_MIXER should probably not be compared to TPLG_PASS_MANIFEST, but one would think that e.g. the TYPE_MIXER is handled in the TPLG_PASS_MIXER, no? and likewise that in the TPLG_PASS_MANIFEST all TPLG_TYPE_MANIFEST are handled? Shouldn't you add a switch case to reconcile the two lists instead of removing the test altogether? #define SND_SOC_TPLG_TYPE_MIXER 1 #define SND_SOC_TPLG_TYPE_BYTES 2 #define SND_SOC_TPLG_TYPE_ENUM 3 #define SND_SOC_TPLG_TYPE_DAPM_GRAPH 4 #define SND_SOC_TPLG_TYPE_DAPM_WIDGET 5 #define SND_SOC_TPLG_TYPE_DAI_LINK 6 #define SND_SOC_TPLG_TYPE_PCM 7 #define SND_SOC_TPLG_TYPE_MANIFEST 8 #define SND_SOC_TPLG_TYPE_CODEC_LINK 9 #define SND_SOC_TPLG_TYPE_BACKEND_LINK 10 #define SND_SOC_TPLG_TYPE_PDATA 11 #define SND_SOC_TPLG_TYPE_DAI 12 #define SOC_TPLG_PASS_MANIFEST 0 #define SOC_TPLG_PASS_VENDOR 1 #define SOC_TPLG_PASS_MIXER 2 #define SOC_TPLG_PASS_WIDGET 3 #define SOC_TPLG_PASS_PCM_DAI 4 #define SOC_TPLG_PASS_GRAPH 5 #define SOC_TPLG_PASS_PINS 6 #define SOC_TPLG_PASS_BE_DAI 7 #define SOC_TPLG_PASS_LINK 8 > > Signed-off-by: Keyon Jie <yang.jie@linux.intel.com> > --- > sound/soc/soc-topology.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c > index 49875978a1ce..58cf5a12af3f 100644 > --- a/sound/soc/soc-topology.c > +++ b/sound/soc/soc-topology.c > @@ -2685,11 +2685,10 @@ static int soc_valid_header(struct soc_tplg *tplg, > return -EINVAL; > } > > - if (tplg->pass == le32_to_cpu(hdr->type)) > - dev_dbg(tplg->dev, > - "ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n", > - hdr->payload_size, hdr->type, hdr->version, > - hdr->vendor_type, tplg->pass); > + dev_dbg(tplg->dev, > + "ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n", > + hdr->payload_size, hdr->type, hdr->version, > + hdr->vendor_type, tplg->pass); > > return 1; > } >
On 5/12/20 2:38 AM, Pierre-Louis Bossart wrote: > Kind reminder to always CC: maintainers.... Thanks for reminding. > > On 5/9/20 3:22 AM, Keyon Jie wrote: >> The check (tplg->pass == le32_to_cpu(hdr->type)) makes no sense as it is >> comparing two different enums, remove the check and log all headers that >> being parsed. > > It's true that the TYPE_MIXER should probably not be compared to > TPLG_PASS_MANIFEST, but one would think that e.g. the TYPE_MIXER is > handled in the TPLG_PASS_MIXER, no? and likewise that in the > TPLG_PASS_MANIFEST all TPLG_TYPE_MANIFEST are handled? Yes, all TYPE_MIXER/ENUM/BYTES are handled in the TPLG_PASS_MIXER, and in the TPLG_PASS_MANIFEST all TPLG_TYPE_MANIFEST are handled, but all these are guaranteed and more details are logged inside the corresponding xxx_load(), at here it is only checking the validity of the header, I think we can log all headers for debug purpose? Imagine that there might be some headers won't be parsed by any xxx_load(). > > Shouldn't you add a switch case to reconcile the two lists instead of > removing the test altogether? I can add a function to reconcile the two list, but since we already have "tplg->pass != SOC_TPLG_PASS_xxx" check in each xxx_load(), do you suggest to moving the logging inside these xxx_load() to reduce the redundant check? Thanks, ~Keyon > > #define SND_SOC_TPLG_TYPE_MIXER 1 > #define SND_SOC_TPLG_TYPE_BYTES 2 > #define SND_SOC_TPLG_TYPE_ENUM 3 > #define SND_SOC_TPLG_TYPE_DAPM_GRAPH 4 > #define SND_SOC_TPLG_TYPE_DAPM_WIDGET 5 > #define SND_SOC_TPLG_TYPE_DAI_LINK 6 > #define SND_SOC_TPLG_TYPE_PCM 7 > #define SND_SOC_TPLG_TYPE_MANIFEST 8 > #define SND_SOC_TPLG_TYPE_CODEC_LINK 9 > #define SND_SOC_TPLG_TYPE_BACKEND_LINK 10 > #define SND_SOC_TPLG_TYPE_PDATA 11 > #define SND_SOC_TPLG_TYPE_DAI 12 > > #define SOC_TPLG_PASS_MANIFEST 0 > #define SOC_TPLG_PASS_VENDOR 1 > #define SOC_TPLG_PASS_MIXER 2 > #define SOC_TPLG_PASS_WIDGET 3 > #define SOC_TPLG_PASS_PCM_DAI 4 > #define SOC_TPLG_PASS_GRAPH 5 > #define SOC_TPLG_PASS_PINS 6 > #define SOC_TPLG_PASS_BE_DAI 7 > #define SOC_TPLG_PASS_LINK 8 > > >> >> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com> >> --- >> sound/soc/soc-topology.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c >> index 49875978a1ce..58cf5a12af3f 100644 >> --- a/sound/soc/soc-topology.c >> +++ b/sound/soc/soc-topology.c >> @@ -2685,11 +2685,10 @@ static int soc_valid_header(struct soc_tplg >> *tplg, >> return -EINVAL; >> } >> - if (tplg->pass == le32_to_cpu(hdr->type)) >> - dev_dbg(tplg->dev, >> - "ASoC: Got 0x%x bytes of type %d version %d vendor %d at >> pass %d\n", >> - hdr->payload_size, hdr->type, hdr->version, >> - hdr->vendor_type, tplg->pass); >> + dev_dbg(tplg->dev, >> + "ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass >> %d\n", >> + hdr->payload_size, hdr->type, hdr->version, >> + hdr->vendor_type, tplg->pass); >> return 1; >> } >>
> I can add a function to reconcile the two list, but since we already > have "tplg->pass != SOC_TPLG_PASS_xxx" check in each xxx_load(), do you > suggest to moving the logging inside these xxx_load() to reduce the > redundant check? yes, you could add a static inline sof_tplg_log_hdr(hdr); after each pass check, sounds good to me.
On 5/13/20 12:22 AM, Pierre-Louis Bossart wrote: > > >> I can add a function to reconcile the two list, but since we already >> have "tplg->pass != SOC_TPLG_PASS_xxx" check in each xxx_load(), do >> you suggest to moving the logging inside these xxx_load() to reduce >> the redundant check? > > yes, you could add a static inline sof_tplg_log_hdr(hdr); after each > pass check, sounds good to me. OK, let me drop this and will send a new one soon. Thanks, ~Keyon
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 49875978a1ce..58cf5a12af3f 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2685,11 +2685,10 @@ static int soc_valid_header(struct soc_tplg *tplg, return -EINVAL; } - if (tplg->pass == le32_to_cpu(hdr->type)) - dev_dbg(tplg->dev, - "ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n", - hdr->payload_size, hdr->type, hdr->version, - hdr->vendor_type, tplg->pass); + dev_dbg(tplg->dev, + "ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n", + hdr->payload_size, hdr->type, hdr->version, + hdr->vendor_type, tplg->pass); return 1; }
The check (tplg->pass == le32_to_cpu(hdr->type)) makes no sense as it is comparing two different enums, remove the check and log all headers that being parsed. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com> --- sound/soc/soc-topology.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)