Re: [alsa-devel] [PATCH 13/14] soundwire: intel: free all resources on hw_free()

[Pierre-Louis Bossart]

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?