Re: [PATCHv3 06/13] media: cec: core: improve cec_notifier_phys_addr_invalidate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: cec: no-op HPD distinction in cec_notifier_phys_addr_invalidate()
- [Low] media: cec: invalid @ prefix on cec_notifier_phys_addr_invalidate() doc ref
--

commit 6215e9e4297c5764a7d09898f00380f708aab802
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: cec: core: improve cec_notifier_phys_addr_invalidate

This commit moves cec_notifier_phys_addr_invalidate() into cec-notifier.c
so it calls cec_phys_addr_invalidate() instead of cec_s_phys_addr(), and
tracks edid_present in struct cec_notifier so cec_notifier_cec_adap_register()
can distinguish between a disconnected sink and an EDID without a valid
physical address.

> diff --git a/drivers/media/cec/core/cec-notifier.c b/drivers/media/cec/core/cec-notifier.c
> index 8cf1474d0361c..5a4588487ea34 100644
> --- a/drivers/media/cec/core/cec-notifier.c
> +++ b/drivers/media/cec/core/cec-notifier.c

[ ... ]

> @@ -162,8 +165,12 @@ cec_notifier_cec_adap_register(struct device *hdmi_dev, const char *port_name,
>  	n->cec_adap = adap;
>  	adap->conn_info = n->conn_info;
>  	adap->notifier = n;
> -	if (!adap->adap_controls_phys_addr)
> -		cec_s_phys_addr(adap, n->phys_addr, false);
> +	if (!adap->adap_controls_phys_addr) {
> +		if (n->phys_addr == CEC_PHYS_ADDR_INVALID && !n->edid_present)
> +			cec_phys_addr_invalidate(adap);
> +		else
> +			cec_s_phys_addr(adap, n->phys_addr, false);
> +	}
>  	mutex_unlock(&n->lock);
>  	return n;
>  }
> @@ -190,6 +197,7 @@ void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa)
>  
>  	mutex_lock(&n->lock);
>  	n->phys_addr = pa;
> +	n->edid_present = true;
>  	if (n->cec_adap && !n->cec_adap->adap_controls_phys_addr)
>  		cec_s_phys_addr(n->cec_adap, n->phys_addr, false);
>  	mutex_unlock(&n->lock);
> @@ -216,6 +224,20 @@ void cec_notifier_set_phys_addr_from_edid(struct cec_notifier *n,
>  }
>  EXPORT_SYMBOL_GPL(cec_notifier_set_phys_addr_from_edid);
>  
> +void cec_notifier_phys_addr_invalidate(struct cec_notifier *n)
> +{
> +	if (!n)
> +		return;
> +
> +	mutex_lock(&n->lock);
> +	n->phys_addr = CEC_PHYS_ADDR_INVALID;
> +	n->edid_present = false;
> +	if (n->cec_adap && !n->cec_adap->adap_controls_phys_addr)
> +		cec_phys_addr_invalidate(n->cec_adap);
> +	mutex_unlock(&n->lock);
> +}
> +EXPORT_SYMBOL_GPL(cec_notifier_phys_addr_invalidate);

[Severity: Medium]
Does calling cec_phys_addr_invalidate() instead of cec_s_phys_addr()
actually allow the CEC framework to distinguish whether HPD or an EDID is
present?

In drivers/media/cec/core/cec-notifier.c, cec_notifier_cec_adap_register()
and cec_notifier_phys_addr_invalidate() use n->edid_present to choose
between calling cec_phys_addr_invalidate(adap) and
cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false).

However, in include/media/cec.h, cec_phys_addr_invalidate() is defined as:

static inline void cec_phys_addr_invalidate(struct cec_adapter *adap)
{
	cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false);
}

Because cec_phys_addr_invalidate() simply calls
cec_s_phys_addr(adap, CEC_PHYS_ADDR_INVALID, false), both branches execute
the exact same call and n->edid_present has no observable effect on the
CEC core. Is a separate change to cec_phys_addr_invalidate() missing?

> diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
> index b1c8397341249..15e9bcb5b8879 100644
> --- a/include/media/cec-notifier.h
> +++ b/include/media/cec-notifier.h
> @@ -80,7 +80,10 @@ void cec_notifier_cec_adap_unregister(struct cec_notifier *n,
>   * @n: the CEC notifier
>   * @pa: the CEC physical address
>   *
> - * Set a new CEC physical address.
> + * Set a new CEC physical address. @pa may be f.f.f.f, indicating that
> + * the EDID does not contain a valid Physical Address.
> + * Use @cec_notifier_phys_addr_invalidate() when there is no EDID at all,
> + * i.e. there is no video sink connected.
>   * Does nothing if @n == NULL.
>   */
>  void cec_notifier_set_phys_addr(struct cec_notifier *n, u16 pa);

[Severity: Low]
Should @cec_notifier_phys_addr_invalidate() in the kernel-doc comment for
cec_notifier_set_phys_addr() in include/media/cec-notifier.h be written as
cec_notifier_phys_addr_invalidate() without the leading @?

When building documentation via Documentation/driver-api/media/cec-core.rst,
the @ prefix causes kernel-doc (RestFormat in
tools/lib/python/kdoc/kdoc_output.py) to match the reference as a
function-pointer parameter (type_fp_param) and escape the parentheses,
which prevents Sphinx from generating a function cross-reference link.

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