Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220421203555.29011-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20220421203555.29011-4-prabhakar.mahadev-lad.rj@bp.renesas.com> <OS0PR01MB5922C8287D7EEAB3F9DB60C386F79@OS0PR01MB5922.jpnprd01.prod.outlook.com>
In-Reply-To: <OS0PR01MB5922C8287D7EEAB3F9DB60C386F79@OS0PR01MB5922.jpnprd01.prod.outlook.com>
From:   "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date:   Mon, 25 Apr 2022 10:29:15 +0100
Message-ID: <CA+V-a8uGhUMK11BXQX9e-+8bwaq9Gx_AGHBob1KTiT6Npb-8Rw@mail.gmail.com>
Subject: Re: [PATCH 3/3] ASoC: sh: rz-ssi: Add a devres action to release the
 DMA channels
To:     Biju Das <biju.das.jz@bp.renesas.com>
Cc:     Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>,
        Pavel Machek <pavel@denx.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-renesas-soc@vger.kernel.org" 
        <linux-renesas-soc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>