| Message ID | 1493214013-15580-3-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. In the meantime Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > 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@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
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@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@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
On Wed, Apr 26, 2017 at 06:23:42PM +0300, Imre Deak wrote: > 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? Hmm. Right, so in the case of one byte read we will still copy the actual data byte as well, if the hardware returned one. So it could still clobber the byte we would like to preserve. I think. > > > 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@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@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
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;
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. 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@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)