Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] media: cec: spurious state events in __cec_s_phys_addr() without cap - [Medium] media: cec: cec_s_phys_addr() falsely sets EDID_PRESENT without EDID --
commit 1fe777ffdbc4971735bf4636dac9ebebeba91665 Author: Hans Verkuil <[email protected]> media: cec: core: add support for EDID_PRESENT This commit adds support for reporting whether an EDID is present when the CEC physical address is set or invalidated. It updates cec_s_phys_addr() to set CEC_LOG_ADDRS_FL_EDID_PRESENT and changes cec_phys_addr_invalidate() to clear it when the EDID is lost. > diff --git a/drivers/media/cec/core/cec-adap.c > b/drivers/media/cec/core/cec-adap.c > index ee8a200b9803f..06a596a0ede3e 100644 > --- a/drivers/media/cec/core/cec-adap.c > +++ b/drivers/media/cec/core/cec-adap.c > @@ -1727,18 +1727,33 @@ 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; > + } > if (!becomes_invalid && adap->devnode.unregistered) > return; > + if (phys_addr == adap->phys_addr) { > + if (edid_was_present != edid_present) { > + dprintk(1, "EDID is %spresent\n", > + edid_present ? "" : "not "); > + cec_post_state_event(adap); > + } > + return; > + } [Severity: Medium] Can this queue spurious CEC_EVENT_STATE_CHANGE events when an adapter does not have CEC_CAP_HAS_EDID_PRESENT? In cec_allocate_adapter(), adapters allocated with CEC_CAP_PHYS_ADDR do not get CEC_CAP_HAS_EDID_PRESENT: drivers/media/cec/core/cec-core.c:cec_allocate_adapter() if (!(caps & CEC_CAP_PHYS_ADDR)) adap->capabilities |= CEC_CAP_HAS_EDID_PRESENT; Because __cec_s_phys_addr() only sets CEC_LOG_ADDRS_FL_EDID_PRESENT in adap->log_addrs.flags when CEC_CAP_HAS_EDID_PRESENT is set, edid_was_present remains permanently false for those adapters. However, cec_adap_s_phys_addr() (for the CEC_ADAP_S_PHYS_ADDR ioctl) and cec_s_phys_addr() still pass edid_present = true when setting a valid physical address. Whenever the same valid physical address is set again on such an adapter (phys_addr == adap->phys_addr), edid_was_present != edid_present evaluates to true and cec_post_state_event(adap) is called even though neither adap->phys_addr nor adap->log_addrs.flags changed. Should edid_present be forced to false when !(adap->capabilities & CEC_CAP_HAS_EDID_PRESENT), or should the edid_was_present != edid_present check be gated on CEC_CAP_HAS_EDID_PRESENT? > > - 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, EDID is %spresent\n", > + cec_phys_addr_exp(phys_addr), edid_present ? "" : "not "); > if (becomes_invalid || !is_invalid) { > adap->phys_addr = CEC_PHYS_ADDR_INVALID; > cec_adap_unconfigure(adap); > @@ -1767,11 +1782,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] Won't passing true unconditionally here cause existing callers that pass CEC_PHYS_ADDR_INVALID when no EDID is present to falsely report CEC_LOG_ADDRS_FL_EDID_PRESENT? For example, in drivers/media/cec/core/cec-notifier.c, struct cec_notifier only tracks n->phys_addr (which starts as CEC_PHYS_ADDR_INVALID when no sink is connected) and does not track whether an EDID is present. When a CEC adapter registers with a notifier while disconnected, cec_notifier_cec_adap_register() calls cec_s_phys_addr() with CEC_PHYS_ADDR_INVALID: 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); Similarly, cec_notifier_set_phys_addr_from_edid() calls cec_notifier_set_phys_addr(n, CEC_PHYS_ADDR_INVALID) -> cec_s_phys_addr() even when edid is NULL: drivers/media/cec/core/cec-notifier.c:cec_notifier_set_phys_addr_from_edid() if (edid && edid->extensions) pa = cec_get_edid_phys_addr((const u8 *)edid, EDID_LENGTH * (edid->extensions + 1), NULL); cec_notifier_set_phys_addr(n, pa); And drm_dp_cec_attach() in drivers/gpu/drm/display/drm_dp_cec.c also calls cec_s_phys_addr(aux->cec.adap, source_physical_address, false) when called from drm_dp_cec_register_connector() with no EDID and source_physical_address set to CEC_PHYS_ADDR_INVALID. In all of those paths, __cec_s_phys_addr() will now set CEC_LOG_ADDRS_FL_EDID_PRESENT and post a state change event even though no EDID is present. Do cec_notifier and drm_dp_cec need to be updated to track or invalidate EDID presence accordingly? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
