Re: [PATCH 4/4] drm/i915/display: harden shift in intel_pmdemand_update_phys_mask()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 19 May 2026, Luca Coelho <luciano.coelho@intel.com> wrote:
> BIT() takes a non-negative shift amount, but phy is of type enum phy,
> which can in theory be PHY_NONE (-1).
>
> This is not a problem with the current implementation, because phy is
> always valid when this code is reached (Type-C encoders are rejected
> earlier), but it's more robust to store it as unsigned int so the
> shifts are always well-defined.
>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_pmdemand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_pmdemand.c b/drivers/gpu/drm/i915/display/intel_pmdemand.c
> index 7819b724795b..d02ac2408e29 100644
> --- a/drivers/gpu/drm/i915/display/intel_pmdemand.c
> +++ b/drivers/gpu/drm/i915/display/intel_pmdemand.c
> @@ -150,7 +150,7 @@ intel_pmdemand_update_phys_mask(struct intel_display *display,
>  				struct intel_pmdemand_state *pmdemand_state,
>  				bool set_bit)
>  {
> -	enum phy phy;
> +	unsigned int phy;

I think it's useful to keep the enum here. That's the type.

I also don't believe in using unsigned types to enforce something is
positive, because this just turns -1 into 4294967295, which isn't better
at all.

I'd rather see a (redundant and silly) assert for phy being >= 0 than
using this trick to enforce it's >= 0.

But let's take a step back instead.

Look at all the places that reference PHY_NONE. Looks like
icl_aux_pw_to_phy() is the only place that actually uses PHY_NONE, and
all the other places are there just to work around the fact that
theoretically phy might be < 0. Which is never. And the return value of
icl_aux_pw_to_phy() is never even checked, it's just passed directly to
phy_name().

Maybe the solution is to just open code icl_aux_pw_to_phy(), handling
encoder being NULL inline, and nuke PHY_NONE and the checks for PHY_NONE
altogether?

*shrug*

BR,
Jani.





>  
>  	if (DISPLAY_VER(display) < 14)
>  		return;

-- 
Jani Nikula, Intel