Re: [PATCH net v7 2/2] net/ps3_gelic_net: Use dma_mapping_error

[Geoff Levand <geoff@infradead.org>]

Hi,

On 3/6/23 08:01, Alexander H Duyck wrote:
> On Sun, 2023-03-05 at 02:08 +0000, Geoff Levand wrote:
>> The current Gelic Etherenet driver was checking the return value of its
>> dma_map_single call, and not using the dma_mapping_error() routine.
>>
>> Fixes runtime problems like these:
>>
>>   DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
>>   WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
>>
>> Fixes: 02c1889166b4 ("ps3: gigabit ethernet driver for PS3, take3")
>> Signed-off-by: Geoff Levand <geoff@infradead.org>
>> ---
>>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index b0ebe0e603b4..40261947e0ea 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> @@ -323,7 +323,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
>>  				       GELIC_DESCR_SIZE,
>>  				       DMA_BIDIRECTIONAL);
>>  
>> -		if (!descr->bus_addr)
>> +		if (dma_mapping_error(ctodev(card), descr->bus_addr))
>>  			goto iommu_error;
>>  
>>  		descr->next = descr + 1;
> 
> The bus_addr value is __be32 and the dma_mapping_error should be CPU
> ordered. I think there was a byteswap using cpu_to_be32 missing here.
> In addition you will probably need to have an intermediate variable to
> store it in to test the DMA address before you byte swap it and store
> it in the descriptor.

I added a local variable 'cpu_addr' as you recommend.

>> @@ -401,7 +401,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>>  						     descr->skb->data,
>>  						     GELIC_NET_MAX_FRAME,
>>  						     DMA_FROM_DEVICE));
>> -	if (!descr->buf_addr) {
>> +	if (dma_mapping_error(ctodev(card), descr->buf_addr)) {
>>  		dev_kfree_skb_any(descr->skb);
>>  		descr->skb = NULL;
>>  		dev_info(ctodev(card),
> 
> This is happening AFTER the DMA is passed through a cpu_to_be32 right?
> The test should be on the raw value, not the byteswapped value.

Did the same here.

>> @@ -781,7 +781,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
>>  
>>  	buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
>>  
>> -	if (!buf) {
>> +	if (dma_mapping_error(ctodev(card), buf)) {
>>  		dev_err(ctodev(card),
>>  			"dma map 2 failed (%p, %i). Dropping packet\n",
>>  			skb->data, skb->len);
> 
> This one is correct from what I can tell. I would recommend using it as
> a template and applying it to the two above so that you can sort out
> the byte ordering issues and perform the test and the CPU ordered DMA
> variable.

Thanks for the review.

-Geoff