Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The DP PHY is published and exposed to concurrent consumers before its calibration data is initialized in the legacy probe path. - [Medium] Unconditional `dev_err` logs an error for missing optional NVMEM cells and probe deferrals in `mtk_dp_phy_get_one_cal_para`. Pre-existing issues: - [High] Dereferencing `dev->platform_data` without checking for NULL in `mtk_dp_phy_legacy_probe`. -- --- Patch [8]: [PATCH v2 08/12] phy: phy-mtk-dp: Add support for digital and analog calibration --- commit 0db0b57c522a5f7eb7d179aeee49708015901046 Author: AngeloGioacchino Del Regno <[email protected]> phy: phy-mtk-dp: Add support for digital and analog calibration This commit adds support for reading DP PHY calibration values from eFuse via nvmem. If the values are present, they are applied; otherwise, the driver relies on default calibration data. Analog calibration writing support is also introduced. > diff --git a/drivers/phy/mediatek/phy-mtk-dp.c > b/drivers/phy/mediatek/phy-mtk-dp.c > --- a/drivers/phy/mediatek/phy-mtk-dp.c > +++ b/drivers/phy/mediatek/phy-mtk-dp.c [ ... ] > +static int mtk_dp_phy_get_one_cal_para(struct device *dev, const char *name, > u8 max_val) > +{ > + u8 buf_byte; > + u16 buf; > + 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, &buf_byte); > + if (ret) > + ret = nvmem_cell_read_u16(dev, name, &buf); > + else > + buf = buf_byte; > + > + if (ret) { > + dev_err(dev, "Cannot get calibration data for %s: %d\n", name, > ret); > + return ret; > + }; [Severity: Medium] Will this unconditionally log a hard error if the NVMEM cells are absent or if the provider returns -EPROBE_DEFER? Since nvmem_cell_read_u8() returns -ENOENT when optional cells are missing in the device tree, could this cause spurious error messages in the kernel log on normal boots? It might also spam the logs during standard probe deferrals. [ ... ] > @@ -444,6 +671,8 @@ static int mtk_dp_phy_legacy_probe(struct platform_device > *pdev, struct mtk_dp_p > if (ret) > return ret; > > + mtk_dp_phy_get_default_cal_data(dp_phy); > + > return 0; > } [Severity: High] Is it safe to publish the PHY to consumers before the calibration defaults are initialized? Earlier in mtk_dp_phy_legacy_probe(), phy_create_lookup() exposes the PHY. If a consumer driver probes concurrently and acquires the PHY right after that lookup registration, could it call phy_init() while the analog parameter registers are still zero-initialized by devm_kzalloc(), rather than having these defaults? [Severity: High] This is a pre-existing issue, but can dev->platform_data be NULL earlier in mtk_dp_phy_legacy_probe()? Looking at the start of the function: dp_phy->regmap = *(struct regmap **)dev->platform_data; if (!dp_phy->regmap) return dev_err_probe(dev, -EINVAL, "No platform data available\n"); If the driver is bound to a device without platform data (e.g. manually via sysfs bind), it looks like dereferencing dev->platform_data to read the regmap will cause a kernel panic before it reaches the NULL check. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
