| Message ID | 20220421203555.29011-4-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | ASoC: sh: rz-ssi: Trivial fixes | expand |
Hi Lad Prabhakar, Thanks for the patch. > Subject: [PATCH 3/3] ASoC: sh: rz-ssi: Add a devres action to release the > DMA channels > > DMA channels requested by rz_ssi_dma_request() in rz_ssi_probe() were never > released in the error path apart from one place. This patch fixes this > issue by adding a devres action to release the DMA channels and dropping > the single rz_ssi_release_dma_channels() call which was placed in the error > path in case devm_snd_soc_register_component() failed. > > Fixes: 26ac471c5354 ("ASoC: sh: rz-ssi: Add SSI DMAC support") > Reported-by: Pavel Machek <pavel@denx.de> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > sound/soc/sh/rz-ssi.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c index > d9a684e71ec3..f04da1bf5680 100644 > --- a/sound/soc/sh/rz-ssi.c > +++ b/sound/soc/sh/rz-ssi.c > @@ -912,6 +912,11 @@ static const struct snd_soc_component_driver > rz_ssi_soc_component = { > .pcm_construct = rz_ssi_pcm_new, > }; > > +static void rz_ssi_release_dma_channels_action(void *data) { > + rz_ssi_release_dma_channels(data); > +} > + > static int rz_ssi_probe(struct platform_device *pdev) { > struct rz_ssi_priv *ssi; > @@ -966,6 +971,11 @@ static int rz_ssi_probe(struct platform_device *pdev) > dev_info(&pdev->dev, "DMA enabled"); > ssi->playback.transfer = rz_ssi_dma_transfer; > ssi->capture.transfer = rz_ssi_dma_transfer; > + > + ret = devm_add_action_or_reset(&pdev->dev, > + rz_ssi_release_dma_channels_action, > ssi); > + if (ret) > + return ret; > } > > ssi->playback.priv = ssi; > @@ -1027,8 +1037,6 @@ static int rz_ssi_probe(struct platform_device *pdev) > rz_ssi_soc_dai, > ARRAY_SIZE(rz_ssi_soc_dai)); > if (ret < 0) { > - rz_ssi_release_dma_channels(ssi); > - Maybe we need to keep this as it is, otherwise DMA channel release will happen after CLK disable and Reset assert. Ideally release the channel, disable the clock and assert the reset. Similarly, you may want to add "rz_ssi_release_dma_channels(ssi)" for error path related to Pm_runtime_resume_get. Also with this change there is unbalanced release_dma_channels() one from devres and other from remove. Regards, Biju > pm_runtime_put(ssi->dev); > pm_runtime_disable(ssi->dev); > reset_control_assert(ssi->rstc); > -- > 2.17.1
Hi Biju, Thank you for the review. On Fri, Apr 22, 2022 at 7:52 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Lad Prabhakar, > > Thanks for the patch. > > > Subject: [PATCH 3/3] ASoC: sh: rz-ssi: Add a devres action to release the > > DMA channels > > > > DMA channels requested by rz_ssi_dma_request() in rz_ssi_probe() were never > > released in the error path apart from one place. This patch fixes this > > issue by adding a devres action to release the DMA channels and dropping > > the single rz_ssi_release_dma_channels() call which was placed in the error > > path in case devm_snd_soc_register_component() failed. > > > > Fixes: 26ac471c5354 ("ASoC: sh: rz-ssi: Add SSI DMAC support") > > Reported-by: Pavel Machek <pavel@denx.de> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > sound/soc/sh/rz-ssi.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c index > > d9a684e71ec3..f04da1bf5680 100644 > > --- a/sound/soc/sh/rz-ssi.c > > +++ b/sound/soc/sh/rz-ssi.c > > @@ -912,6 +912,11 @@ static const struct snd_soc_component_driver > > rz_ssi_soc_component = { > > .pcm_construct = rz_ssi_pcm_new, > > }; > > > > +static void rz_ssi_release_dma_channels_action(void *data) { > > + rz_ssi_release_dma_channels(data); > > +} > > + > > static int rz_ssi_probe(struct platform_device *pdev) { > > struct rz_ssi_priv *ssi; > > @@ -966,6 +971,11 @@ static int rz_ssi_probe(struct platform_device *pdev) > > dev_info(&pdev->dev, "DMA enabled"); > > ssi->playback.transfer = rz_ssi_dma_transfer; > > ssi->capture.transfer = rz_ssi_dma_transfer; > > + > > + ret = devm_add_action_or_reset(&pdev->dev, > > + rz_ssi_release_dma_channels_action, > > ssi); > > + if (ret) > > + return ret; > > } > > > > ssi->playback.priv = ssi; > > @@ -1027,8 +1037,6 @@ static int rz_ssi_probe(struct platform_device *pdev) > > rz_ssi_soc_dai, > > ARRAY_SIZE(rz_ssi_soc_dai)); > > if (ret < 0) { > > - rz_ssi_release_dma_channels(ssi); > > - > > Maybe we need to keep this as it is, otherwise DMA channel release will happen > after CLK disable and Reset assert. Ideally release the channel, disable the clock and assert > the reset. > Ok will move this call to individual error paths. > Similarly, you may want to add "rz_ssi_release_dma_channels(ssi)" for error path related to > Pm_runtime_resume_get. > yes this needs to go under all error paths except the pio chunk. > > Also with this change there is unbalanced release_dma_channels() one from devres and other from remove. > Agreed. Cheers, Prabhakar > Regards, > Biju > > > > > pm_runtime_put(ssi->dev); > > pm_runtime_disable(ssi->dev); > > reset_control_assert(ssi->rstc); > > -- > > 2.17.1 >
diff --git a/sound/soc/sh/rz-ssi.c b/sound/soc/sh/rz-ssi.c index d9a684e71ec3..f04da1bf5680 100644 --- a/sound/soc/sh/rz-ssi.c +++ b/sound/soc/sh/rz-ssi.c @@ -912,6 +912,11 @@ static const struct snd_soc_component_driver rz_ssi_soc_component = { .pcm_construct = rz_ssi_pcm_new, }; +static void rz_ssi_release_dma_channels_action(void *data) +{ + rz_ssi_release_dma_channels(data); +} + static int rz_ssi_probe(struct platform_device *pdev) { struct rz_ssi_priv *ssi; @@ -966,6 +971,11 @@ static int rz_ssi_probe(struct platform_device *pdev) dev_info(&pdev->dev, "DMA enabled"); ssi->playback.transfer = rz_ssi_dma_transfer; ssi->capture.transfer = rz_ssi_dma_transfer; + + ret = devm_add_action_or_reset(&pdev->dev, + rz_ssi_release_dma_channels_action, ssi); + if (ret) + return ret; } ssi->playback.priv = ssi; @@ -1027,8 +1037,6 @@ static int rz_ssi_probe(struct platform_device *pdev) rz_ssi_soc_dai, ARRAY_SIZE(rz_ssi_soc_dai)); if (ret < 0) { - rz_ssi_release_dma_channels(ssi); - pm_runtime_put(ssi->dev); pm_runtime_disable(ssi->dev); reset_control_assert(ssi->rstc);
DMA channels requested by rz_ssi_dma_request() in rz_ssi_probe() were never released in the error path apart from one place. This patch fixes this issue by adding a devres action to release the DMA channels and dropping the single rz_ssi_release_dma_channels() call which was placed in the error path in case devm_snd_soc_register_component() failed. Fixes: 26ac471c5354 ("ASoC: sh: rz-ssi: Add SSI DMAC support") Reported-by: Pavel Machek <pavel@denx.de> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- sound/soc/sh/rz-ssi.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)