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 Mon, Jan 15, 2018 at 10:01 AM, Lloyd Atkinson
<latkinso@xxxxxxxxxxxxxx> wrote:
> On 1/15/2018 9:48 AM, Rob Clark wrote:
>> 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
>
> Sure. Do you want to add WARN_ONs to msm_dsi and dev as well?
>

yup, thanks

BR,
-R

> Thanks,
> Lloyd
>
>>
>>
>>>                 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
>
>
> --
> 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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux