Re: [PATCH v2 13/24] PCI: dwc: Add ZTE ZX279128S host controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: dwc: zx279128s: -EPROBE_DEFER masked when requesting core reset
- [Medium] pci: dwc: zx279128s link training failure on concurrent probe
--

commit 31ef562bd53943175554ff3b4004b8759b588b24
Author: Navid Ghahremani <ghahramani.navid@gmail.com>
Subject: PCI: dwc: Add ZTE ZX279128S host controller driver

This commit adds support for the two Synopsys DesignWare PCIe host controllers
found on the ZTE ZX279128S SoC. It manages the clocks, resets, and PHYs using
standard kernel frameworks. The driver limits the link speed to Gen1 as done
by the vendor firmware.

> diff --git a/drivers/pci/controller/dwc/pcie-zx279128s.c b/drivers/pci/controller/dwc/pcie-zx279128s.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-zx279128s.c
[ ... ]
> +static int zx279128s_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct zx279128s_pcie *pcie = to_zx279128s_pcie(pci);
> +	int ret;
> +
> +	/*
> +	 * The board lines reset a device that can sit behind both controllers
> +	 * (the MT7915 of the H3600 uses both). The first controller to start
> +	 * resets and releases it; the second must not reset it again.
> +	 */
> +	mutex_lock(&zx279128s_board_lock);
> +	if (!zx279128s_board_released && (pcie->reset || pcie->enable)) {

[Severity: Medium]
If both PCIe controllers are probed concurrently (for example, via
asynchronous probing), could the second controller skip this reset sequence
and fail link training?

If the second controller requests the shared reset GPIO in
zx279128s_pcie_probe() and receives -EBUSY, its GPIO pointers will be set to
NULL. When it reaches this critical section before the first controller has
executed the reset toggle, it will evaluate pcie->reset || pcie->enable as
false.

This would cause it to skip the wait block, proceeding immediately to link
training while the endpoints are still in reset.

> +		ret = gpiod_direction_output(pcie->reset, 1);
> +		if (ret)
> +			goto err_unlock;
[ ... ]
> +static int zx279128s_pcie_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	pcie->clk = devm_clk_get_enabled(dev, "pcie");
> +	if (IS_ERR(pcie->clk))
> +		return dev_err_probe(dev, PTR_ERR(pcie->clk),
> +				     "failed to get pcie clock\n");
> +
> +	pcie->rst = devm_reset_control_get_exclusive(dev, "core");
> +	if (IS_ERR(pcie->rst))
> +		pcie->rst = devm_reset_control_get_exclusive(dev, "pcie");
> +	if (IS_ERR(pcie->rst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->rst),
> +				     "failed to get pcie reset\n");

[Severity: High]
Does this unconditional fallback logic mask probe deferrals?

If the reset controller is not yet ready, the initial request for the core
reset line returns -EPROBE_DEFER.

The fallback unconditionally requests the pcie reset line. If the device
tree only specifies core, the second request will return -ENOENT or -EINVAL.

This overwrites the original -EPROBE_DEFER, causing the controller to
permanently fail probing instead of deferring.

> +
> +	pcie->phy = devm_phy_get(dev, "pcie");
> +	if (IS_ERR(pcie->phy))
> +		return dev_err_probe(dev, PTR_ERR(pcie->phy),
> +				     "failed to get pcie phy\n");
> +
> +	/*
> +	 * Optional board lines to reset and enable endpoints. If already
> +	 * claimed by another controller, treat -EBUSY as already managed.
> +	 */
> +	pcie->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
> +	if (IS_ERR(pcie->reset)) {
> +		if (PTR_ERR(pcie->reset) == -EBUSY)
> +			pcie->reset = NULL;
> +		else
> +			return dev_err_probe(dev, PTR_ERR(pcie->reset),
> +					     "failed to get the reset GPIO\n");
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014072109.1759502-1-ghahramani.navid@gmail.com?part=13