Messages in this thread | | | Subject | Re: [alsa-devel] [PATCH 13/14] soundwire: intel: free all resources on hw_free() | From | Pierre-Louis Bossart <> | Date | Fri, 8 Nov 2019 08:39:49 -0600 |
| |
On 11/7/19 10:14 PM, Vinod Koul wrote: > On 04-11-19, 15:46, Pierre-Louis Bossart wrote: >> On 11/4/19 2:08 PM, Cezary Rojewski wrote: >>> On 2019-10-23 23:28, Pierre-Louis Bossart wrote: >>>> @@ -816,6 +835,7 @@ static int >>>> intel_hw_free(struct snd_pcm_substream *substream, struct >>>> snd_soc_dai *dai) >>>> { >>>> struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); >>>> + struct sdw_intel *sdw = cdns_to_intel(cdns); >>>> struct sdw_cdns_dma_data *dma; >>>> int ret; >>>> @@ -823,12 +843,28 @@ intel_hw_free(struct snd_pcm_substream >>>> *substream, struct snd_soc_dai *dai) >>>> if (!dma) >>>> return -EIO; >>>> + ret = sdw_deprepare_stream(dma->stream); >>>> + if (ret) { >>>> + dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret); >>>> + return ret; >>>> + } >>>> + >>> >>> I understand that you want to be transparent to caller with failure >>> reasons via dev_err/_warn. However, sdw_deprepare_stream already dumps >>> all the logs we need. The same applies for most of the other calls (and >>> not just in this patch..). > > I think this is a valid concern! In linux we do not do that, for example > we ask people to not log errors on kmalloc as it will be logged on > failures so drivers do not need to do that. > >>> Do we really need to be that verbose? Maybe just agree on caller -or- >>> subject being the source for the messaging, align existing usages and >>> thus preventing further duplication? >>> >>> Not forcing anything, just asking for your opinion on this. >> >> the sdw_prepare/deprepare_stream calls provide error logs, but they are not >> mapped to specific devices/dais (pr_err vs. dev_dbg). I found it was easier >> to check for which dai the error was reported. > > Well in that case we should fix pr_err, there are only 17 instances of > these in core, and few of them are justified in core (no dev pointer) > and 11 in stream (few of them valid (no stream pointer) but rest can be > converted to use dev_err! Even then they print stream name, so checking > error is not justified argument!
the stream has no notion of device, it can be made of multiple devices, so which one would you choose?
> >> We are also in the middle of integration with new hardware/boards, and >> erring on the side of more traces will help everyone involved. We can >> revisit later which ones are strictly necessary. > > Naah you are having duplicate logs, it does *not* help in debug seems > 1000 line logs and few lines conveying duplicate info, I would rather > have each line unique so that I dont have to skip duplicate ones while > debugging!
They are not all duplicates.
Again, if I remove the logs in stream.c, then I do lose valuable information on bad state machines transitions, etc. An error code is not enough to reconstruct the issues from intel.c
If I remove the logs in intel.c, I can't know which dai had an error and what caused it.
seriously, these are all details, you have over 50 patches to review with a complete rework of this subsystem and we argue about dev_err verbosity?
| |