Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The refactoring introduces incorrect integer values for two DisplayPort PHY driving parameters, silently altering the underlying hardware configuration. - [High] Integer underflow and undefined behavior in mtk_dp_phy_disable_all_lanes when no lanes are enabled. - [High] Flawed PM runtime initialization, race condition, and unhandled errors in probe. - [Medium] Incorrect endianness conversion by pointer aliasing a u16 as a u8*. --
commit 3ae397642076d659c70475bdbefbc3286e0e7c1f Author: AngeloGioacchino Del Regno <[email protected]> Subject: phy: phy-mtk-dp: Rewrite and document default driving param macros This patch uses FIELD_PREP_CONST and adds definitions and macros to build the default driving parameters for the PHY. It also adds comments explaining what they are supposed to set in the PHY. > diff --git a/drivers/phy/mediatek/phy-mtk-dp.c > b/drivers/phy/mediatek/phy-mtk-dp.c > index d7024a1443358..1d73a622ef322 100644 > --- a/drivers/phy/mediatek/phy-mtk-dp.c > +++ b/drivers/phy/mediatek/phy-mtk-dp.c [ ... ] > +/* MT8195: Logic State Change Point (LC TX C) */ > +#define MT8195_DRIVING_PARAM_3_DEFAULT BUILD_DRIVING_PARAM_0( 16, 20, > 24, 32) > +#define MT8195_DRIVING_PARAM_4_DEFAULT BUILD_DRIVING_PARAM_12(24, 30, > 60, 32) [Severity: High] Does this unintentionally change the default for SW1_PRE2? The conversion to macros incorrectly alters the sw1_pre2 value. The original macro XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT was (BIT(18) | BIT(21)), which evaluates to 36 (0x24) when shifted. This macro now sets it to 60 (0x3c). > +#define MT8195_DRIVING_PARAM_5_DEFAULT BUILD_DRIVING_PARAM_23(8, 48) [Severity: High] Is there a similar issue with the default for SW2_PRE1? The original value was 40 (0x28), but this is now passing 8 (0x08), which drops BIT(5). These altered driving parameters can degrade signal integrity and cause connection instability. [ ... ] > +static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy) > +{ > + const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata; > + const u8 *regs = pdata->regs_dig_glb; > + int ret; > + u32 val; > + > + ret = regmap_read(dp_phy->regmap, pdata->off_dig_glb + > regs[DP_PHYD_TX_CTL_0], &val); > + if (ret) > + return ret; > + > + /* Get mask of currently enabled lane */ > + val = FIELD_GET(PHYD_TX_LN_EN, val); > + > + /* Disable all lanes (needs to be done one by one, from last to first) > */ > + do { > + u32 lane_num = fls(val) - 1; > + val &= ~BIT(lane_num); > + > + ret = regmap_clear_bits(dp_phy->regmap, > + pdata->off_dig_glb + > regs[DP_PHYD_TX_CTL_0], > + FIELD_PREP(PHYD_TX_LN_EN, lane_num)); > + if (ret) > + return ret; > + } while (val); [Severity: High] Can this do-while loop execute with val initially at 0? If the PHY is already fully disabled when this is called, val will be 0. The loop blindly executes once, fls(0) returns 0, and lane_num underflows to 0xffffffff. Using ~BIT(0xffffffff) invokes undefined behavior, and passing 0xffffffff to FIELD_PREP() extracts unpredictable bits, causing a spurious out-of-bounds register write when clearing the PHY control registers. > + > + return 0; > +} [ ... ] > +static int mtk_dp_phy_get_one_cal_para(struct device *dev, const char *name, > u8 max_val) > +{ > + u16 buf = 0; > + int ret; > + > + /* > + * All of the calibrations are always max 8 bits long, but some may > + * be split between two different 8-bits cells: handle this corner > + * case by retrying reading as u16. > + */ > + ret = nvmem_cell_read_u8(dev, name, (u8 *)&buf); [Severity: Medium] Will this lead to an endianness violation on Big Endian platforms? Casting a u16 buffer pointer to (u8 *) will cause the single byte to be written into the most significant byte of buf instead of the least significant byte on Big Endian architectures. For instance, reading 0x0F would result in buf holding 0x0F00 (3840), which would falsely trigger the subsequent if (buf > max_val) validation check. > + if (ret) > + ret = nvmem_cell_read_u16(dev, name, &buf); [ ... ] > +static int mtk_dp_phy_probe(struct platform_device *pdev) > +{ [ ... ] > + phy_set_drvdata(phy, dp_phy); > + > + provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > + if (IS_ERR(provider)) > + return PTR_ERR(provider); [Severity: High] Could registering the provider before enabling PM runtime cause a race condition? Exposing the PHY to consumers here means a consumer could claim the PHY and attempt to power it on before pm_runtime_enable() is called below. This would return -EACCES and break the consumer initialization. > + > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); [Severity: High] Should the return value of pm_runtime_get_sync(dev) be checked? Ignoring the return value means the probe silently succeeds even if the hardware cannot be powered. Additionally, this driver lacks a .remove callback. Since pm_runtime_get_sync() increases the usage counter without it ever being released, and pm_runtime_disable() is never called, unloading the module permanently leaks the PM reference and breaks PM runtime for this device. > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
