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

On 5/2/26 10:56 AM, [email protected] wrote:
> 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.

All of the reports here are valid, and all will be addressed
in the next version of the patch (as described further below).

> --
> 
> commit e138cc3787ee995063c093b9ca94f6ca453bbcda
> Author: Daniel Thompson <[email protected]>
> 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?

This is a legitimate issue.  However in the next version
of this patch, the call to tc956x_mac_configure() is no
longer necessary.  As as a result, the issue will no
longer occur, and in fact tc956x_mac_enable() becomes a
void function, simplifying callers as well.

> 
> [ ... ]
> 
>> +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().

Here, the translated variable (in all four functions) will be
converted to a u64, which avoids the problem even if the
size of a dma_addr_t is 32 bits.

> 
> [ ... ]
> 
>> +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?

Yes, this did leak the reference to the mdio_node.  The
next version of this patch addresses this in the caller,
dropping the reference on any error, and dropping it also
in the driver remove function.

> 
> [ ... ]
> 
>> +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]?

Yes, Ayaan mentioned this as well and it has been fixed.

> 
> 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?

Yes, and this will be addressed in the next version of this patch.

> 
> [ ... ]
> 
>> +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?

A new function tc956x_stmmac_resources_exit() will be created to
release the mappings created by tc956x_stmmac_resources_init().

					-Alex

> 
> 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().
>