Re: [PATCH net] ethernet: et131x: Add missing check after DMA map

[Simon Horman <horms@kernel.org>]

On Mon, Jul 07, 2025 at 11:09:49AM +0200, Thomas Fourier wrote:
> The DMA map functions can fail and should be tested for errors.
> If the mapping fails, unmap and return an error.
> 
> Fixes: 38df6492eb51 ("et131x: Add PCIe gigabit ethernet driver et131x to drivers/net")
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>

nits:

1) There are two spaces after "et131x:" in the subject.
   One is enough.

2) I think you can drop "ethernet: " from the subject.
   "et131x: " seems to be an appropriate prefix based on git history.

...

> @@ -2578,6 +2593,28 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb)
>  		       &adapter->regs->global.watchdog_timer);
>  	}
>  	return 0;
> +
> +unmap_out:
> +	// Unmap everything from i-1 to 1
> +	while (--i) {
> +		frag--;
> +		dma_addr = desc[frag].addr_lo;
> +		dma_addr |= (u64)desc[frag].addr_hi << 32;
> +		dma_unmap_page(&adapter->pdev->dev, dma_addr,
> +			       desc[frag].len_vlan, DMA_TO_DEVICE);
> +	}

I'm probably missing something obvious. But it seems to me that frag is
incremented iff a mapping is successful. So I think only the loop below is
needed.

> +
> +unmap_first_out:
> +	// unmap header
> +	while (frag--) {
> +		frag--;

I don't think you want to decrement frag twice here.

> +		dma_addr = desc[frag].addr_lo;
> +		dma_addr |= (u64)desc[frag].addr_hi << 32;
> +		dma_unmap_single(&adapter->pdev->dev, dma_addr,
> +				 desc[frag].len_vlan, DMA_TO_DEVICE);
> +	}
> +
> +	return -ENOMEM;
>  }
>  
>  static int send_packet(struct sk_buff *skb, struct et131x_adapter *adapter)

-- 
pw-bot: changes-requested