Re: [PATCH 2/2] phy: mediatek: Add support for PCI-Express Gen4 S-PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The 4-bit bitfields in `struct mtk_pcie_sphy_imp_sel` will silently truncate 5-bit TX impedance calibration values.
- [High] Probe fails when CONFIG_NVMEM is disabled due to unhandled -EOPNOTSUPP.
- [Medium] Unconditional read of lane 1 calibration data needlessly disables calibration entirely for 1-lane PHYs.
- [Medium] Valid calibration data containing the value '0' incorrectly aborts the entire calibration process.
- [Medium] PHY provider is registered before Runtime PM is enabled, creating a probe race condition.
- [Low] Expected missing calibration data and probe deferrals trigger unconditional dev_err() logs.
- [Low] Missing `#include <linux/pm_runtime.h>` while using `devm_pm_runtime_enable()`.
--

--- Patch [2]: [PATCH 2/2] phy: mediatek: Add support for PCI-Express Gen4 S-PHY ---
commit 7cbdba90e32bbdc889bb04306231fd5de6141303
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: mediatek: Add support for PCI-Express Gen4 S-PHY

Add support for the PCI-Express Gen4 S-PHY found in the latest
MediaTek SoCs, including MT8196, MT8894 and similar.

> diff --git a/drivers/phy/mediatek/phy-mtk-pcie-sphy.c b/drivers/phy/mediatek/phy-mtk-pcie-sphy.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-pcie-sphy.c
> @@ -0,0 +1,560 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Copyright (c) 2026 Collabora Ltd.
> + *                    AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>

[Severity: Low]
Does this need linux/pm_runtime.h included since devm_pm_runtime_enable()
is used later in the file?

[ ... ]

> +/**
> + * struct mtk_pcie_sphy_imp_sel - Impedance Selection parameters
> + * @pmos: Impedance selection for P-Channel MOSFET
> + * @nmos: Impedance selection for N-Channel MOSFET
> + */
> +struct mtk_pcie_sphy_imp_sel {
> +	u8 pmos : 4;
> +	u8 nmos : 4;
> +};

[Severity: High]
Can this truncate calibration values? The eFuse data read for tx-lanes-pmos
and tx-lanes-nmos uses FIELD_MAX(RG_XTP_LN_TX_IMPSEL_PMOS), which is
GENMASK(4, 0), allowing 5-bit values up to 31. Will values greater than 15
be silently truncated here?

[ ... ]

> +static int mtk_pcie_sphy_get_one_cal_para(struct device *dev, const char *name, u8 max_val)
> +{
> +	u16 buf;
> +	u8 tmp;
> +	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, &tmp);
> +	if (ret == 0)
> +		buf = tmp;
> +	else
> +		ret = nvmem_cell_read_u16(dev, name, &buf);
> +
> +	if (ret) {
> +		dev_err(dev, "Cannot get calibration data for %s: %d\n", name, ret);
> +		return ret;
> +	};

[Severity: Low]
Does this unconditionally log an error for expected missing cells (-ENOENT)
or during probe deferral (-EPROBE_DEFER)?

> +
> +	if (buf == 0) {
> +		dev_warn(dev, "No calibration for %s. Using defaults\n", name);
> +		return -ENOENT;
> +	}

[Severity: Medium]
Is 0 an invalid calibration value?

Later code checks if the calibration version is <= 4, which implies version
0 could exist. If a valid cell contains 0, will this mistakenly abort the
entire calibration process?

[ ... ]

> +static int mtk_pcie_sphy_get_calibration_data(struct mtk_pcie_sphy *pcie_sphy)
> +{

[ ... ]

> +	ret = mtk_pcie_sphy_get_one_cal_para(dev, "tx-ln0-rswn",
> +					     FIELD_MAX(RG_XTP_LN_TX_RSWN_IMPSEL));
> +	if (ret < 0)
> +		goto end;
> +	cal->tx_rswn_impsel[0] = ret;
> +
> +	ret = mtk_pcie_sphy_get_one_cal_para(dev, "tx-ln1-rswn",
> +					     FIELD_MAX(RG_XTP_LN_TX_RSWN_IMPSEL));
> +	if (ret < 0)
> +		goto end;
> +	cal->tx_rswn_impsel[1] = ret;

[Severity: Medium]
Will this break calibration on 1-lane PHYs?

If num_lanes is 1, the device tree might naturally omit lane 1 data. Would
the read for tx-ln1-rswn then return -ENOENT and cause all valid lane 0
calibration to be discarded?

[ ... ]

> +end:
> +	if (ret < 0) {
> +		/*
> +		 * If any of the calibration values is missing, or if there is
> +		 * no calibration at all in the eFuses, this is not a problem,
> +		 * as the PHY doesn't require one to actually work.
> +		 */
> +		if (ret == -ENOENT) {
> +			cal->supported = false;
> +			return 0;
> +		}
> +		return ret;
> +	};

[Severity: High]
If the kernel is compiled without CONFIG_NVMEM, nvmem_cell_read_u8 returns
-EOPNOTSUPP. Does this cause the entire PHY probe to fail on systems without
NVMEM, even though calibration is optional?

[ ... ]

> +static int mtk_pcie_sphy_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(provider))
> +		return dev_err_probe(dev, PTR_ERR(provider),
> +				     "Could not register PCI-Express S-PHY\n");
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[Severity: Medium]
Could registering the PHY provider before enabling Runtime PM lead to a race
condition?

If a consumer driver probes concurrently and immediately calls phy_init(),
could it try to interact with the device before devm_pm_runtime_enable() has
completed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716151333.77254-1-angelogioacchino.delregno@collabora.com?part=2