linux-kernel - Re: [PATCH 3/3] ASoC: sh: rz-ssi: Add a devres action to release the DMA channels

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]

Message-ID: <CA+V-a8uGhUMK11BXQX9e-+8bwaq9Gx_AGHBob1KTiT6Npb-8Rw@mail.gmail.com>
Date:   Mon, 25 Apr 2022 10:29:15 +0100
From:   "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To:     Biju Das <biju.das.jz@...renesas.com>
Cc:     Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>,
        Pavel Machek <pavel@...x.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH 3/3] ASoC: sh: rz-ssi: Add a devres action to release the
 DMA channels

Hi Biju,

Thank you for the review.

On Fri, Apr 22, 2022 at 7:52 AM Biju Das <biju.das.jz@...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@...x.de>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...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
>