Re: [PATCHv3 07/13] media: uapi: cec.h: add CEC_CAP_HAS_HPD_PRESENT/CEC_LOG_ADDRS_FL_HPD_PRESENT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: uapi: cec.h: unimplemented and undocumented HPD_PRESENT UAPI flags
- [Low] media: uapi: cec.h: mismatched EDID comment on CEC_LOG_ADDRS_FL_HPD_PRESENT
--

commit 2490741b0b49047176c24c5b5fcdf74889b9d347
Author: Hans Verkuil <hverkuil+cisco@kernel.org>

media: uapi: cec.h: add CEC_CAP_HAS_HPD_PRESENT/CEC_LOG_ADDRS_FL_HPD_PRESENT

This commit adds the CEC_CAP_HAS_HPD_PRESENT capability and the
CEC_LOG_ADDRS_FL_HPD_PRESENT logical address flag to include/uapi/linux/cec.h
to indicate whether HPD is present.

> diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
> index fdfc97a6e4ec0..b1b5f47485836 100644
> --- a/include/uapi/linux/cec.h
> +++ b/include/uapi/linux/cec.h
> @@ -325,27 +325,27 @@ static inline int cec_is_unconfigured(__u16 log_addr_mask)
>  #define CEC_MODE_FOLLOWER_MSK		0xf0
>  
>  /* Userspace has to configure the physical address */
> -#define CEC_CAP_PHYS_ADDR	(1 << 0)
> +#define CEC_CAP_PHYS_ADDR		(1 << 0)
>  /* Userspace has to configure the logical addresses */
> -#define CEC_CAP_LOG_ADDRS	(1 << 1)
> +#define CEC_CAP_LOG_ADDRS		(1 << 1)
>  /* Userspace can transmit messages (and thus become follower as well) */
> -#define CEC_CAP_TRANSMIT	(1 << 2)
> -/*
> - * Passthrough all messages instead of processing them.
> - */
> -#define CEC_CAP_PASSTHROUGH	(1 << 3)
> +#define CEC_CAP_TRANSMIT		(1 << 2)
> +/* Passthrough all messages instead of processing them. */
> +#define CEC_CAP_PASSTHROUGH		(1 << 3)
>  /* Supports remote control */
> -#define CEC_CAP_RC		(1 << 4)
> +#define CEC_CAP_RC			(1 << 4)
>  /* Hardware can monitor all messages, not just directed and broadcast. */
> -#define CEC_CAP_MONITOR_ALL	(1 << 5)
> +#define CEC_CAP_MONITOR_ALL		(1 << 5)
>  /* Hardware can use CEC only if the HDMI HPD pin is high. */
> -#define CEC_CAP_NEEDS_HPD	(1 << 6)
> +#define CEC_CAP_NEEDS_HPD		(1 << 6)
>  /* Hardware can monitor CEC pin transitions */
> -#define CEC_CAP_MONITOR_PIN	(1 << 7)
> +#define CEC_CAP_MONITOR_PIN		(1 << 7)
>  /* CEC_ADAP_G_CONNECTOR_INFO is available */
> -#define CEC_CAP_CONNECTOR_INFO	(1 << 8)
> +#define CEC_CAP_CONNECTOR_INFO		(1 << 8)
>  /* CEC_MSG_FL_REPLY_VENDOR_ID is available */
> -#define CEC_CAP_REPLY_VENDOR_ID	(1 << 9)
> +#define CEC_CAP_REPLY_VENDOR_ID		(1 << 9)
> +/* CEC_LOG_ADDRS_FL_HPD_PRESENT is available */
> +#define CEC_CAP_HAS_HPD_PRESENT	(1 << 10)
>  
>  /**
>   * struct cec_caps - CEC capabilities structure.
> @@ -405,6 +405,8 @@ struct cec_log_addrs {
>  #define CEC_LOG_ADDRS_FL_CDC_ONLY		(1 << 2)
>  /* Configuration failed */
>  #define CEC_LOG_ADDRS_FL_CONFIG_FAILED		(1 << 3)
> +/* EDID is present */
> +#define CEC_LOG_ADDRS_FL_HPD_PRESENT		(1 << 4)

[Severity: Medium]
Are CEC_CAP_HAS_HPD_PRESENT and CEC_LOG_ADDRS_FL_HPD_PRESENT missing their
kernel implementation and documentation?

Neither symbol is referenced anywhere in the kernel tree or documented in
Documentation/userspace-api/media/cec/cec-ioc-adap-g-caps.rst and
Documentation/userspace-api/media/cec/cec-ioc-adap-g-log-addrs.rst.

When userspace calls the CEC_ADAP_S_LOG_ADDRS ioctl, cec_adap_s_log_addrs()
strips CEC_LOG_ADDRS_FL_HPD_PRESENT before calling __cec_s_log_addrs():

drivers/media/cec/core/cec-api.c:cec_adap_s_log_addrs() {
	...
	log_addrs.flags &= CEC_LOG_ADDRS_FL_ALLOW_UNREG_FALLBACK |
			   CEC_LOG_ADDRS_FL_ALLOW_RC_PASSTHRU |
			   CEC_LOG_ADDRS_FL_CDC_ONLY;
	mutex_lock(&adap->lock);
	if (!adap->is_claiming_log_addrs && !adap->is_configuring &&
	    (!log_addrs.num_log_addrs || !adap->is_configured) &&
	    !cec_is_busy(adap, fh)) {
		err = __cec_s_log_addrs(adap, &log_addrs, block);
	...
}

Nothing in the CEC core or drivers ever sets or checks
CEC_CAP_HAS_HPD_PRESENT in adap->capabilities or CEC_LOG_ADDRS_FL_HPD_PRESENT
in adap->log_addrs.flags. Should these UAPI definitions be introduced
together with the kernel code and documentation that use them?

[Severity: Low]
Is the comment "EDID is present" above CEC_LOG_ADDRS_FL_HPD_PRESENT intended?
The macro name and commit message both describe indicating whether HPD is
present rather than EDID.

Also, this isn't a bug, but the patch re-indents all existing CEC_CAP_*
definitions with an extra tab to align their values, while the new
CEC_CAP_HAS_HPD_PRESENT uses a single tab and leaves `(1 << 10)` unaligned
with the rest of the block.

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