On 9/3/2025 3:46 PM, Jani Nikula wrote:
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
Great, this will indeed help, I was thinking about how to clean this up
before starting, and this makes it much cleaner.
Removing need for TC_PORT_NONE will further simplify things : just check
if intel_phy_is_tc() is true, then print the TC-style name otherwise,
use the regular DDI port format.
Thanks again for the guidance.
Regards,
Ankit
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;
