[Intel-gfx] [PATCH 2/8] drm/i915/dp: Check error return during DPCD capability queries

[Imre Deak]

On Wed, Apr 26, 2017 at 06:08:24PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 26, 2017 at 04:40:07PM +0300, Imre Deak wrote:
> > The assumptions of these users of drm_dp_dpcd_readb() is that the passed
> > in output buffer won't change in case of error, but this isn't
> > guaranteed.
> 
> Hmm. We blindly copy as many bytes from the rxbuf into the user
> provided buffer as the hardware told us to. 

Haven't checked this, but now looking at it doesn't the 
'bytes > recv_size' check in intel_dp_aux_ch() bound that?

> And whether the transfer
> actually is considered a success or not depends on what the first
> received byte says. I don't recall what the DP spec really says about
> replying with more than one byte on failure, but I guess we shouldn't
> depend on it anyway.
> 
> We could actually make that guarantee if we moved txbuf+rxbuf up
> into drm_dp_dpcd_access() and did the copy to the user buffer
> only on succes. But that would require changing all the DP
> capable drivers at the same time, so would require someone
> motivated enough.

Ok. I stopped at the ZR24w workaround in drm_dp_dpcd_read().. But that
could also be used with a separate buffer. In any case I thought that
even with the guarantee that the buffer doesn't change - which is in
general a reasonable assumption - checking for the error return would
be more robust.

> 
> In the meantime
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Thanks.

> 
> > Fix this by treating any error as the lack of the given
> > capability.
> > 
> > In case of DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP an error would leave the
> > buffer uninitialized even with the above assumption.
> > 
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 08834f7..4a6feb6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3088,7 +3088,8 @@ static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> >  {
> >  	uint8_t psr_caps = 0;
> >  
> > -	drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps);
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_CAPS, &psr_caps) != 1)
> > +		return false;
> >  	return psr_caps & DP_PSR2_SU_Y_COORDINATE_REQUIRED;
> >  }
> >  
> > @@ -3096,9 +3097,9 @@ static bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
> >  {
> >  	uint8_t dprx = 0;
> >  
> > -	drm_dp_dpcd_readb(&intel_dp->aux,
> > -			DP_DPRX_FEATURE_ENUMERATION_LIST,
> > -			&dprx);
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
> > +			      &dprx) != 1)
> > +		return false;
> >  	return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
> >  }
> >  
> > @@ -3106,7 +3107,9 @@ static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
> >  {
> >  	uint8_t alpm_caps = 0;
> >  
> > -	drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP, &alpm_caps);
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_RECEIVER_ALPM_CAP,
> > +			      &alpm_caps) != 1)
> > +		return false;
> >  	return alpm_caps & DP_ALPM_CAP;
> >  }
> >  
> > @@ -3679,9 +3682,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> >  		uint8_t frame_sync_cap;
> >  
> >  		dev_priv->psr.sink_support = true;
> > -		drm_dp_dpcd_readb(&intel_dp->aux,
> > -				  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > -				  &frame_sync_cap);
> > +		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > +				      DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > +				      &frame_sync_cap) != 1)
> > +			frame_sync_cap = 0;
> >  		dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
> >  		/* PSR2 needs frame sync as well */
> >  		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC