| Message ID | 20220818203308.439043-3-nicolas.dufresne@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | cedrus: Various bug fixes | expand |
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:
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:
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: