Re: [PATCH] drm/bridge: Add null pointer check for ITE IT6263

[Liu Ying <victor.liu@oss.nxp.com>]

Hi Chenyuan,

On 07/23/2025, Chenyuan Yang wrote:
> On Tue, Jul 22, 2025 at 23:57 Maxime Ripard <mripard@kernel.org> wrote:
> 
>> On Tue, Jul 22, 2025 at 03:41:14PM -0500, Chenyuan Yang wrote:
>>> drm_atomic_get_new_connector_for_encoder and
>>> drm_atomic_get_new_connector_state could return Null.
>>
>> They can, but not in that scenario. atomic_enable will never be called
>> if either would return NULL.
>>
>> In which situation did you trigger this bug?
> 
> 
> This is found by our static analysis tool based on the fact that
> drm_atomic_get_new_connector_state() could return NULL. We also noticed
> that under the same dir, ITE IT6505 transmitter has such checks. Thus, we
> assume it would be good to have similar checks here.

You need to properly triage the tool's report, which means if there is
no real case that could produce those NULL pointers, then the report
should be false-positive, otherwise show us any real case that could in
commit message.  Assuming the checks are needed is not good enough.
Please do your home work before sending the patch.

> 
> 
>>> Thus, add the null pointer check for them with a similar format with
>>> it6505_bridge_atomic_enable in ITE IT6505.
>>>
>>> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
>>> Fixes: 049723628716 ("drm/bridge: Add ITE IT6263 LVDS to HDMI converter")
>>> ---
>>>  drivers/gpu/drm/bridge/ite-it6263.c | 15 ++++++++++++++-
>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/ite-it6263.c
>> b/drivers/gpu/drm/bridge/ite-it6263.c
>>> index a3a63a977b0a..3a20b2088bf9 100644
>>> --- a/drivers/gpu/drm/bridge/ite-it6263.c
>>> +++ b/drivers/gpu/drm/bridge/ite-it6263.c
>>> @@ -590,15 +590,28 @@ static void it6263_bridge_atomic_enable(struct
>> drm_bridge *bridge,
>>>       struct drm_connector *connector;
>>>       bool is_stable = false;
>>>       struct drm_crtc *crtc;
>>> +     struct drm_connector_state *conn_state;
>>>       unsigned int val;
>>>       bool pclk_high;
>>>       int i, ret;
>>>
>>>       connector = drm_atomic_get_new_connector_for_encoder(state,
>>>
>> bridge->encoder);
>>> -     crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
>>> +     if (WARN_ON(!connector))
>>> +             return;
>>> +
>>> +     conn_state = drm_atomic_get_new_connector_state(state, connector);
>>> +     if (WARN_ON(!conn_state))
>>> +             return;
>>> +
>>> +     crtc = conn_state->crtc;
>>>       crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>>> +     if (WARN_ON(!crtc_state))
>>> +             return;
>>> +
>>>       mode = &crtc_state->adjusted_mode;
>>> +     if (WARN_ON(!mode))
>>> +             return;
>>
>> And that condition can never be true.
>>
>> Maxime
>>
> 


-- 
Regards,
Liu Ying