On 05.02.2022 14:53, Greg Kroah-Hartman wrote:
On Sat, Feb 05, 2022 at 02:40:37PM +0300, Alexey Khoroshilov wrote:
On 04.02.2022 12:20, Greg Kroah-Hartman wrote:
From: Maxime Ripard maxime@cerno.tech
Commit 20b0dfa86bef0e80b41b0e5ac38b92f23b6f27f9 upstream.
The original commit depended on a rework commit (724fc856c09e ("drm/vc4: hdmi: Split the CEC disable / enable functions in two")) that (rightfully) didn't reach stable.
However, probably because the context changed, when the patch was applied to stable the pm_runtime_put called got moved to the end of the vc4_hdmi_cec_adap_enable function (that would have become vc4_hdmi_cec_disable with the rework) to vc4_hdmi_cec_init.
This means that at probe time, we now drop our reference to the clocks and power domains and thus end up with a CPU hang when the CPU tries to access registers.
The call to pm_runtime_resume_and_get() is also problematic since the .adap_enable CEC hook is called both to enable and to disable the controller. That means that we'll now call pm_runtime_resume_and_get() at disable time as well, messing with the reference counting.
The behaviour we should have though would be to have pm_runtime_resume_and_get() called when the CEC controller is enabled, and pm_runtime_put when it's disabled.
We need to move things around a bit to behave that way, but it aligns stable with upstream.
Cc: stable@vger.kernel.org # 5.10.x Cc: stable@vger.kernel.org # 5.15.x Cc: stable@vger.kernel.org # 5.16.x Reported-by: Michael Stapelberg michael+drm@stapelberg.ch Signed-off-by: Maxime Ripard maxime@cerno.tech Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/gpu/drm/vc4/vc4_hdmi.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1402,18 +1402,18 @@ static int vc4_hdmi_cec_adap_enable(stru u32 val; int ret;
- ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
- if (ret)
return ret;- val = HDMI_READ(HDMI_CEC_CNTRL_5);
- val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |
VC4_HDMI_CEC_CNT_TO_4700_US_MASK |VC4_HDMI_CEC_CNT_TO_4500_US_MASK);- val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |
((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);- if (enable) {
ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);if (ret)return ret;val = HDMI_READ(HDMI_CEC_CNTRL_5);val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |VC4_HDMI_CEC_CNT_TO_4700_US_MASK |VC4_HDMI_CEC_CNT_TO_4500_US_MASK);val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);- HDMI_WRITE(HDMI_CEC_CNTRL_5, val | VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET); HDMI_WRITE(HDMI_CEC_CNTRL_5, val);
@@ -1439,7 +1439,10 @@ static int vc4_hdmi_cec_adap_enable(stru HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC); HDMI_WRITE(
, val |
VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
pm_runtime_put(&vc4_hdmi->pdev->dev);- return 0;
} @@ -1531,8 +1534,6 @@ static int vc4_hdmi_cec_init(struct vc4_ if (ret < 0) goto err_delete_cec_adap;
- pm_runtime_put(&vc4_hdmi->pdev->dev);
- return 0;
err_delete_cec_adap:
The patch has moved initialization of val local variable into if (enable) branch. But the variable is used in in the else branch as well. As a result we write of its initialized value here:
HDMI_WRITE(HDMI_CEC_CNTRL_5, val | VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);Found by Linux Verification Center (linuxtesting.org) with SVACE.
static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable) { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); /* clock period in microseconds */ const u32 usecs = 1000000 / CEC_CLOCK_FREQ; u32 val; int ret;
if (enable) { ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev); if (ret) return ret;
val = HDMI_READ(HDMI_CEC_CNTRL_5); .....} else { HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC); HDMI_WRITE(HDMI_CEC_CNTRL_5, val | <------------------ UNINIT VALUE VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
pm_runtime_put(&vc4_hdmi->pdev->dev);}
return 0; }
So what does this mean? That this backport is incorrect and should be dropped? Or that the original commit was wrong? Or something else?
confused,
greg k-h
As far as I can see, the upstream commit was correct and the problem is in the backport. I would suggest to fix it like this:
} else { HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC); - HDMI_WRITE(HDMI_CEC_CNTRL_5, val + HDMI_WRITE(HDMI_CEC_CNTRL_5, HDMI_READ(HDMI_CEC_CNTRL_5) VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
pm_runtime_put(&vc4_hdmi->pdev->dev); }
But I would left it to comment someone who knows this code better.
-- Best regards, Alexey Khoroshilov Linux Verification Center, ISPRAS