On Wed, 03 Sep 2025, "Nautiyal, Ankit K" <ankit.k.nauti...@intel.com> wrote:
> On 9/3/2025 1:07 PM, Jani Nikula wrote:
>> On Wed, 03 Sep 2025, Ankit Nautiyal <ankit.k.nauti...@intel.com> wrote:
>>> The helper intel_encoder_to_tc() can potentially return TC_PORT_NONE
>>> (-1) and cause overflow while computing ddc pins in
>>> icl_encoder_to_ddc_pin().
>>>
>>> Check for TC_PORT_NONE before deriving the ddc pins for TC port.
>>>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nauti...@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_hdmi.c | 12 +++++++++---
>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
>>> b/drivers/gpu/drm/i915/display/intel_hdmi.c
>>> index cbee628eb26b..85f70cedc40c 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
>>> @@ -2791,10 +2791,16 @@ static u8 icl_encoder_to_ddc_pin(struct
>>> intel_encoder *encoder)
>>> struct intel_display *display = to_intel_display(encoder);
>>> enum port port = encoder->port;
>>>
>>> - if (intel_encoder_is_combo(encoder))
>>> + if (intel_encoder_is_combo(encoder)) {
>>> return GMBUS_PIN_1_BXT + port;
>>> - else if (intel_encoder_is_tc(encoder))
>>> - return GMBUS_PIN_9_TC1_ICP + intel_encoder_to_tc(encoder);
>>> + } else if (intel_encoder_is_tc(encoder)) {
>>> + enum tc_port tc_port = intel_encoder_to_tc(encoder);
>> intel_encoder_to_tc() can only return TC_PORT_NONE if intel_phy_is_tc()
>> == false. But intel_encoder_is_tc() just above means intel_phy_is_tc()
>> == true.
>>
>> This case can't happen, it's a static analyzer being overzealous.
>
> Agreed, in this case tc_port is valid, I can see that now.
>
>
>>
>> Adding checks like this to please a static analyzer leads to misery,
>> because it adds unnecessary code to maintain, and it will never be run.
>>
>> I think it would be more interesting to make intel_port_to_tc() WARN on
>> !intel_phy_is_tc(), return a bogus >= 0 port, and ensure all callers
>> only call it on TC ports.
>
> Hmm.. I can try this out. Can change intel_port_to_tc() as suggested and
> ensure the callers call intel_phy_is_tc(),
>
> before calling intel_port_to_tc()/intel_encoder_to_tc().
>
> There are few places where the callers of intel_port_to_tc() and
> intel_encoder_to_tc() specifically check for TC_PORT_NONE,
>
> so those places need to be changed as well.
>
>
> Thanks for the suggestions and comments. I will drop this patch, and
> will prepare for the suggested changes.
I think with those changes you can eradicate TC_PORT_NONE altogether,
and get rid of a whole class of static analyzer warnings. And the code
becomes cleaner all around.
I've had this patch [1] for more than a year, maybe finally time to
merge it as prep work. You'll need to add tc checks in there, so it's
cleaner to lift it from intel_ddi_init(). And you can have separate code
paths for tc and non-tc, making the whole thing much cleaner I think.
BR,
Jani.
[1] https://lore.kernel.org/r/20250903101050.3671305-1-jani.nik...@intel.com
>
> Regards,
>
> Ankit
>
>>
>> This obviously leads to issues if it happens, but hey, it shouldn't
>> happen, and intel_encoder_to_tc() returning TC_PORT_NONE is *already*
>> such a case. Just move it to lower levels.
>>
>> If we start checking for every impossible situation, and propagating
>> errors for them, our codebase will be 90% error handling.
>>
>>
>> BR,
>> Jani.
>>
>>
>>> +
>>> + if (tc_port != TC_PORT_NONE)
>>> + return GMBUS_PIN_9_TC1_ICP + tc_port;
>>> +
>>> + drm_WARN(display->drm, 1, "Invalid TC port\n");
>>> + }
>>>
>>> drm_WARN(display->drm, 1, "Unknown port:%c\n", port_name(port));
>>> return GMBUS_PIN_2_BXT;
--
Jani Nikula, Intel
