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.syrj...@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.d...@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
