Re: [PATCH 09/13] media: cec: core: add support for EDID_PRESENT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: cec: spurious state events in __cec_s_phys_addr() without cap
- [Medium] media: cec: cec_s_phys_addr() falsely sets EDID_PRESENT without EDID
--

commit 1fe777ffdbc4971735bf4636dac9ebebeba91665
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: cec: core: add support for EDID_PRESENT

This commit adds support for reporting whether an EDID is present when the
CEC physical address is set or invalidated. It updates cec_s_phys_addr() to
set CEC_LOG_ADDRS_FL_EDID_PRESENT and changes cec_phys_addr_invalidate() to
clear it when the EDID is lost.

> diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
> index ee8a200b9803f..06a596a0ede3e 100644
> --- a/drivers/media/cec/core/cec-adap.c
> +++ b/drivers/media/cec/core/cec-adap.c
> @@ -1727,18 +1727,33 @@ int cec_adap_enable(struct cec_adapter *adap)
>   *
>   * This function is called with adap->lock held.
>   */
> -void __cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
> +void __cec_s_phys_addr(struct cec_adapter *adap, bool edid_present,
> +		       u16 phys_addr, bool block)
>  {
>  	bool becomes_invalid = phys_addr == CEC_PHYS_ADDR_INVALID;
>  	bool is_invalid = adap->phys_addr == CEC_PHYS_ADDR_INVALID;
> +	bool edid_was_present =
> +		adap->log_addrs.flags & CEC_LOG_ADDRS_FL_EDID_PRESENT;
>  
> -	if (phys_addr == adap->phys_addr)
> -		return;
> +	if (adap->capabilities & CEC_CAP_HAS_EDID_PRESENT) {
> +		if (edid_present)
> +			adap->log_addrs.flags |= CEC_LOG_ADDRS_FL_EDID_PRESENT;
> +		else
> +			adap->log_addrs.flags &= ~CEC_LOG_ADDRS_FL_EDID_PRESENT;
> +	}
>  	if (!becomes_invalid && adap->devnode.unregistered)
>  		return;
> +	if (phys_addr == adap->phys_addr) {
> +		if (edid_was_present != edid_present) {
> +			dprintk(1, "EDID is %spresent\n",
> +				edid_present ? "" : "not ");
> +			cec_post_state_event(adap);
> +		}
> +		return;
> +	}

[Severity: Medium]
Can this queue spurious CEC_EVENT_STATE_CHANGE events when an adapter does
not have CEC_CAP_HAS_EDID_PRESENT?

In cec_allocate_adapter(), adapters allocated with CEC_CAP_PHYS_ADDR do not
get CEC_CAP_HAS_EDID_PRESENT:

drivers/media/cec/core/cec-core.c:cec_allocate_adapter()
	if (!(caps & CEC_CAP_PHYS_ADDR))
		adap->capabilities |= CEC_CAP_HAS_EDID_PRESENT;

Because __cec_s_phys_addr() only sets CEC_LOG_ADDRS_FL_EDID_PRESENT in
adap->log_addrs.flags when CEC_CAP_HAS_EDID_PRESENT is set, edid_was_present
remains permanently false for those adapters.

However, cec_adap_s_phys_addr() (for the CEC_ADAP_S_PHYS_ADDR ioctl) and
cec_s_phys_addr() still pass edid_present = true when setting a valid
physical address. Whenever the same valid physical address is set again on
such an adapter (phys_addr == adap->phys_addr), edid_was_present !=
edid_present evaluates to true and cec_post_state_event(adap) is called even
though neither adap->phys_addr nor adap->log_addrs.flags changed.

Should edid_present be forced to false when !(adap->capabilities &
CEC_CAP_HAS_EDID_PRESENT), or should the edid_was_present != edid_present
check be gated on CEC_CAP_HAS_EDID_PRESENT?

>  
> -	dprintk(1, "new physical address %x.%x.%x.%x\n",
> -		cec_phys_addr_exp(phys_addr));
> +	dprintk(1, "new physical address %x.%x.%x.%x, EDID is %spresent\n",
> +		cec_phys_addr_exp(phys_addr), edid_present ? "" : "not ");
>  	if (becomes_invalid || !is_invalid) {
>  		adap->phys_addr = CEC_PHYS_ADDR_INVALID;
>  		cec_adap_unconfigure(adap);
> @@ -1767,11 +1782,22 @@ void cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
>  		return;
>  
>  	mutex_lock(&adap->lock);
> -	__cec_s_phys_addr(adap, phys_addr, block);
> +	__cec_s_phys_addr(adap, true, phys_addr, block);
>  	mutex_unlock(&adap->lock);
>  }
>  EXPORT_SYMBOL_GPL(cec_s_phys_addr);

[Severity: Medium]
Won't passing true unconditionally here cause existing callers that pass
CEC_PHYS_ADDR_INVALID when no EDID is present to falsely report
CEC_LOG_ADDRS_FL_EDID_PRESENT?

For example, in drivers/media/cec/core/cec-notifier.c, struct cec_notifier
only tracks n->phys_addr (which starts as CEC_PHYS_ADDR_INVALID when no
sink is connected) and does not track whether an EDID is present. When a
CEC adapter registers with a notifier while disconnected,
cec_notifier_cec_adap_register() calls cec_s_phys_addr() with
CEC_PHYS_ADDR_INVALID:

drivers/media/cec/core/cec-notifier.c:cec_notifier_cec_adap_register()
	if (!adap->adap_controls_phys_addr)
		cec_s_phys_addr(adap, n->phys_addr, false);

Similarly, cec_notifier_set_phys_addr_from_edid() calls
cec_notifier_set_phys_addr(n, CEC_PHYS_ADDR_INVALID) -> cec_s_phys_addr()
even when edid is NULL:

drivers/media/cec/core/cec-notifier.c:cec_notifier_set_phys_addr_from_edid()
	if (edid && edid->extensions)
		pa = cec_get_edid_phys_addr((const u8 *)edid,
				EDID_LENGTH * (edid->extensions + 1), NULL);
	cec_notifier_set_phys_addr(n, pa);

And drm_dp_cec_attach() in drivers/gpu/drm/display/drm_dp_cec.c also calls
cec_s_phys_addr(aux->cec.adap, source_physical_address, false) when
called from drm_dp_cec_register_connector() with no EDID and
source_physical_address set to CEC_PHYS_ADDR_INVALID.

In all of those paths, __cec_s_phys_addr() will now set
CEC_LOG_ADDRS_FL_EDID_PRESENT and post a state change event even though no
EDID is present. Do cec_notifier and drm_dp_cec need to be updated to track
or invalidate EDID presence accordingly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793615824.git.hverkuil+cisco@kernel.org?part=9