Message ID | 20220818203308.439043-3-nicolas.dufresne@collabora.com (mailing list archive) |
---|---|
State | Accepted |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1oOmDI-006F8R-U0; Thu, 18 Aug 2022 20:34:01 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346096AbiHRUd7 (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 18 Aug 2022 16:33:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52876 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1346091AbiHRUd6 (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 18 Aug 2022 16:33:58 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B9F3F3C8F9; Thu, 18 Aug 2022 13:33:56 -0700 (PDT) Received: from whitebuilder.lan (192-222-136-102.qc.cable.ebox.net [192.222.136.102]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: nicolas) by madras.collabora.co.uk (Postfix) with ESMTPSA id 9106A6601B7A; Thu, 18 Aug 2022 21:33:53 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1660854835; bh=hvEiAvpDFVVSoBAQ52gtXwVxHayJqh7AhxJnyUuWZII=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Yo6EbmMswKJNtgeL+rG3yxayMi3H8ZSeZRYASO4kAqFDuxN3l3Ka30qQpubYUGgwc 6bAbT8UosuDfp30uy4kGhil39cvLZTnCkhfmVPqoCVfshyhIlmcvunEeU7yrTXaWrk SUo1ns8Fz/YomVGR3o501y17WiWZBSot5PRmVP2tUZn2Wn+wqwccGOHGHf17+PBQjd gFw10zuG5qGqZOfA+S6Ltn9LkPlamcg63+EATVJ4Ee4a4pJhGWsElLHTlenrSFlnnW JaKDEqlydcRNE0hQe3E5+RDkpQN8+K6oJW3bpFKgzmS1tRklppRZpl8ADQYoAVjUav yQ+jsc1Vk2f7Q== From: Nicolas Dufresne <nicolas.dufresne@collabora.com> To: linux-media@vger.kernel.org, Maxime Ripard <mripard@kernel.org>, Paul Kocialkowski <paul.kocialkowski@bootlin.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, Samuel Holland <samuel@sholland.org> Cc: kernel@collabora.com, Dmitry Osipenko <dmitry.osipenko@collabora.com>, stable@vger.kernel.org, Nicolas Dufresne <nicolas.dufresne@collabora.com>, linux-staging@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org Subject: [PATCH v1 2/3] media: cedrus: Set the platform driver data earlier Date: Thu, 18 Aug 2022 16:33:07 -0400 Message-Id: <20220818203308.439043-3-nicolas.dufresne@collabora.com> X-Mailer: git-send-email 2.37.2 In-Reply-To: <20220818203308.439043-1-nicolas.dufresne@collabora.com> References: <20220818203308.439043-1-nicolas.dufresne@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
cedrus: Various bug fixes
|
|
Commit Message
Nicolas Dufresne
Aug. 18, 2022, 8:33 p.m. UTC
From: Dmitry Osipenko <dmitry.osipenko@collabora.com> The cedrus_hw_resume() crashes with NULL deference on driver probe if runtime PM is disabled because it uses platform data that hasn't been set up yet. Fix this by setting the platform data earlier during probe. Cc: stable@vger.kernel.org Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> --- drivers/staging/media/sunxi/cedrus/cedrus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Comments
Dne četrtek, 18. avgust 2022 ob 22:33:07 CEST je Nicolas Dufresne napisal(a): > From: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > The cedrus_hw_resume() crashes with NULL deference on driver probe if > runtime PM is disabled because it uses platform data that hasn't been > set up yet. Fix this by setting the platform data earlier during probe. Does it even work without PM? Maybe it would be better if Cedrus would select PM in Kconfig. Best regards, Jernej > > Cc: stable@vger.kernel.org > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > drivers/staging/media/sunxi/cedrus/cedrus.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > 960a0130cd620..55c54dfdc585c 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > @@ -448,6 +448,8 @@ static int cedrus_probe(struct platform_device *pdev) > if (!dev) > return -ENOMEM; > > + platform_set_drvdata(pdev, dev); > + > dev->vfd = cedrus_video_device; > dev->dev = &pdev->dev; > dev->pdev = pdev; > @@ -521,8 +523,6 @@ static int cedrus_probe(struct platform_device *pdev) > goto err_m2m_mc; > } > > - platform_set_drvdata(pdev, dev); > - > return 0; > > err_m2m_mc:
Le vendredi 19 août 2022 à 06:17 +0200, Jernej Škrabec a écrit : > Dne četrtek, 18. avgust 2022 ob 22:33:07 CEST je Nicolas Dufresne napisal(a): > > From: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > > > The cedrus_hw_resume() crashes with NULL deference on driver probe if > > runtime PM is disabled because it uses platform data that hasn't been > > set up yet. Fix this by setting the platform data earlier during probe. > > Does it even work without PM? Maybe it would be better if Cedrus would select > PM in Kconfig. I cannot comment myself on this, but it does not seem to invalidate this Dmitry's fix. > > Best regards, > Jernej > > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > --- > > drivers/staging/media/sunxi/cedrus/cedrus.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > 960a0130cd620..55c54dfdc585c 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > @@ -448,6 +448,8 @@ static int cedrus_probe(struct platform_device *pdev) > > if (!dev) > > return -ENOMEM; > > > > + platform_set_drvdata(pdev, dev); > > + > > dev->vfd = cedrus_video_device; > > dev->dev = &pdev->dev; > > dev->pdev = pdev; > > @@ -521,8 +523,6 @@ static int cedrus_probe(struct platform_device *pdev) > > goto err_m2m_mc; > > } > > > > - platform_set_drvdata(pdev, dev); > > - > > return 0; > > > > err_m2m_mc: > > > >
Dne petek, 19. avgust 2022 ob 17:37:20 CEST je Nicolas Dufresne napisal(a): > Le vendredi 19 août 2022 à 06:17 +0200, Jernej Škrabec a écrit : > > Dne četrtek, 18. avgust 2022 ob 22:33:07 CEST je Nicolas Dufresne napisal(a): > > > From: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > > > > > The cedrus_hw_resume() crashes with NULL deference on driver probe if > > > runtime PM is disabled because it uses platform data that hasn't been > > > set up yet. Fix this by setting the platform data earlier during probe. > > > > Does it even work without PM? Maybe it would be better if Cedrus would > > select PM in Kconfig. > > I cannot comment myself on this, but it does not seem to invalidate this > Dmitry's fix. If NULL pointer dereference happens only when PM is disabled, then it does. I have PM always enabled and I never experienced above issue. Best regards, Jernej > > > Best regards, > > Jernej > > > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > --- > > > > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus.c index > > > 960a0130cd620..55c54dfdc585c 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > > > @@ -448,6 +448,8 @@ static int cedrus_probe(struct platform_device > > > *pdev) > > > > > > if (!dev) > > > > > > return -ENOMEM; > > > > > > + platform_set_drvdata(pdev, dev); > > > + > > > > > > dev->vfd = cedrus_video_device; > > > dev->dev = &pdev->dev; > > > dev->pdev = pdev; > > > > > > @@ -521,8 +523,6 @@ static int cedrus_probe(struct platform_device > > > *pdev) > > > > > > goto err_m2m_mc; > > > > > > } > > > > > > - platform_set_drvdata(pdev, dev); > > > - > > > > > > return 0; > > > > > > err_m2m_mc:
On 8/20/22 11:25, Jernej Škrabec wrote: > Dne petek, 19. avgust 2022 ob 17:37:20 CEST je Nicolas Dufresne napisal(a): >> Le vendredi 19 août 2022 à 06:17 +0200, Jernej Škrabec a écrit : >>> Dne četrtek, 18. avgust 2022 ob 22:33:07 CEST je Nicolas Dufresne > napisal(a): >>>> From: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> >>>> The cedrus_hw_resume() crashes with NULL deference on driver probe if >>>> runtime PM is disabled because it uses platform data that hasn't been >>>> set up yet. Fix this by setting the platform data earlier during probe. >>> >>> Does it even work without PM? Maybe it would be better if Cedrus would >>> select PM in Kconfig. >> >> I cannot comment myself on this, but it does not seem to invalidate this >> Dmitry's fix. > > If NULL pointer dereference happens only when PM is disabled, then it does. I > have PM always enabled and I never experienced above issue. Originally this fix was needed when I changed cedrus_hw_probe() to do the rpm-resume while was debugging the hang issue and then also noticed that the !PM should be broken. It's a good common practice for all drivers to have the drv data set early to avoid such problems, hence it won't hurt to make this change anyways. In practice nobody disables PM other than for debugging purposes and !PM handling is often broken in drivers. I assume that it might be even better to enable PM for all Allwiner SoCs and remove !PM handling from all the affected drivers, like it was done for Tegra some time ago.
On 8/20/22 3:25 AM, Jernej Škrabec wrote: > Dne petek, 19. avgust 2022 ob 17:37:20 CEST je Nicolas Dufresne napisal(a): >> Le vendredi 19 août 2022 à 06:17 +0200, Jernej Škrabec a écrit : >>> Dne četrtek, 18. avgust 2022 ob 22:33:07 CEST je Nicolas Dufresne > napisal(a): >>>> From: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> >>>> The cedrus_hw_resume() crashes with NULL deference on driver probe if >>>> runtime PM is disabled because it uses platform data that hasn't been >>>> set up yet. Fix this by setting the platform data earlier during probe. >>> >>> Does it even work without PM? Maybe it would be better if Cedrus would >>> select PM in Kconfig. >> >> I cannot comment myself on this, but it does not seem to invalidate this >> Dmitry's fix. > > If NULL pointer dereference happens only when PM is disabled, then it does. I > have PM always enabled and I never experienced above issue. There's still a bug even with PM enabled: the v4l2 device is exposed to userspace, and therefore userspace could trigger a PM resume, before platform_set_drvdata() is called. >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Please add a Fixes tag. With that: Reviewed-by: Samuel Holland <samuel@sholland.org> >>>> --- >>>> >>>> drivers/staging/media/sunxi/cedrus/cedrus.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c >>>> b/drivers/staging/media/sunxi/cedrus/cedrus.c index >>>> 960a0130cd620..55c54dfdc585c 100644 >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c >>>> @@ -448,6 +448,8 @@ static int cedrus_probe(struct platform_device >>>> *pdev) >>>> >>>> if (!dev) >>>> return -ENOMEM; >>>> >>>> + platform_set_drvdata(pdev, dev); >>>> + >>>> dev->vfd = cedrus_video_device; >>>> dev->dev = &pdev->dev; >>>> dev->pdev = pdev; >>>> >>>> @@ -521,8 +523,6 @@ static int cedrus_probe(struct platform_device >>>> *pdev) >>>> >>>> goto err_m2m_mc; >>>> } >>>> >>>> - platform_set_drvdata(pdev, dev); >>>> - >>>> return 0; >>>> >>>> err_m2m_mc:
Hi, On Mon 22 Aug 22, 22:57, Samuel Holland wrote: > On 8/20/22 3:25 AM, Jernej Škrabec wrote: > > Dne petek, 19. avgust 2022 ob 17:37:20 CEST je Nicolas Dufresne napisal(a): > >> Le vendredi 19 août 2022 à 06:17 +0200, Jernej Škrabec a écrit : > >>> Dne četrtek, 18. avgust 2022 ob 22:33:07 CEST je Nicolas Dufresne > > napisal(a): > >>>> From: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >>>> > >>>> The cedrus_hw_resume() crashes with NULL deference on driver probe if > >>>> runtime PM is disabled because it uses platform data that hasn't been > >>>> set up yet. Fix this by setting the platform data earlier during probe. > >>> > >>> Does it even work without PM? Maybe it would be better if Cedrus would > >>> select PM in Kconfig. > >> > >> I cannot comment myself on this, but it does not seem to invalidate this > >> Dmitry's fix. > > > > If NULL pointer dereference happens only when PM is disabled, then it does. I > > have PM always enabled and I never experienced above issue. > > There's still a bug even with PM enabled: the v4l2 device is exposed to > userspace, and therefore userspace could trigger a PM resume, before > platform_set_drvdata() is called. Absolutely agreed! > >>>> Cc: stable@vger.kernel.org > >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Please add a Fixes tag. With that: > > Reviewed-by: Samuel Holland <samuel@sholland.org> Same here: Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Thanks! Paul > >>>> --- > >>>> > >>>> drivers/staging/media/sunxi/cedrus/cedrus.c | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c > >>>> b/drivers/staging/media/sunxi/cedrus/cedrus.c index > >>>> 960a0130cd620..55c54dfdc585c 100644 > >>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c > >>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c > >>>> @@ -448,6 +448,8 @@ static int cedrus_probe(struct platform_device > >>>> *pdev) > >>>> > >>>> if (!dev) > >>>> return -ENOMEM; > >>>> > >>>> + platform_set_drvdata(pdev, dev); > >>>> + > >>>> dev->vfd = cedrus_video_device; > >>>> dev->dev = &pdev->dev; > >>>> dev->pdev = pdev; > >>>> > >>>> @@ -521,8 +523,6 @@ static int cedrus_probe(struct platform_device > >>>> *pdev) > >>>> > >>>> goto err_m2m_mc; > >>>> } > >>>> > >>>> - platform_set_drvdata(pdev, dev); > >>>> - > >>>> return 0; > >>>> > >>>> err_m2m_mc:
Dne nedelja, 21. avgust 2022 ob 22:40:21 CEST je Dmitry Osipenko napisal(a): > On 8/20/22 11:25, Jernej Škrabec wrote: > > Dne petek, 19. avgust 2022 ob 17:37:20 CEST je Nicolas Dufresne napisal(a): > >> Le vendredi 19 août 2022 à 06:17 +0200, Jernej Škrabec a écrit : > >>> Dne četrtek, 18. avgust 2022 ob 22:33:07 CEST je Nicolas Dufresne > > > > napisal(a): > >>>> From: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >>>> > >>>> The cedrus_hw_resume() crashes with NULL deference on driver probe if > >>>> runtime PM is disabled because it uses platform data that hasn't been > >>>> set up yet. Fix this by setting the platform data earlier during probe. > >>> > >>> Does it even work without PM? Maybe it would be better if Cedrus would > >>> select PM in Kconfig. > >> > >> I cannot comment myself on this, but it does not seem to invalidate this > >> Dmitry's fix. > > > > If NULL pointer dereference happens only when PM is disabled, then it > > does. I have PM always enabled and I never experienced above issue. > > Originally this fix was needed when I changed cedrus_hw_probe() to do > the rpm-resume while was debugging the hang issue and then also noticed > that the !PM should be broken. It's a good common practice for all > drivers to have the drv data set early to avoid such problems, hence it > won't hurt to make this change anyways. Ok, as others pointed out, this is still a good thing, so: Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com> > > In practice nobody disables PM other than for debugging purposes and !PM > handling is often broken in drivers. I assume that it might be even > better to enable PM for all Allwiner SoCs and remove !PM handling from > all the affected drivers, like it was done for Tegra some time ago. > Maybe in the future :) Best regards, Jernej > -- > Best regards, > Dmitry
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c index 960a0130cd620..55c54dfdc585c 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c @@ -448,6 +448,8 @@ static int cedrus_probe(struct platform_device *pdev) if (!dev) return -ENOMEM; + platform_set_drvdata(pdev, dev); + dev->vfd = cedrus_video_device; dev->dev = &pdev->dev; dev->pdev = pdev; @@ -521,8 +523,6 @@ static int cedrus_probe(struct platform_device *pdev) goto err_m2m_mc; } - platform_set_drvdata(pdev, dev); - return 0; err_m2m_mc: