Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Missing NULL check for `device_get_match_data()` leads to a NULL pointer dereference on manual sysfs bind. - [High] Overlapping devicetree reg property and hardcoded offset prevents successful DT probe. Pre-existing issues: - [High] Potential NULL pointer dereference when accessing `dev->platform_data` prior to validation. -- commit 6b41d18c7c7f31b8549b66b4262af72e2486491d Author: AngeloGioacchino Del Regno <[email protected]> phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata In preparation for adding support for newer SoCs and for adding more capabilities to this driver in an efficient manner, migrate all of the hardcoded register offsets to SoC specific pdata and assign that for both DT and platform probing. > diff --git a/drivers/phy/mediatek/phy-mtk-dp.c > b/drivers/phy/mediatek/phy-mtk-dp.c > index d241004e28286..20974650d9409 100644 > --- a/drivers/phy/mediatek/phy-mtk-dp.c > +++ b/drivers/phy/mediatek/phy-mtk-dp.c [ ... ] > @@ -170,12 +241,19 @@ static void mtk_dp_phy_legacy_remove_lookup(void *data) > phy_remove_lookup(phy, "dp", dev_name(dp_phy->dev)); > } > > +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; > int ret; > > + /* > + * 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; > if (!dp_phy->regmap) > return dev_err_probe(dev, -EINVAL, "No platform data > available\n"); [Severity: High] This isn't a bug introduced by this patch, but can this dereference dev->platform_data before checking if it is NULL? If the driver is manually bound via sysfs or a module without platform data provided, it looks like it would crash immediately here: drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_legacy_probe() { ... dp_phy->regmap = *(struct regmap **)dev->platform_data; if (!dp_phy->regmap) return dev_err_probe(dev, -EINVAL, "No platform data available\n"); ... } [ ... ] > @@ -235,6 +313,8 @@ static int mtk_dp_phy_probe(struct platform_device *pdev) > if (ret) > return ret; > > + dp_phy->pdata = device_get_match_data(dev); > + > phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops); > if (IS_ERR(phy)) > return dev_err_probe(dev, PTR_ERR(phy), [Severity: High] Could this result in a NULL pointer dereference? If the driver is manually bound (e.g., via driver_override in sysfs) to a device that possesses an of_node but does not match any entry in mtk_dp_phy_of_match, device_get_match_data() will return NULL. This NULL pointer would later be dereferenced when accessing dp_phy->pdata in mtk_dp_phy_init(). [ ... ] > @@ -249,8 +329,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 }, > + .regs_dig_glb = mt8195_phy_dig_glb_regs, > + .regs_dig_lane = mt8195_phy_dig_lane_regs, > +}; > + [Severity: High] Will these hardcoded offsets prevent a successful devicetree probe? The driver maps its devicetree reg resource using devm_platform_ioremap_resource() in mtk_dp_phy_probe(): drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() { ... base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(base)) return PTR_ERR(base); ... } If the DT reg maps the specific PHY region, the driver will still add 0x1000 to the mapped base, accessing unmapped physical memory. If the DT reg maps the entire DP controller region to make the offsets work, devm_platform_ioremap_resource() will fail with -EBUSY because the parent DP driver already holds the request_mem_region lock. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
