Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] Missing NULL pointer check for `device_get_match_data()` return value leads to kernel panic. - [Low] Build failure due to invalid initialization of an array field with a compound literal. Pre-existing issues: - [High] Missing driver `.remove` callback leads to unbalanced runtime PM usage counter and resource leak. - [High] Dangling `phy_lookup` entry in the global list causes a Use-After-Free upon driver unbind. - [High] Unconditional dereference of `dev->platform_data` before validating it for NULL. -- commit 2a851e9d237369ac4340bdce02183defb68c9865 Author: AngeloGioacchino Del Regno <[email protected]> phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata This patch migrates the hardcoded register offsets in the MediaTek DP PHY driver to SoC-specific platform data. It assigns this platform data for both DeviceTree and platform probing. It also loops through lanes to clean up the writing of driving parameters. > diff --git a/drivers/phy/mediatek/phy-mtk-dp.c > b/drivers/phy/mediatek/phy-mtk-dp.c > index 98e05fe05ce3b..ce33f6812bae6 100644 > --- a/drivers/phy/mediatek/phy-mtk-dp.c > +++ b/drivers/phy/mediatek/phy-mtk-dp.c [ ... ] > @@ -161,11 +225,18 @@ static const struct phy_ops mtk_dp_phy_dev_ops = { > .owner = THIS_MODULE, > }; > > +static const struct mtk_dp_phy_pdata mt8195_dp_phy_data; > + > static int mtk_dp_phy_legacy_probe(struct platform_device *pdev, struct > mtk_dp_phy *dp_phy) > { > struct device *dev = &pdev->dev; > struct phy *phy; > > + /* > + * If legacy platform driver probe, assume this is MT8195 or compatible > + * with a devicetree that was not migrated to the new, proper bindings. > + */ > + dp_phy->pdata = &mt8195_dp_phy_data; > dp_phy->regmap = *(struct regmap **)dev->platform_data; [Severity: High] This is a pre-existing issue, but does this code unconditionally dereference dev->platform_data before checking if it is NULL? If the platform device is probed through the legacy path without an of_node and without platform_data provided (which can happen by manually binding an arbitrary compatible device via sysfs), could this lead to a kernel panic? > if (!dp_phy->regmap) > return dev_err_probe(dev, -EINVAL, "No platform data > available\n"); [ ... ] > @@ -214,6 +285,8 @@ static int mtk_dp_phy_probe(struct platform_device *pdev) > if (IS_ERR(dp_phy->regmap)) > return PTR_ERR(dp_phy->regmap); > > + dp_phy->pdata = device_get_match_data(dev); > + [Severity: High] Can device_get_match_data() return NULL here? If a user with privileges to write to sysfs binds the device manually using /sys/bus/platform/drivers/.../bind, or the device is instantiated via platform name matching while having an of_node, device_get_match_data() will return NULL. Could this lead to a NULL pointer dereference resulting in a kernel panic when dp_phy->pdata is accessed later? > phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops); > if (IS_ERR(phy)) > return dev_err_probe(dev, PTR_ERR(phy), [ ... ] > @@ -231,8 +304,15 @@ static int mtk_dp_phy_probe(struct platform_device *pdev) > return 0; > } > > +static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = { > + .off_dig_glb = 0x1000, > + .off_dig_lane = (const u16[]) { 0x1100, 0x1200, 0x1300, 0x1400 }, [Severity: Low] Will this fail to compile under standard C compilers? The off_dig_lane field is declared as an array u16 off_dig_lane[MTK_DP_PHY_MAX_LANES], but it is being initialized using a pointer from a compound literal cast (const u16[]) { ... }. Assigning an explicitly cast compound literal to an array field is syntactically invalid and could prevent the driver from building. > + .regs_dig_glb = mt8195_phy_dig_glb_regs, > + .regs_dig_lane = mt8195_phy_dig_lane_regs, > +}; [Severity: High] This is a pre-existing issue, but does the mtk_dp_phy_driver missing a .remove callback lead to unbalanced runtime PM usage? drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() { ... pm_runtime_enable(dev); pm_runtime_get_sync(dev); return 0; } drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_driver { .probe = mtk_dp_phy_probe, .driver = { .name = "mediatek-dp-phy", .of_match_table = mtk_dp_phy_of_match, }, }; Without a .remove implementation to handle unbinding the driver, could the device remain permanently powered on and fail to cleanly suspend or unbind? [Severity: High] This is also a pre-existing issue, but is the phy_lookup entry left dangling in the global list when the driver is unbound? drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_legacy_probe() { ... phy_create_lookup(phy, "dp", dev_name(dev)); ... } Because there is no .remove callback, phy_remove_lookup() is never called. Could a subsequent phy_get() call traversing the list access the freed pointer, resulting in a Use-After-Free since devres automatically frees the struct phy? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
