Re: [PATCH 3/3] drm/msm/dsi: check msm_dsi and dsi pointers before use

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 12, 2018 at 3:55 PM, Lloyd Atkinson <latkinso@xxxxxxxxxxxxxx> wrote:
> Move null checks of pointer arguments to the beginning of the
> modeset init function since they are referenced immediately
> instead of after they have already been used.
>
> Signed-off-by: Lloyd Atkinson <latkinso@xxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/dsi/dsi.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index 98742d7..be855db 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -196,7 +196,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>         struct drm_bridge *ext_bridge;
>         int ret;
>
> -       if (WARN_ON(!encoder))
> +       if (!encoder || !msm_dsi || !dev)

hmm, the checking if msm_dsi is null later in the fail: case is
certainly sketchy after we've already deref'd it..  so this looks like
the right thing.  But I'd like to keep the WARN_ON(), since this is a
case that shouldn't really happen.  The WARN_ON() nicely documents
that none of these parameters are expected to be NULL, and it gives a
big shouty message to anyone who inadvertently changes something that
breaks that assumption.  Other than that, it looks good.

BR,
-R


>                 return -EINVAL;
>
>         msm_dsi->dev = dev;
> @@ -245,19 +245,17 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>
>         return 0;
>  fail:
> -       if (msm_dsi) {
> -               /* bridge/connector are normally destroyed by drm: */
> -               if (msm_dsi->bridge) {
> -                       msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
> -                       msm_dsi->bridge = NULL;
> -               }
> +       /* bridge/connector are normally destroyed by drm: */
> +       if (msm_dsi->bridge) {
> +               msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
> +               msm_dsi->bridge = NULL;
> +       }
>
> -               /* don't destroy connector if we didn't make it */
> -               if (msm_dsi->connector && !msm_dsi->external_bridge)
> -                       msm_dsi->connector->funcs->destroy(msm_dsi->connector);
> +       /* don't destroy connector if we didn't make it */
> +       if (msm_dsi->connector && !msm_dsi->external_bridge)
> +               msm_dsi->connector->funcs->destroy(msm_dsi->connector);
>
> -               msm_dsi->connector = NULL;
> -       }
> +       msm_dsi->connector = NULL;
>
>         return ret;
>  }
> --
> QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux