On Wed, 09 Oct 2024, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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