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

[Lloyd Atkinson <latkinso@codeaurora.org>]

On 1/15/2018 9:48 AM, Rob Clark wrote:
> On Fri, Jan 12, 2018 at 3:55 PM, Lloyd Atkinson <latkinso@codeaurora.org> 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@codeaurora.org>
>> ---
>>  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?

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@vger.kernel.org
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel