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: <288af70dafc5e73d0fdfac71a33449385d4d6bd3.camel@crapouillou.net>
Date:   Mon, 04 Sep 2023 11:15:09 +0200
From:   Paul Cercueil <paul@...pouillou.net>
To:     Douglas Anderson <dianders@...omium.org>,
        dri-devel@...ts.freedesktop.org, Maxime Ripard <mripard@...nel.org>
Cc:     airlied@...il.com, daniel@...ll.ch, linux-kernel@...r.kernel.org,
        linux-mips@...r.kernel.org
Subject: Re: [RFT PATCH 03/15] drm/ingenic: Call
 drm_atomic_helper_shutdown() at shutdown time

Hi Douglas,

Le vendredi 01 septembre 2023 à 16:41 -0700, Douglas Anderson a écrit :
> Based on grepping through the source code this driver appears to be
> missing a call to drm_atomic_helper_shutdown() at system shutdown
> time. Among other things, this means that if a panel is in use that
> it
> won't be cleanly powered off at system shutdown time.
> 
> The fact that we should call drm_atomic_helper_shutdown() in the case
> of OS shutdown/restart comes straight out of the kernel doc "driver
> instance overview" in drm_drv.c.
> 
> Since this driver uses the component model and shutdown happens at
> the
> base driver, we communicate whether we have to call
> drm_atomic_helper_shutdown() by seeing if drvdata is non-NULL.
> 
> Suggested-by: Maxime Ripard <mripard@...nel.org>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>

LGTM.
Acked-by: Paul Cercueil <paul@...pouillou.net>

> ---
> This commit is only compile-time tested.
> 
> NOTE: this patch touches a lot more than other similar patches since
> the bind() function is long and we want to make sure that we unset
> the
> drvdata if bind() fails.
> 
> While making this patch, I noticed that the bind() function of this
> driver is using "devm" and thus assumes it doesn't need to do much
> explicit error handling. That's actually a bug. As per kernel docs
> [1]
> "the lifetime of the aggregate driver does not align with any of the
> underlying struct device instances. Therefore devm cannot be used and
> all resources acquired or allocated in this callback must be
> explicitly released in the unbind callback". Fixing that is outside
> the scope of this commit.
> 
> [1] https://docs.kernel.org/driver-api/component.html
> 

Noted, thanks.

Cheers,
-Paul

