| Message ID | 20231005064257.570671-1-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/ddi: Fix i2c_adapter assignment | expand |
Hi Suraj, On Thu, Oct 05, 2023 at 12:12:58PM +0530, Suraj Kandpal wrote: > i2c_adapter is being assigned using intel_connector even before the > NULL check occurs and even though it shouldn't be a problem > lets just clean this up as logically it does not make sense to check > the connector for NULL but dereference it before that. > > Fixes: e046d1562491 ("drm/i915/hdmi: Use connector->ddc everwhere") > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> please don't leave a blank line in the tag section. No need to resend, I guess whoever will merge this patch can fix it before pushing. Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Andi
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/ddi: Fix i2c_adapter assignment > > Hi Suraj, > > On Thu, Oct 05, 2023 at 12:12:58PM +0530, Suraj Kandpal wrote: > > i2c_adapter is being assigned using intel_connector even before the > > NULL check occurs and even though it shouldn't be a problem lets just > > clean this up as logically it does not make sense to check the > > connector for NULL but dereference it before that. > > > > Fixes: e046d1562491 ("drm/i915/hdmi: Use connector->ddc everwhere") > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > please don't leave a blank line in the tag section. No need to resend, I guess > whoever will merge this patch can fix it before pushing. > Hi Andi, Thanks for the review ! will keep this in mind in future Regards, Suraj Kandpal > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > > Andi
On Thu, Oct 05, 2023 at 12:12:58PM +0530, Suraj Kandpal wrote: > i2c_adapter is being assigned using intel_connector even before the > NULL check occurs and even though it shouldn't be a problem > lets just clean this up as logically it does not make sense to check > the connector for NULL but dereference it before that. > > Fixes: e046d1562491 ("drm/i915/hdmi: Use connector->ddc everwhere") > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 4668de45d6fe..6b658faf1fc3 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -4326,7 +4326,7 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder, > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_hdmi *hdmi = enc_to_intel_hdmi(encoder); > struct intel_connector *connector = hdmi->attached_connector; > - struct i2c_adapter *ddc = connector->base.ddc; > + struct i2c_adapter *ddc; > struct drm_connector_state *conn_state; > struct intel_crtc_state *crtc_state; > struct intel_crtc *crtc; > @@ -4336,6 +4336,8 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder, > if (!connector || connector->base.status != connector_status_connected) > return 0; The connector is never NULL here. So the check is just nonsense. > > + ddc = connector->base.ddc; > + > ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, > ctx); > if (ret) > -- > 2.25.1
On Thu, 05 Oct 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Oct 05, 2023 at 12:12:58PM +0530, Suraj Kandpal wrote: >> i2c_adapter is being assigned using intel_connector even before the >> NULL check occurs and even though it shouldn't be a problem >> lets just clean this up as logically it does not make sense to check >> the connector for NULL but dereference it before that. >> >> Fixes: e046d1562491 ("drm/i915/hdmi: Use connector->ddc everwhere") >> >> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_ddi.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c >> index 4668de45d6fe..6b658faf1fc3 100644 >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c >> @@ -4326,7 +4326,7 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder, >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> struct intel_hdmi *hdmi = enc_to_intel_hdmi(encoder); >> struct intel_connector *connector = hdmi->attached_connector; >> - struct i2c_adapter *ddc = connector->base.ddc; >> + struct i2c_adapter *ddc; >> struct drm_connector_state *conn_state; >> struct intel_crtc_state *crtc_state; >> struct intel_crtc *crtc; >> @@ -4336,6 +4336,8 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder, >> if (!connector || connector->base.status != connector_status_connected) >> return 0; > > The connector is never NULL here. So the check is just nonsense. Yeah I'd rather remove that. Leaving it in makes people (and static analyzers, apprently) think it could be NULL, and that leads to more what ifs. BR, Jani. > >> >> + ddc = connector->base.ddc; >> + >> ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, >> ctx); >> if (ret) >> -- >> 2.25.1
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/ddi: Fix i2c_adapter assignment > > On Thu, 05 Oct 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Oct 05, 2023 at 12:12:58PM +0530, Suraj Kandpal wrote: > >> i2c_adapter is being assigned using intel_connector even before the > >> NULL check occurs and even though it shouldn't be a problem lets just > >> clean this up as logically it does not make sense to check the > >> connector for NULL but dereference it before that. > >> > >> Fixes: e046d1562491 ("drm/i915/hdmi: Use connector->ddc everwhere") > >> > >> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_ddi.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > >> b/drivers/gpu/drm/i915/display/intel_ddi.c > >> index 4668de45d6fe..6b658faf1fc3 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > >> @@ -4326,7 +4326,7 @@ static int intel_hdmi_reset_link(struct > intel_encoder *encoder, > >> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >> struct intel_hdmi *hdmi = enc_to_intel_hdmi(encoder); > >> struct intel_connector *connector = hdmi->attached_connector; > >> - struct i2c_adapter *ddc = connector->base.ddc; > >> + struct i2c_adapter *ddc; > >> struct drm_connector_state *conn_state; > >> struct intel_crtc_state *crtc_state; > >> struct intel_crtc *crtc; > >> @@ -4336,6 +4336,8 @@ static int intel_hdmi_reset_link(struct > intel_encoder *encoder, > >> if (!connector || connector->base.status != > connector_status_connected) > >> return 0; > > > > The connector is never NULL here. So the check is just nonsense. > > Yeah I'd rather remove that. Leaving it in makes people (and static analyzers, > apprently) think it could be NULL, and that leads to more what ifs. > > BR, > Jani. > Makes sense will remove the connector check instead of moving the assignment around Regards, Suraj Kandpal > > > >> > >> + ddc = connector->base.ddc; > >> + > >> ret = drm_modeset_lock(&dev_priv- > >drm.mode_config.connection_mutex, > >> ctx); > >> if (ret) > >> -- > >> 2.25.1 > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 4668de45d6fe..6b658faf1fc3 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -4326,7 +4326,7 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder, struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_hdmi *hdmi = enc_to_intel_hdmi(encoder); struct intel_connector *connector = hdmi->attached_connector; - struct i2c_adapter *ddc = connector->base.ddc; + struct i2c_adapter *ddc; struct drm_connector_state *conn_state; struct intel_crtc_state *crtc_state; struct intel_crtc *crtc; @@ -4336,6 +4336,8 @@ static int intel_hdmi_reset_link(struct intel_encoder *encoder, if (!connector || connector->base.status != connector_status_connected) return 0; + ddc = connector->base.ddc; + ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, ctx); if (ret)
i2c_adapter is being assigned using intel_connector even before the NULL check occurs and even though it shouldn't be a problem lets just clean this up as logically it does not make sense to check the connector for NULL but dereference it before that. Fixes: e046d1562491 ("drm/i915/hdmi: Use connector->ddc everwhere") Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)