linux-kernel - Re: [alsa-devel] [PATCH 13/14] soundwire: intel: free all resources on hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6daf0d90-a5f9-d510-f458-879528500134@linux.intel.com>
Date:   Fri, 8 Nov 2019 08:39:49 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     Cezary Rojewski <cezary.rojewski@...el.com>, tiwai@...e.de,
        gregkh@...uxfoundation.org, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, broonie@...nel.org,
        srinivas.kandagatla@...aro.org,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        jank@...ence.com, slawomir.blauciak@...el.com,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        Bard liao <yung-chuan.liao@...ux.intel.com>,
        Rander Wang <rander.wang@...ux.intel.com>
Subject: Re: [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 
verbosity?