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

[Thomas Fourier <fourier.thomas@gmail.com>]

On 07/07/2025 22:01, Simon Horman wrote:

> 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.
>
> ...
Ok, I'll make sure to fix that.
>
>> @@ -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.
Yes, frag is incremented only after a successful mapping.

The first loop is to unmap the body of the packet (i >= 1) which is mapped

with skb_frag_dma_map() (and unmapped with dma_unmap_page).  The

second is to unmap the header which is either one or two mappings of

map_single.  Should I make the comments more explicit?

>
>> +
>> +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)