Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [High] PHY driving parameters are bulk written to the wrong starting register offset, corrupting hardware state. - [High] Missing NULL check on the result of `device_get_match_data()` leads to a NULL pointer dereference. - [Low] The array `off_dig_lane` in `mt8195_dp_phy_data` is initialized using a compound literal, causing a compilation error. - [Low] The variable `ret` is used without being declared in `mtk_dp_phy_probe`, causing a build failure. - [Low] Missing `#include <linux/pm_runtime.h>` for `devm_pm_runtime_enable`. Pre-existing issues: - [High] Dereferencing `dev->platform_data` before verifying it is not `NULL` causes a kernel panic. -- --- Patch [4]: [PATCH v2 04/12] phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata --- commit b50fb23a21796e73379a3d3b300932c7ffb998c8 Author: AngeloGioacchino Del Regno <[email protected]> Subject: phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata This commit migrates the hardcoded register offsets in the MediaTek DP PHY driver to SoC-specific platform data. It also refactors the driving parameter initialization to loop through all lanes instead of hardcoding each lane. > diff --git a/drivers/phy/mediatek/phy-mtk-dp.c > b/drivers/phy/mediatek/phy-mtk-dp.c > index b042ab67ebaf3..915f148325c49 100644 > --- a/drivers/phy/mediatek/phy-mtk-dp.c > +++ b/drivers/phy/mediatek/phy-mtk-dp.c [ ... ] > @@ -94,15 +143,21 @@ static int mtk_dp_phy_init(struct phy *phy) > DRIVING_PARAM_7_DEFAULT, > DRIVING_PARAM_8_DEFAULT > }; > - > - regmap_bulk_write(dp_phy->regmap, MTK_DP_LANE0_DRIVING_PARAM_3, > - driving_params, ARRAY_SIZE(driving_params)); [ ... ] > + int i, ret; > + > + /* > + * Assume that all lanes need the same driving parameters: this > + * will bulk write from DRIVING_PARAM_0 to DRIVING_PARAM_8 on > + * all lanes (a grand total of [9 * num_lanes] 32-bit writes) > + */ > + for (i = 0; i < MTK_DP_PHY_MAX_LANES; i++) { > + ret = regmap_bulk_write(dp_phy->regmap, > + pdata->off_dig_lane[i] + reg, > + driving_params, > + ARRAY_SIZE(driving_params)); [Severity: High] Does this bulk write start at the correct register offset? The loop writes the driving_params array starting at the base offset for DP_PHYD_LAN_DRIVING_PARAM_0. However, the driving_params array only contains 6 elements (defaults for PARAM_3 through PARAM_8). Will this incorrectly write parameters 3-8 into the registers for 0-5, leaving registers 6-8 uninitialized and corrupting the hardware PHY configuration? > + if (ret) > + return ret; > + }; > > return 0; > } [ ... ] > @@ -169,12 +233,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; [Severity: High] This is a pre-existing issue, but can dev->platform_data be NULL here? The code directly dereferences dev->platform_data to assign dp_phy->regmap before checking if it is NULL on the next line. If a device is probed without devicetree and platform data is missing, will this trigger a kernel panic? > if (!dp_phy->regmap) > return dev_err_probe(dev, -EINVAL, "No platform data > available\n"); [ ... ] > @@ -227,6 +298,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] Could device_get_match_data() return NULL here? If the driver is manually bound via sysfs without a matching OF compatible (but the device has an of_node), device_get_match_data() will return NULL. Since dp_phy->pdata is later unconditionally dereferenced in functions like mtk_dp_phy_init() to look up register offsets, will this lead to a NULL pointer dereference? > + > phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops); [ ... ] [Severity: Low] Is the variable ret declared in the scope of mtk_dp_phy_probe? It was noted that ret might be assigned the result of devm_pm_runtime_enable() and evaluated: ret = devm_pm_runtime_enable(dev); if (ret) return ret; But it appears it might not be declared within the local scope, which could cause a compilation error. [Severity: Low] Should <linux/pm_runtime.h> be included in this file? If devm_pm_runtime_enable() is used, it relies on the Power Management API. Relying on implicit transitive inclusion for Power Management APIs can be fragile and may cause a build failure. > @@ -245,8 +318,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 array initialization compile correctly? In standard C, an array member inside a struct should be initialized using a brace-enclosed list. Using a compound literal cast like (const u16[]) decays to a pointer, which may cause a compilation error for an array type initializer. > + .regs_dig_glb = mt8195_phy_dig_glb_regs, > + .regs_dig_lane = mt8195_phy_dig_lane_regs, > +}; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
