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 <[email protected]> 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+ci...@kernel.org?part=1
