| Message ID | 20191023212823.608-14-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: intel: implement new ASoC interfaces | expand |
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@linux.intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > 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? > + 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. > 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. > > static void intel_shutdown(struct snd_pcm_substream *substream, >
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@linux.intel.com> >> Signed-off-by: Pierre-Louis Bossart >> <pierre-louis.bossart@linux.intel.com> >> --- >> 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.
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!
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?
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) + 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; + } + 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; } static void intel_shutdown(struct snd_pcm_substream *substream,