Re: [PATCHv3 09/13] media: cec: core: add support for HPD_PRESENT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: cec: undeclared CEC_CAP_HAS_HPD_PRESENT and CEC_LOG_ADDRS_FL_HPD_PRESENT
- [Medium] media: cec: cec_s_phys_addr() falsely sets HPD_PRESENT via cec_notifier
--

commit 9869442bbf59dec664959906e52999513a57f708
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: cec: core: add support for HPD_PRESENT

This commit adds support for tracking and reporting whether the HPD is
present via the CEC_LOG_ADDRS_FL_HPD_PRESENT flag and
CEC_CAP_HAS_HPD_PRESENT capability. It updates __cec_s_phys_addr(),
cec_s_phys_addr(), cec_s_phys_addr_from_edid(), and
cec_phys_addr_invalidate() to update this state.

> diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c
> index ee8a200b9803f..55b2e711fe77f 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 hpd_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 hpd_was_present =
> +		adap->log_addrs.flags & CEC_LOG_ADDRS_FL_HPD_PRESENT;
>  
> -	if (phys_addr == adap->phys_addr)
> -		return;
> +	if (adap->capabilities & CEC_CAP_HAS_HPD_PRESENT) {
> +		if (hpd_present)
> +			adap->log_addrs.flags |= CEC_LOG_ADDRS_FL_HPD_PRESENT;
> +		else
> +			adap->log_addrs.flags &= ~CEC_LOG_ADDRS_FL_HPD_PRESENT;
> +	} else {
> +		hpd_present = false;
> +	}

[Severity: Medium]
Was a hunk for include/uapi/linux/cec.h left out of this patch?

In drivers/media/cec/core/cec-adap.c, __cec_s_phys_addr() and
__cec_s_log_addrs() reference CEC_LOG_ADDRS_FL_HPD_PRESENT and
CEC_CAP_HAS_HPD_PRESENT, and cec_allocate_adapter() in
drivers/media/cec/core/cec-core.c also references CEC_CAP_HAS_HPD_PRESENT:

	if (!(caps & CEC_CAP_PHYS_ADDR))
		adap->capabilities |= CEC_CAP_HAS_HPD_PRESENT;

However, include/uapi/linux/cec.h is not updated by this commit and neither
macro is defined in the tree, causing the build to fail when CONFIG_CEC_CORE
is enabled.

>  	if (!becomes_invalid && adap->devnode.unregistered)
>  		return;
> +	if (phys_addr == adap->phys_addr) {
> +		if (hpd_was_present != hpd_present) {
> +			dprintk(1, "HPD is %spresent\n",
> +				hpd_present ? "" : "not ");
> +			cec_post_state_event(adap);
> +		}
> +		return;
> +	}
>  
> -	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, HPD is %spresent\n",
> +		cec_phys_addr_exp(phys_addr), hpd_present ? "" : "not ");
>  	if (becomes_invalid || !is_invalid) {
>  		adap->phys_addr = CEC_PHYS_ADDR_INVALID;
>  		cec_adap_unconfigure(adap);
> @@ -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 hpd_present = true unconditionally in cec_s_phys_addr() cause
CEC_LOG_ADDRS_FL_HPD_PRESENT to be falsely set for adapters using
cec_notifier or invalidating the physical address via cec_s_phys_addr()?

In drivers/media/cec/core/cec-core.c, cec_allocate_adapter() sets
CEC_CAP_HAS_HPD_PRESENT on all adapters without CEC_CAP_PHYS_ADDR, which
includes cec_notifier-based drivers.

In drivers/media/cec/core/cec-notifier.c, cec_notifier_cec_adap_register()
initializes the adapter's physical address via cec_s_phys_addr():

	if (!adap->adap_controls_phys_addr)
		cec_s_phys_addr(adap, n->phys_addr, false);

When no sink is connected at registration time, n->phys_addr is
CEC_PHYS_ADDR_INVALID, so calling cec_s_phys_addr() sets
CEC_LOG_ADDRS_FL_HPD_PRESENT and posts a state change event even though HPD
is low.

Similarly, when HPD goes low, cec_notifier_phys_addr_invalidate() in
include/media/cec-notifier.h and cec_notifier_set_phys_addr_from_edid(n, NULL)
in drivers/media/cec/core/cec-notifier.c both route through
cec_notifier_set_phys_addr(n, CEC_PHYS_ADDR_INVALID):

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

Because this calls cec_s_phys_addr() rather than cec_phys_addr_invalidate(),
and other callers such as drm_dp_cec_attach() (when called from
drm_dp_cec_set_edid(aux, NULL)), hdmi4_bridge_hpd_notify(), and
adv7511_check_monitor_present_status() also pass CEC_PHYS_ADDR_INVALID to
cec_s_phys_addr() on disconnect, won't CEC_LOG_ADDRS_FL_HPD_PRESENT remain
set when HPD is lost?

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