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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Nov 2019 09:44:35 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     Cezary Rojewski <cezary.rojewski@...el.com>,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        tiwai@...e.de, broonie@...nel.org, gregkh@...uxfoundation.org,
        jank@...ence.com, srinivas.kandagatla@...aro.org,
        slawomir.blauciak@...el.com,
        Bard liao <yung-chuan.liao@...ux.intel.com>,
        Rander Wang <rander.wang@...ux.intel.com>,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>
Subject: Re: [alsa-devel] [PATCH 13/14] soundwire: intel: free all resources
 on hw_free()

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!

> 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!

-- 
~Vinod