Re: [PATCH net-next 10/12] net: stmmac: tc956x: add TC956x/QPS615 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The RX queue IRQ mapping loop incorrectly checks `res->tx_irq[i]` instead of `res->rx_irq[i]` for failures.
- [High] DMA address truncation on 32-bit architectures leads to corrupted hardware DMA programming.
- [High] Mapped IRQs are never disposed, leaking descriptors and causing a WARN on device removal.
- [Medium] Hardware clocks and resets are leaked on `probe()` and `mac_enable()` error paths.
- [Medium] OF node reference leak for `mdio_node` on probe error paths and device removal.
--

commit e138cc3787ee995063c093b9ca94f6ca453bbcda
Author: Daniel Thompson <daniel@riscstar.com>
Subject: net: stmmac: tc956x: add TC956x/QPS615 support

This commit adds support for the Toshiba TC956x eMAC/MSIGEN combination,
implemented as a driver for an auxiliary bus device that bridges TC956x
level interrupts to PCIe MSIs.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tc956x.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tc956x.c
> new file mode 100644
> index 000000000000..27ff83dd3bd9
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tc956x.c

[ ... ]

> +static int tc956x_mac_enable(struct tc956x_data *td)
> +{
> +	const struct tc956x_chip *chip = td->auxbus_data->chip;
> +	struct plat_stmmacenet_data *plat = td->plat;
> +	u32 id = td->auxbus_data->mac_id;
> +	int ret;
> +
> +	tc956x_clock_enable(chip, id, MAC_CLOCK_TX);
> +	tc956x_clock_enable(chip, id, MAC_CLOCK_RX);
> +	tc956x_clock_enable(chip, id, MAC_CLOCK_ALL);
> +	if (id)
> +		tc956x_clock_enable(chip, id, MAC_CLOCK_RMII);
> +
> +	/* Set the speed related registers */
> +	ret = tc956x_mac_configure(td, plat->max_speed);
> +	if (ret)
> +		return ret;

If tc956x_mac_configure() fails, will this leak the clocks that were just
enabled since tc956x_clock_disable() is not called?

[ ... ]

> +static void tc956x_dma_init_rx_chan(struct stmmac_priv *priv,
> +				    void __iomem *ioaddr,
> +				    struct stmmac_dma_cfg *dma_cfg,
> +				    dma_addr_t phy, u32 chan)
> +{
> +	dma_addr_t translated = phy + TC956X_SLV00_SRC_ADDR;

If dma_addr_t is 32-bit on the target architecture, will adding
TC956X_SLV00_SRC_ADDR (0x1000000000ULL) to phy truncate the result and cause
the upper 32 bits to be 0? 

Should translated be explicitly typed as u64? The same pattern appears
in tc956x_dma_init_tx_chan(), tc956x_desc_set_addr(), and
tc956x_desc_set_sec_addr().

[ ... ]

> +static int tc956x_dwmac_parse_dt(struct tc956x_data *td)
> +{
> +	struct device_node *mdio_node;
> +	struct device *dev = td->dev;
> +	struct device_node *np;
> +
> +	np = dev_of_node(dev);
> +	if (!np)
> +		return dev_err_probe(dev, -EINVAL, "no devicetree node\n");
> +
> +	/* Find the MDIO bus */
> +	for_each_child_of_node(np, mdio_node) {
> +		if (of_device_is_compatible(mdio_node,
> +					    "snps,dwmac-mdio"))
> +			break;
> +	}
> +
> +	/* Pass the MDIO bus (if there is one) to the core driver */
> +	if (mdio_node) {
> +		td->plat->mdio_node = mdio_node;
> +		td->plat->mdio_bus_data->needs_reset = true;
> +	}
> +
> +	return 0;
> +}

Since the loop breaks early when a compatible node is found, does this leak
the reference to mdio_node? 

Is a call to of_node_put() needed in tc956x_dwmac_remove() and on probe
error paths?

[ ... ]

> +static int tc956x_stmmac_resources_init(struct tc956x_data *td,
> +					struct stmmac_resources *res)
> +{
> +	struct irq_domain *irq_domain = td->irq_domain;
> +	u32 i;
> +
> +	res->irq = irq_create_mapping(irq_domain, HWIRQ_EVENT);
> +	if (!res->irq)
> +		return -EINVAL;
> +
> +	for (i = 0; i < td->plat->tx_queues_to_use; i++) {
> +		res->tx_irq[i] = irq_create_mapping(irq_domain, HWIRQ_TX0 + i);
> +		if (!res->tx_irq[i])
> +			return -EINVAL;
> +	}
> +
> +	for (i = 0; i < td->plat->rx_queues_to_use; i++) {
> +		res->rx_irq[i] = irq_create_mapping(irq_domain, HWIRQ_RX0 + i);
> +		if (!res->tx_irq[i])
> +			return -EINVAL;
> +	}

Should this be checking res->rx_irq[i] instead of res->tx_irq[i]? 

If tx_irq[i] was previously set during the TX mapping loop, this check won't
catch RX mapping failures and uninitialized IRQs could be left in the
resource structure.

[ ... ]

> +static int tc956x_dwmac_probe(struct auxiliary_device *adev,
> +			      const struct auxiliary_device_id *id)
> +{

[ ... ]

> +	ret = tc956x_mac_enable(td);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable MAC\n");
> +
> +	ret = stmmac_dvr_probe(dev, td->plat, &res);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed stmmac probe\n");
> +
> +	return 0;
> +}

Does this error path need to call tc956x_mac_disable(td) to unwind the
hardware state if stmmac_dvr_probe() fails?

[ ... ]

> +static void tc956x_dwmac_remove(struct auxiliary_device *adev)
> +{
> +	struct device *dev = &adev->dev;
> +	struct net_device *ndev = dev_get_drvdata(dev);
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	struct tc956x_data *td = priv->plat->bsp_priv;
> +
> +	stmmac_dvr_remove(dev);
> +	tc956x_mac_disable(td);
> +}

Are the IRQs created by irq_create_mapping() leaking here?

There appear to be no calls to irq_dispose_mapping() on device removal or
probe error paths. While IRQ_DOMAIN_FLAG_DESTROY_GC frees the generic
chip data, the descriptors might leak and trigger a warning in
irq_domain_remove().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260501155421.3329862-1-elder@riscstar.com?part=10