Re: [PATCH] drm/i915/hdcp: Move dig_port assignment lower in the sequence

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

On Wed, 09 Oct 2024, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> Move dig_port assignment much lower in the sequence to avoid NULL
> pointer deference in case encoder is not present.

Please describe the case exactly. Is this real or a static analyzer
warning?

I see there's commit 6c63e6e14da7 ("drm/i915/hdcp: No HDCP when encoder
is't initialized") adding the !connector->encoder check. But how was
that supposed to fix anything when it leaves

	struct intel_digital_port *dig_port = intel_attached_dig_port(connector);

above it in place, the line you're fixing now?

If we haven't seen an oops here, it makes me think the reasoning in
6c63e6e14da7 is bogus, the connector->encoder check is bogus, and this
fix on top is also bogus.

OTOH if we have seen a real issue, we need the backtrace and the
conditions triggering it, and we need to backport this.

While it may seem benign and defensive to add extra NULL checks, one of
the dangers is the cargo culting of those checks. Static analyzers see a
check somewhere, so they complain about unchecked dereferences
everywhere. Checks start cropping up everywhere, and people lose track
of when it's actually possible for the pointers to be NULL, and when
not.

BR,
Jani.


>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index ed6aa87403e2..ea8d56b25f6e 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> @@ -2404,7 +2404,7 @@ static int _intel_hdcp_enable(struct intel_atomic_state *state,
>  	struct drm_i915_private *i915 = to_i915(display->drm);
>  	struct intel_connector *connector =
>  		to_intel_connector(conn_state->connector);
> -	struct intel_digital_port *dig_port = intel_attached_dig_port(connector);
> +	struct intel_digital_port *dig_port;
>  	struct intel_hdcp *hdcp = &connector->hdcp;
>  	unsigned long check_link_interval = DRM_HDCP_CHECK_PERIOD_MS;
>  	int ret = -EINVAL;
> @@ -2418,6 +2418,8 @@ static int _intel_hdcp_enable(struct intel_atomic_state *state,
>  		return -ENODEV;
>  	}
>  
> +	dig_port = intel_attached_dig_port(connector);
> +
>  	mutex_lock(&hdcp->mutex);
>  	mutex_lock(&dig_port->hdcp_mutex);
>  	drm_WARN_ON(display->drm,

-- 
Jani Nikula, Intel