Re: [PATCH] media: cec: meson: ao-cec: enable the controller clock at probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: cec: meson: ao-cec: hardware reset and clock enable run after registration in meson_ao_cec_probe()
--

commit 217c22ea59bd67681b3e3f366c510df7ceb9177a
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

media: cec: meson: ao-cec: enable the controller clock at probe

This commit updates meson_ao_cec_probe() to enable the CEC controller's
clock in CEC_GEN_CNTL_REG while pulsing the reset bit and to leave the clock
enabled after reset. This is intended to prevent AO bus stalls on Amlogic S4
when .adap_log_addr() accesses the controller before .adap_enable() has been
called.

> diff --git a/drivers/media/cec/platform/meson/ao-cec.c b/drivers/media/cec/platform/meson/ao-cec.c
> index 2df0d37430779..dd4d91d7b927c 100644
> --- a/drivers/media/cec/platform/meson/ao-cec.c
> +++ b/drivers/media/cec/platform/meson/ao-cec.c
> @@ -674,8 +674,25 @@ static int meson_ao_cec_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto out_probe_notify;
>  
> -	/* Setup Hardware */
> -	writel_relaxed(CEC_GEN_CNTL_RESET,
> +	/*
> +	 * Setup Hardware.
> +	 *
> +	 * Leave the controller with its gated clock running. The CEC core
> +	 * calls .adap_log_addr() before it ever calls .adap_enable(), so the
> +	 * first register access happens while the block would otherwise still
> +	 * be in power-off mode (CEC_GEN_CNTL clk field == 0). On Amlogic S4
> +	 * that access never completes: it stalls the AO bus and hard-hangs the
> +	 * SoC - every CPU stops, IRQs included - which happens on every HDMI
> +	 * hot-unplug, because that is what invalidates the CEC physical
> +	 * address.
> +	 */
> +	writel_relaxed(CEC_GEN_CNTL_RESET |
> +		       FIELD_PREP(CEC_GEN_CNTL_CLK_CTRL_MASK,
> +				  CEC_GEN_CNTL_CLK_ENABLE),
> +		       ao_cec->base + CEC_GEN_CNTL_REG);
> +	fsleep(100);
> +	writel_relaxed(FIELD_PREP(CEC_GEN_CNTL_CLK_CTRL_MASK,
> +				  CEC_GEN_CNTL_CLK_ENABLE),
>  		       ao_cec->base + CEC_GEN_CNTL_REG);

[Severity: High]
Should this hardware reset and clock enable sequence be moved to run before
cec_notifier_cec_adap_register() and cec_register_adapter() in
meson_ao_cec_probe()?

In drivers/media/cec/platform/meson/ao-cec.c:meson_ao_cec_probe(), the
adapter is published to the HDMI notifier and registered with userspace
before CEC_GEN_CNTL_REG is written:

	ao_cec->notify = cec_notifier_cec_adap_register(hdmi_dev, NULL,
							ao_cec->adap);
	if (!ao_cec->notify) {
		ret = -ENOMEM;
		goto out_probe_clk;
	}

	ret = cec_register_adapter(ao_cec->adap, &pdev->dev);
	if (ret < 0)
		goto out_probe_notify;

	/* Setup Hardware. */
	writel_relaxed(CEC_GEN_CNTL_RESET | ...);
	fsleep(100);
	writel_relaxed(...);

Inside drivers/media/cec/core/cec-notifier.c:cec_notifier_cec_adap_register(),
n->cec_adap is assigned and cec_s_phys_addr() sets the initial physical
address:

	mutex_lock(&n->lock);
	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);
	mutex_unlock(&n->lock);

If an HDMI hot-unplug or EDID update invokes cec_notifier_set_phys_addr()
after cec_notifier_cec_adap_register() releases n->lock but before
meson_ao_cec_probe() writes CEC_GEN_CNTL_REG, the call chain:

  cec_notifier_set_phys_addr()
    -> __cec_s_phys_addr()
      -> cec_adap_unconfigure()
        -> meson_ao_cec_set_log_addr()

will still access the controller while CEC_GEN_CNTL_REG has its clock field
at 0, triggering the AO bus stall on Amlogic S4.

Additionally, because /dev/cecX is already live after cec_register_adapter()
and the notifier is active while meson_ao_cec_probe() sleeps in fsleep(100)
with CEC_GEN_CNTL_RESET asserted, a concurrent call to
meson_ao_cec_adap_enable() or meson_ao_cec_set_log_addr() (for example from
udev or a CEC daemon opening the device and issuing CEC_ADAP_S_LOG_ADDRS)
can race with the unlocked reset and CEC_GEN_CNTL_REG writes in probe,
losing the controller configuration.

All prerequisites for the register writes (ao_cec->base, ao_cec->core clock
enable and rate setup, and device_reset_optional()) are already initialized
prior to cec_notifier_cec_adap_register().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192046.208710-1-christian.koever-draxl@student.uibk.ac.at?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic