Re: [PATCH net-next 1/2] net: sgi: ioc3-eth: apply the DMA attributes only to 64-bit addresses

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] ioc3_init() picks the device-wide RX barrier upper-address
  bits from the RX descriptor ring's coherent address:…
- [Medium] For 32-bit direct-window addresses the patch drops every per-
  transaction Bridge attribute the driver used to request: - RX buffers…
- [Low] In this tree, the new 32-bit-window branches in ioc3_map() and
  ioc3_erbar() (drivers/net/ethernet/sgi/ioc3-eth.c:162-163 and 171-172,…

--

net: sgi: ioc3-eth: apply the DMA attributes only to 64-bit addresses

This patch changes ioc3_map() so that addresses with bits 63:60 clear
(the Bridge 32-bit direct window) are returned unchanged, without the
DMA attributes OR-ed in. It also replaces the ERBAR_VAL constant with
ioc3_erbar(), which sets the RX barrier upper address bits only when the
RX ring address is a 64-bit direct-mapped address.

> diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
> index 009f37105eafd..25139cf5ff62c 100644
> --- a/drivers/net/ethernet/sgi/ioc3-eth.c
> +++ b/drivers/net/ethernet/sgi/ioc3-eth.c
> @@ -152,19 +152,37 @@ static inline int ioc3_alloc_skb(struct ioc3_private *ip, struct sk_buff **skb,
>  }
>  
>  #ifdef CONFIG_PCI_XTALK_BRIDGE
> +/* A Bridge 64-bit direct-mapped DMA address carries the target widget in
> + * bits 63:60 and the DMA attributes below it.  An address in the 32-bit
> + * direct window carries neither, so attributes OR-ed onto one form a
> + * 64-bit address aimed at widget 0.
> + */
>  static inline unsigned long ioc3_map(dma_addr_t addr, unsigned long attr)
>  {
> +	if (!(addr & PCI64_ATTR_TARG_MASK))
> +		return addr;

[Severity: Low]
Can the code currently in the tree ever reach this branch? With
CONFIG_PCI_XTALK_BRIDGE, phys_to_dma() is the only source of IOC3 DMA
addresses:

arch/mips/pci/pci-xtalk-bridge.c:phys_to_dma() {
	...
	return bc->baddr + paddr;
}

and bridge_probe() always sets:

	bc->baddr = (u64)bd->masterwid << 60 | PCI64_ATTR_BAR;

masterwid is never zero. IP27 uses 0xa for a directly attached Bridge,
or the hub's xbow port from xbow_probe(). IP30 uses IP30_WIDGET_HEART
(8). That means every address the IOC3 gets from dma_map_single() or
dma_alloc_coherent() has bits 63:60 set. The same applies to the new
check in ioc3_erbar().

dma_to_phys() only masks bits 63:56, so it also could not round-trip a
window address.

The commit message says:

  "An address in the 2GB 32-bit direct window carries neither, so
  attributes OR-ed onto one aim it at widget 0, and every receive
  raises a PCI error."

and:

  "IP27 and IP30 give the IOC3 64-bit addresses; behind an XBridge or
  PIC it can use the window."

In this tree, XBridges are handled by the IP27 and IP30 code
(XBRIDGE_WIDGET_PART_NUM in probe_one_port() and xtalk_init_widget()),
and they still get 64-bit addresses. There is no PIC or IP35 support
yet.

Could the commit message say that this prepares for platform code that
hands out window addresses? As written, it reads like a fix for an RX
failure on existing systems.

> +
>  	return (addr & ~PCI64_ATTR_BAR) | attr;
>  }
>  
> -#define ERBAR_VAL	(ERBAR_BARRIER_BIT << ERBAR_RXBARR_SHIFT)
> +/* The RX barrier's upper address bits follow the same form. */
> +static inline u32 ioc3_erbar(dma_addr_t addr)
> +{
> +	if (!(addr & PCI64_ATTR_TARG_MASK))
> +		return 0;
> +
> +	return ERBAR_BARRIER_BIT << ERBAR_RXBARR_SHIFT;
> +}

[Severity: Medium]
For a window address, ioc3_map() and ioc3_erbar() now drop every Bridge
attribute the driver used to request:

  ioc3_alloc_rx_bufs() and ioc3_rx(): RX buffers lose PCI64_ATTR_BAR
  ioc3_start(): RX and TX ring bases lose PCI64_ATTR_PREC
  ioc3_start_xmit(): TX buffers lose PCI64_ATTR_PREF
  ioc3_init(): ERBAR is written as 0

Window addresses cannot carry these attributes. For 32-bit direct DMA
the Bridge takes them from the slot's b_device register instead.
BRIDGE_DEV_D32_BITS in arch/mips/include/asm/pci/bridge.h lists
BRIDGE_DEV_PREF, BRIDGE_DEV_PRECISE, BRIDGE_DEV_COH and
BRIDGE_DEV_BARRIER for that.

In this tree, the only b_device setup that affects the IOC3 slot is:

arch/mips/pci/pci-xtalk-bridge.c:bridge_probe() {
	...
	for (slot = 0; slot < 8; slot++) {
		bridge_set(bc, b_device[slot].reg, BRIDGE_DEV_SWAP_DIR);
	...
}

followed by the IOC3 fixup:

arch/mips/pci/pci-xtalk-bridge.c:bridge_disable_swapping() {
	...
	bridge_clr(bc, b_device[slot].reg, BRIDGE_DEV_SWAP_DIR);
	...
}

Nothing sets BRIDGE_DEV_BARRIER or the other D32 attribute bits.

Without the barrier, could the RX status write to rxb->w0 become visible
before the packet data that ioc3_rx() reads once it sees ERXBUF_V?

Does the window path rely on platform code setting the D32 bits in
b_device for the IOC3 slot? If so, could the commit message mention
that, or could the setup be done together with this change?

>  #else
>  static inline unsigned long ioc3_map(dma_addr_t addr, unsigned long attr)
>  {

[ ... ]

> @@ -727,7 +745,7 @@ static void ioc3_init(struct net_device *dev)
>  	readl(&regs->emcr);
>  
>  	/* Misc registers  */
> -	writel(ERBAR_VAL, &regs->erbar);
> +	writel(ioc3_erbar(ip->rxr_dma), &regs->erbar);

[Severity: Medium]
Should the ERBAR value be decided from the RX descriptor ring address?
ERBAR is set once for the whole device from the coherent ring address,
but the RX barrier applies to writes into the RX data buffers.

Each buffer is mapped on its own by dma_map_single() in
ioc3_alloc_skb(), and its form is decided per buffer:

ioc3_alloc_rx_bufs():
	ip->rxr[i] = cpu_to_be64(ioc3_map(d, PCI64_ATTR_BAR));

ioc3_rx():
	rxr[n_entry] = cpu_to_be64(ioc3_map(d, PCI64_ATTR_BAR));

drivers/mfd/ioc3.c sets a 64-bit DMA mask, and the 2GB window covers
only part of memory. A platform that picks the window or the 64-bit
form by physical address could therefore put the ring in one form and
the buffers in the other.

If the ring is 64-bit and the buffers use the window, ERBAR still adds
the BAR attribute to barrier writes into window-addressed buffers. That
gives the widget 0 PCI error the commit message describes.

If the ring uses the window and the buffers are 64-bit, the barrier is
cleared for buffers that need it.

Could ioc3_erbar() use something that also holds for the RX buffers?
Alternatively, could the buffer mappings be kept in the same form as the
ring?

>  	readl(&regs->etcdc);			/* Clear on read */
>  	writel(15, &regs->ercsr);		/* RX low watermark  */
>  	writel(0, &regs->ertr);			/* Interrupt immediately */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111095138.1757-1-kaloz%40kernel.org