Re: [PATCHv2 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:
- [High] media: cec: missing definitions for CEC_CAP_HAS_EDID_PRESENT and CEC_LOG_ADDRS_FL_EDID_PRESENT
- [Medium] media: cec: cec_notifier falsely sets CEC_LOG_ADDRS_FL_EDID_PRESENT via cec_s_phys_addr()
--

commit 4b86ce283c2fd61b5f6b3978888a26f706c9da03
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: cec: core: add support for EDID_PRESENT

This commit adds tracking for whether an EDID is present when updating or
invalidating the CEC physical address, setting CEC_LOG_ADDRS_FL_EDID_PRESENT
in cec_s_phys_addr() and clearing it in cec_phys_addr_invalidate().

> diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
> index ee8a200b9803f..1a7b79916e47d 100644
> --- a/drivers/media/cec/core/cec-adap.c
> +++ b/drivers/media/cec/core/cec-adap.c
> @@ -1727,18 +1727,35 @@ 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;
> +	} else {
> +		edid_present = false;
> +	}

[Severity: High]
Were the header changes defining CEC_CAP_HAS_EDID_PRESENT and
CEC_LOG_ADDRS_FL_EDID_PRESENT in include/uapi/linux/cec.h accidentally omitted
from this commit?

In drivers/media/cec/core/cec-adap.c, __cec_s_phys_addr() and
__cec_s_log_addrs() reference CEC_LOG_ADDRS_FL_EDID_PRESENT and
CEC_CAP_HAS_EDID_PRESENT, and cec_allocate_adapter() in
drivers/media/cec/core/cec-core.c also uses CEC_CAP_HAS_EDID_PRESENT, causing
the build to fail with undeclared identifier errors when CONFIG_CEC_CORE is
enabled.

[ ... ]

> @@ -1767,11 +1784,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]
Does passing edid_present = true unconditionally in cec_s_phys_addr() break
EDID presence tracking for drivers using cec_notifier?

In include/media/cec-notifier.h, cec_notifier_phys_addr_invalidate() still
calls cec_notifier_set_phys_addr(n, CEC_PHYS_ADDR_INVALID):

include/media/cec-notifier.h:
static inline void cec_notifier_phys_addr_invalidate(struct cec_notifier *n)
{
	cec_notifier_set_phys_addr(n, CEC_PHYS_ADDR_INVALID);
}

And in drivers/media/cec/core/cec-notifier.c, cec_notifier_set_phys_addr() and
cec_notifier_cec_adap_register() both call cec_s_phys_addr():

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);

cec_notifier_set_phys_addr()
	n->phys_addr = pa;
	if (n->cec_adap && !n->cec_adap->adap_controls_phys_addr)
		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);

As a result, when an adapter is registered via
cec_notifier_cec_adap_register() before any display is connected, when
cec_notifier_set_phys_addr_from_edid(n, NULL) is called, or when HPD goes low
and cec_notifier_phys_addr_invalidate() is called, cec_s_phys_addr() sets
CEC_LOG_ADDRS_FL_EDID_PRESENT instead of clearing it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/3a270f7742aaeadc15bceb727b8d8415caad9c76.1793691124.git.hverkuil+cisco@kernel.org?part=1