Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The patch omits `sm8750.dtsi`, failing to add the `INT2_GDSC` power domain for SM8750 despite the commit message explicitly declaring that SM8750 requires it. - [High] Missing cleanup of the IRQ domain and chained handler when power domain attachment fails, leading to a Use-After-Free. --
--- Patch [3]: [PATCH 3/3] arm64: dts: qcom: Add INT2_GDSC to MDSS on Kaanapali and Glymur --- commit a757d0e4c829d9c4eb6cdd0c681fb7e3459e226e Author: Yongxing Mou <[email protected]> arm64: dts: qcom: Add INT2_GDSC to MDSS on Kaanapali and Glymur Kaanapali (SM8750) and Glymur use two display power domains. CORE_GDSC powers the main display hardware while INT2_GDSC powers a subset of SSPP blocks (VIG2/VIG3/DMA5/DMA6). Add DISP_CC_MDSS_CORE_INT2_GDSC alongside the existing CORE_GDSC reference in the MDSS node on both platforms and provide matching power-domain-names entries for the display power domains. > Kaanapali (SM8750) and Glymur use two display power domains. > CORE_GDSC powers the main display hardware while INT2_GDSC powers > a subset of SSPP blocks (VIG2/VIG3/DMA5/DMA6). [Severity: High] The commit message explicitly states that Kaanapali (SM8750) requires the INT2_GDSC power domain. However, the patch omits updating sm8750.dtsi. Could this omission cause SM8750 platforms using sm8750.dtsi to fall back to single-domain mode and fail to attach the secondary power domain, leaving a subset of SSPP blocks (VIG2/VIG3/DMA5/DMA6) unpowered and inaccessible? diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c index 9087c4b290dba..63ece55c4f2ec 100644 --- a/drivers/gpu/drm/msm/msm_mdss.c +++ b/drivers/gpu/drm/msm/msm_mdss.c [ ... ] @@ -421,6 +451,27 @@ static struct msm_mdss *msm_mdss_init(struct platform_device *pdev, bool is_mdp5 irq_set_chained_handler_and_data(irq, msm_mdss_irq, msm_mdss); + /* + * When two power domains are listed in DT, the core framework does not + * auto-attach either of them. Attach and manage both explicitly so + * that INT2_GDSC (needed for VIG2/VIG3) is enabled alongside the main + * CORE_GDSC. Single-domain platforms are unaffected. + */ + if (of_count_phandle_with_args(pdev->dev.of_node, "power-domains", + "#power-domain-cells") > 1) { + static const char * const mdss_pd_names[] = { "core", "int2" }; + static const struct dev_pm_domain_attach_data pd_data = { + .pd_names = mdss_pd_names, + .num_pd_names = ARRAY_SIZE(mdss_pd_names), + .pd_flags = PD_FLAG_NO_DEV_LINK, + }; + + ret = devm_pm_domain_attach_list(&pdev->dev, &pd_data, + &msm_mdss->pd_list); + if (ret < 0) + return ERR_PTR(ret); [Severity: High] Is it possible this error path leaves a dangling pointer registered in the IRQ subsystem? In msm_mdss_init(), irq_set_chained_handler_and_data() is called earlier to install the chained handler with msm_mdss as the data pointer, and the IRQ domain is created via irq_domain_create_linear(). If devm_pm_domain_attach_list() fails (e.g., due to probe deferral), this block directly returns an error without calling irq_set_chained_handler_and_data() with NULL to clean it up, or calling irq_domain_remove(). Because msm_mdss is devres-allocated, it will be freed upon probe failure. If a spurious or shared interrupt fires on that IRQ line, will the kernel execute msm_mdss_irq() and dereference the freed msm_mdss pointer, causing a use-after-free and a kernel panic? + } + pm_runtime_enable(&pdev->dev); return msm_mdss; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
