[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?