Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Double unregister of the PHY platform device in the error path of `mtk_dp_register_phy()`. - [High] `phy_power_on()` is called before `phy_init()` during eDP probe on new devicetrees. --
--- Patch [7]: [PATCH v3 07/11] drm/mediatek: mtk_dp: Add support for PHY from devicetree --- commit 325a8fa9ebb5392753c0df0c9aa94be702f67e6e Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dp: Add support for PHY from devicetree This commit adds support for passing a displayport PHY handle from the devicetree. It introduces a check for the phys property to support newer configurations while maintaining legacy fallback behavior. > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > b/drivers/gpu/drm/mediatek/mtk_dp.c > index b4cb1996a3314..cabfc833c0a80 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c [ ... ] > @@ -2754,6 +2775,7 @@ static int mtk_dp_register_phy(struct mtk_dp *mtk_dp) > return dev_err_probe(dev, ret, > "Failed to add phy unregister devm > action"); > > + /* PHY calibration data is in mtk_dp only for legacy devicetree */ > mtk_dp_get_calibration_data(mtk_dp); > > mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp"); [Severity: High] Could this error path trigger a double-free of the PHY platform device? In mtk_dp_register_phy(), we register a devres action to unregister the platform device via mtk_dp_unregister_pdevs(). If devm_phy_get() fails and we return an error (like -EPROBE_DEFER), the explicit call to platform_device_unregister() in the error path will free the device. When the devres cleanup runs later, it will execute mtk_dp_unregister_pdevs() and call platform_device_unregister() a second time on the same pointer. [ ... ] > @@ -2857,9 +2884,18 @@ static int mtk_dp_probe(struct platform_device *pdev) > "Failed to register audio > driver\n"); > } > > - ret = mtk_dp_register_phy(mtk_dp); > - if (ret) > - return ret; > + if (!mtk_dp->legacy_regoff) { > + mtk_dp->phy = devm_phy_get(dev, NULL); > + if (IS_ERR(mtk_dp->phy)) > + return dev_err_probe(dev, PTR_ERR(mtk_dp->phy), > + "Failed to get phy\n"); > + > + mtk_dp->phy_dev = NULL; [Severity: High] Are we calling phy_power_on() before phy_init() for eDP displays on new devicetrees? During mtk_dp_probe() for eDP connectors, the driver calls mtk_dp_power_enable() to read the EDID before the bridge is attached. With the new PHY model, this executes phy_power_on(mtk_dp->phy): mtk_dp_probe() mtk_dp_power_enable() phy_power_on(mtk_dp->phy); However, phy_init() is not called until mtk_dp_bridge_attach() via mtk_dp_poweron(). Violating the PHY API contract by powering on before initialization can cause hardware initialization failures, as clocks or regulators managed by phy_init() are not yet enabled. > + } else { > + ret = mtk_dp_register_phy(mtk_dp); > + if (ret) > + return ret; > + } > > mtk_dp->bridge.of_node = dev->of_node; > mtk_dp->bridge.type = mtk_dp->data->bridge_type; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
