> -----Original Message----- > From: Jani Nikula <[email protected]> > Sent: Wednesday, October 9, 2024 3:20 PM > To: Kandpal, Suraj <[email protected]>; intel- > [email protected]; [email protected] > Cc: Nautiyal, Ankit K <[email protected]>; Kandpal, Suraj > <[email protected]> > Subject: Re: [PATCH] drm/i915/hdcp: Move dig_port assignment lower in the > sequence > > On Wed, 09 Oct 2024, Suraj Kandpal <[email protected]> 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? Yes this was 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. That change was brought about because I had seen a oops when hdcp_capable was being called from debugfs And I thought it would be better to fail safe other places too but I see you point how the code may end up getting Cluttering up with useless checks Regards, Suraj Kandpal > > BR, > Jani. > > > > > > Signed-off-by: Suraj Kandpal <[email protected]> > > --- > > 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
