[lkml]   [2019]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [alsa-devel] [PATCH 13/14] soundwire: intel: free all resources on hw_free()

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

 \ /
  Last update: 2019-11-08 16:49    [W:0.049 / U:4.036 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site