Re: [PATCH] drm/crtc: constify drm_crtc_index parameter

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 10, 2016 at 06:26:10PM +0300, Jani Nikula wrote:
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> I needed this for some stuff that turned out to be a dead end. But this
> seems to be the right thing to do anyway.
> ---
>  include/drm/drm_crtc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 61932f55f788..0aa292526567 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1342,7 +1342,7 @@ extern void drm_crtc_cleanup(struct drm_crtc *crtc);
>   * Given a registered CRTC, return the index of that CRTC within a DRM
>   * device's list of CRTCs.
>   */
> -static inline unsigned int drm_crtc_index(struct drm_crtc *crtc)
> +static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc)
>  {
>  	return crtc->index;

BTW speaking about the index stuff. It dawned on me recently that calling
drm_crtc_cleanup() & co. is totally not safe except maybe during the
final cleanup.

If you would do something like:
a = drm_crtc_init();
b = drm_crtc_init();
drm_crtc_cleanup(a);
c = drm_crtc_init();

b and c would end up with the same index.


Or if you would do just
a = drm_crtc_init();
b = drm_crtc_init();
drm_crtc_cleanup(a);

We'd end up with num_crtc==1, but b->index==1, so we'd actually access
beyond the allocated arrays in a bunch of places.

This would need to fixed somehow, or at least documented clearly that if
you have to call any of the cleanup functions during init, you have to
abort the entire thing.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel