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

[Pierre-Louis Bossart]

On 11/4/19 2:08 PM, Cezary Rojewski wrote:
> On 2019-10-23 23:28, Pierre-Louis Bossart wrote:
> > Make sure all calls to the SoundWire stream API are done and involve
> > callback
> >
> > Signed-off-by: Rander Wang <rander.wang@xxxxxxxxxxxxxxx>
> > Signed-off-by: Pierre-Louis Bossart 
> > <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> > ---
> > Â drivers/soundwire/intel.c | 40 +++++++++++++++++++++++++++++++++++++--
> > Â 1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > index af24fa048add..cad1c0b64ee3 100644
> > --- a/drivers/soundwire/intel.c
> > +++ b/drivers/soundwire/intel.c
> > @@ -548,6 +548,25 @@ static int intel_params_stream(struct sdw_intel 
> > *sdw,
> > ÂÂÂÂÂ return -EIO;
> > Â }
> > +static int intel_free_stream(struct sdw_intel *sdw,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_pcm_substream *substream,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int link_id)
> > +{
> > +ÂÂÂ struct sdw_intel_link_res *res = sdw->link_res;
> > +ÂÂÂ struct sdw_intel_stream_free_data free_data;
> > +
> > +ÂÂÂ free_data.substream = substream;
> > +ÂÂÂ free_data.dai = dai;
> > +ÂÂÂ free_data.link_id = link_id;
> > +
> > +ÂÂÂ if (res->ops && res->ops->free_stream && res->dev)
>
> Can res->dev even be null?

in error cases yes. this 'res' structure is setup by the DSP driver, and 
it could be wrong or not set.

Note that in the previous solution we tested the res->arg pointer, we 
did find a case where we could oops here.

> > +ÂÂÂÂÂÂÂ return res->ops->free_stream(res->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &free_data);
> > +
> > +ÂÂÂ return 0;
> > +}
> > +
> > Â /*
> > ÂÂ * bank switch routines
> > ÂÂ */
> > @@ -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..).
>
> 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.

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.

> > ÂÂÂÂÂ ret = sdw_stream_remove_master(&cdns->bus, dma->stream);
> > -ÂÂÂ if (ret < 0)
> > +ÂÂÂ if (ret < 0) {
> > ÂÂÂÂÂÂÂÂÂ dev_err(dai->dev, "remove master from stream %s failed: %d\n",
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂ dma->stream->name, ret);
> > +ÂÂÂÂÂÂÂ return ret;
> > +ÂÂÂ }
> > -ÂÂÂ return ret;
> > +ÂÂÂ ret = intel_free_stream(sdw, substream, dai, sdw->instance);
> > +ÂÂÂ if (ret < 0) {
> > +ÂÂÂÂÂÂÂ dev_err(dai->dev, "intel_free_stream: failed %d", ret);
> > +ÂÂÂÂÂÂÂ return ret;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ sdw_release_stream(dma->stream);
> > +
> > +ÂÂÂ return 0;
> > Â }
>
> Given the structure of this function, shouldn't the generic flow be 
> handled by upper layer i.e. part of sdw core?. Apart from 
> intel_free_stream, the rest looks pretty generic to me and this sole 
> call could easily be extracted into ops.

The mapping between DAI and stream is not necessarily the same for all 
platforms, we just had this discussion while reviewing the QCOM patches 
last week. whether you release the resources in .hw_free() or 
.shutdown() is also platform dependent.

Also this code will change when we support the multi-CPU dais, more work 
will be handled at the dailink level than at the dai.
We can (and will) refactor at a later point.