>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 66 +++++++++++++++------
> --
>  1 file changed, 44 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 8dbd4847d3a6..51995a0cd568 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -1130,7 +1130,7 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>  
>         ret = drmm_mode_config_init(drm);
>         if (ret)
> -               return ret;
> +               goto err_drvdata;
>  
>         drm->mode_config.min_width = 0;
>         drm->mode_config.min_height = 0;
> @@ -1142,7 +1142,8 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>         base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>         if (IS_ERR(base)) {
>                 dev_err(dev, "Failed to get memory resource\n");
> -               return PTR_ERR(base);
> +               ret = PTR_ERR(base);
> +               goto err_drvdata;
>         }
>  
>         regmap_config = ingenic_drm_regmap_config;
> @@ -1151,33 +1152,40 @@ static int ingenic_drm_bind(struct device
> *dev, bool has_components)
>                                           &regmap_config);
>         if (IS_ERR(priv->map)) {
>                 dev_err(dev, "Failed to create regmap\n");
> -               return PTR_ERR(priv->map);
> +               ret = PTR_ERR(priv->map);
> +               goto err_drvdata;
>         }
>  
>         irq = platform_get_irq(pdev, 0);
> -       if (irq < 0)
> -               return irq;
> +       if (irq < 0) {
> +               ret = irq;
> +               goto err_drvdata;
> +       }
>  
>         if (soc_info->needs_dev_clk) {
>                 priv->lcd_clk = devm_clk_get(dev, "lcd");
>                 if (IS_ERR(priv->lcd_clk)) {
>                         dev_err(dev, "Failed to get lcd clock\n");
> -                       return PTR_ERR(priv->lcd_clk);
> +                       ret = PTR_ERR(priv->lcd_clk);
> +                       goto err_drvdata;
>                 }
>         }
>  
>         priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
>         if (IS_ERR(priv->pix_clk)) {
>                 dev_err(dev, "Failed to get pixel clock\n");
> -               return PTR_ERR(priv->pix_clk);
> +               ret = PTR_ERR(priv->pix_clk);
> +               goto err_drvdata;
>         }
>  
>         priv->dma_hwdescs = dmam_alloc_coherent(dev,
>                                                 sizeof(*priv-
> >dma_hwdescs),
>                                                 &priv-
> >dma_hwdescs_phys,
>                                                 GFP_KERNEL);
> -       if (!priv->dma_hwdescs)
> -               return -ENOMEM;
> +       if (!priv->dma_hwdescs) {
> +               ret = -ENOMEM;
> +               goto err_drvdata;
> +       }
>  
>         /* Configure DMA hwdesc for foreground0 plane */
>         ingenic_drm_configure_hwdesc_plane(priv, 0);
> @@ -1199,7 +1207,7 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>                                        NULL, DRM_PLANE_TYPE_PRIMARY,
> NULL);
>         if (ret) {
>                 dev_err(dev, "Failed to register plane: %i\n", ret);
> -               return ret;
> +               goto err_drvdata;
>         }
>  
>         if (soc_info->map_noncoherent)
> @@ -1211,7 +1219,7 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>                                         NULL,
> &ingenic_drm_crtc_funcs, NULL);
>         if (ret) {
>                 dev_err(dev, "Failed to init CRTC: %i\n", ret);
> -               return ret;
> +               goto err_drvdata;
>         }
>  
>         drm_crtc_enable_color_mgmt(&priv->crtc, 0, false,
> @@ -1230,7 +1238,7 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>                 if (ret) {
>                         dev_err(dev, "Failed to register overlay
> plane: %i\n",
>                                 ret);
> -                       return ret;
> +                       goto err_drvdata;
>                 }
>  
>                 if (soc_info->map_noncoherent)
> @@ -1241,17 +1249,18 @@ static int ingenic_drm_bind(struct device
> *dev, bool has_components)
>                         if (ret) {
>                                 if (ret != -EPROBE_DEFER)
>                                         dev_err(dev, "Failed to bind
> components: %i\n", ret);
> -                               return ret;
> +                               goto err_drvdata;
>                         }
>  
>                         ret = devm_add_action_or_reset(dev,
> ingenic_drm_unbind_all, priv);
>                         if (ret)
> -                               return ret;
> +                               goto err_drvdata;
>  
>                         priv->ipu_plane = drm_plane_from_index(drm,
> 2);
>                         if (!priv->ipu_plane) {
>                                 dev_err(dev, "Failed to retrieve IPU
> plane\n");
> -                               return -EINVAL;
> +                               ret = -EINVAL;
> +                               goto err_drvdata;
>                         }
>                 }
>         }
> @@ -1263,7 +1272,7 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>                                 break; /* we're done */
>                         if (ret != -EPROBE_DEFER)
>                                 dev_err(dev, "Failed to get bridge
> handle\n");
> -                       return ret;
> +                       goto err_drvdata;
>                 }
>  
>                 if (panel)
> @@ -1275,7 +1284,7 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>                 if (IS_ERR(ib)) {
>                         ret = PTR_ERR(ib);
>                         dev_err(dev, "Failed to init encoder: %d\n",
> ret);
> -                       return ret;
> +                       goto err_drvdata;
>                 }
>  
>                 encoder = &ib->encoder;
> @@ -1290,13 +1299,14 @@ static int ingenic_drm_bind(struct device
> *dev, bool has_components)
>                                         DRM_BRIDGE_ATTACH_NO_CONNECTO
> R);
>                 if (ret) {
>                         dev_err(dev, "Unable to attach bridge\n");
> -                       return ret;
> +                       goto err_drvdata;
>                 }
>  
>                 connector = drm_bridge_connector_init(drm, encoder);
>                 if (IS_ERR(connector)) {
>                         dev_err(dev, "Unable to init connector\n");
> -                       return PTR_ERR(connector);
> +                       ret = PTR_ERR(connector);
> +                       goto err_drvdata;
>                 }
>  
>                 drm_connector_attach_encoder(connector, encoder);
> @@ -1313,13 +1323,13 @@ static int ingenic_drm_bind(struct device
> *dev, bool has_components)
>         ret = devm_request_irq(dev, irq, ingenic_drm_irq_handler, 0,
> drm->driver->name, drm);
>         if (ret) {
>                 dev_err(dev, "Unable to install IRQ handler\n");
> -               return ret;
> +               goto err_drvdata;
>         }
>  
>         ret = drm_vblank_init(drm, 1);
>         if (ret) {
>                 dev_err(dev, "Failed calling drm_vblank_init()\n");
> -               return ret;
> +               goto err_drvdata;
>         }
>  
>         drm_mode_config_reset(drm);
> @@ -1327,7 +1337,7 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>         ret = clk_prepare_enable(priv->pix_clk);
>         if (ret) {
>                 dev_err(dev, "Unable to start pixel clock\n");
> -               return ret;
> +               goto err_drvdata;
>         }
>  
>         if (priv->lcd_clk) {
> @@ -1402,6 +1412,8 @@ static int ingenic_drm_bind(struct device *dev,
> bool has_components)
>                 clk_disable_unprepare(priv->lcd_clk);
>  err_pixclk_disable:
>         clk_disable_unprepare(priv->pix_clk);
> +err_drvdata:
> +       platform_set_drvdata(pdev, NULL);
>         return ret;
>  }
>  
> @@ -1422,6 +1434,7 @@ static void ingenic_drm_unbind(struct device
> *dev)
>  
>         drm_dev_unregister(&priv->drm);
>         drm_atomic_helper_shutdown(&priv->drm);
> +       dev_set_drvdata(dev, NULL);
>  }
>  
>  static const struct component_master_ops ingenic_master_ops = {
> @@ -1461,6 +1474,14 @@ static int ingenic_drm_remove(struct
> platform_device *pdev)
>         return 0;
>  }
>  
> +static void ingenic_drm_shutdown(struct platform_device *pdev)
> +{
> +       struct ingenic_drm *priv = platform_get_drvdata(pdev);
> +
> +       if (priv)
> +               drm_atomic_helper_shutdown(&priv->drm);
> +}
> +
>  static int ingenic_drm_suspend(struct device *dev)
>  {
>         struct ingenic_drm *priv = dev_get_drvdata(dev);
> @@ -1612,6 +1633,7 @@ static struct platform_driver
> ingenic_drm_driver = {
>         },
>         .probe = ingenic_drm_probe,
>         .remove = ingenic_drm_remove,
> +       .shutdown = ingenic_drm_shutdown,
>  };
>  
>  static int ingenic_drm_init(void)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