[PATCH] gpu: drm_dp_cec: fix broken CEC adapter properties check

[Hans Verkuil]

On 29/01/2025 10:51, Hans Verkuil wrote:
> If the hotplug detect of a display is low for longer than one second
> (configurable through drm_dp_cec_unregister_delay), then the CEC adapter
> is unregistered since we assume the display was disconnected. If the
> HPD went low for less than one second, then we check if the properties
> of the CEC adapter have changed, since that indicates that we actually
> switch to new hardware and we have to unregister the old CEC device and
> register a new one.
> 
> Unfortunately, the test for changed properties was written poorly, and
> after a new CEC capability was added to the CEC core code the test always
> returned true (i.e. the properties had changed).
> 
> As a result the CEC device was unregistered and re-registered for every
> HPD toggle. If the CEC remote controller integration was also enabled
> (CONFIG_MEDIA_CEC_RC was set), then the corresponding input device was
> also unregistered and re-registered. As a result the input device in
> /sys would keep incrementing its number, e.g.:
> 
> /sys/devices/pci0000:00/0000:00:08.1/0000:e7:00.0/rc/rc0/input20
> 
> Since short HPD toggles are common, the number could over time get into
> the thousands.
> 
> While not a serious issue (i.e. nothing crashes), it is not intended
> to work that way.
> 
> This patch changes the test so that it only checks for the single CEC
> capability that can actually change, and it ignores any other
> capabilities, so this is now safe as well if new caps are added in
> the future.
> 
> With the changed test the bit under #ifndef CONFIG_MEDIA_CEC_RC can be
> dropped as well, so that's a nice cleanup.
> 
> Signed-off-by: Hans Verkuil <hverkuil at xs4all.nl>
> Reported-by: Farblos <farblos at vodafonemail.de>

Fixes: 2c6d1fffa1d9 ("drm: add support for DisplayPort CEC-Tunneling-over-AUX")

Regards,

	Hans

> ---
> Jens (aka Farblos), can you test this patch?
> ---
>  drivers/gpu/drm/display/drm_dp_cec.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_cec.c b/drivers/gpu/drm/display/drm_dp_cec.c
> index 007ceb281d00..56a4965e518c 100644
> --- a/drivers/gpu/drm/display/drm_dp_cec.c
> +++ b/drivers/gpu/drm/display/drm_dp_cec.c
> @@ -311,16 +311,6 @@ void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address)
>  	if (!aux->transfer)
>  		return;
> 
> -#ifndef CONFIG_MEDIA_CEC_RC
> -	/*
> -	 * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> -	 * cec_allocate_adapter() if CONFIG_MEDIA_CEC_RC is undefined.
> -	 *
> -	 * Do this here as well to ensure the tests against cec_caps are
> -	 * correct.
> -	 */
> -	cec_caps &= ~CEC_CAP_RC;
> -#endif
>  	cancel_delayed_work_sync(&aux->cec.unregister_work);
> 
>  	mutex_lock(&aux->cec.lock);
> @@ -337,7 +327,9 @@ void drm_dp_cec_attach(struct drm_dp_aux *aux, u16 source_physical_address)
>  		num_las = CEC_MAX_LOG_ADDRS;
> 
>  	if (aux->cec.adap) {
> -		if (aux->cec.adap->capabilities == cec_caps &&
> +		/* Check if the adapter properties have changed */
> +		if ((aux->cec.adap->capabilities & CEC_CAP_MONITOR_ALL) ==
> +		    (cec_caps & CEC_CAP_MONITOR_ALL) &&
>  		    aux->cec.adap->available_log_addrs == num_las) {
>  			/* Unchanged, so just set the phys addr */
>  			cec_s_phys_addr(aux->cec.adap, source_physical_address, false);