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

[Geoff Levand <geoff@infradead.org>]

Hi Alexander,

On 2/6/23 08:37, Alexander H Duyck wrote:
> On Sun, 2023-02-05 at 22:10 +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 | 52 ++++++++++----------
>>  1 file changed, 27 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index 7a8b5e1e77a6..5622b512e2e4 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> @@ -309,22 +309,34 @@ static int gelic_card_init_chain(struct gelic_card *card,
>>  				 struct gelic_descr_chain *chain,
>>  				 struct gelic_descr *start_descr, int no)
>>  {
>> -	int i;
>> +	struct device *dev = ctodev(card);
>>  	struct gelic_descr *descr;
>> +	int i;
>>  
>> -	descr = start_descr;
>> -	memset(descr, 0, sizeof(*descr) * no);
>> +	memset(start_descr, 0, no * sizeof(*start_descr));
>>  
>>  	/* set up the hardware pointers in each descriptor */
>> -	for (i = 0; i < no; i++, descr++) {
>> +	for (i = 0, descr = start_descr; i < no; i++, descr++) {
>>  		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
>>  		descr->bus_addr =
>>  			dma_map_single(ctodev(card), descr,
>>  				       GELIC_DESCR_SIZE,
>>  				       DMA_BIDIRECTIONAL);
> 
> Are bus_addr and the CPU the same byte ordering? Just wondering since
> this is being passed raw. I would have expected it to go through a
> cpu_to_be32.

As I mentioned in my reply to the first patch, the PS3's CPU is
big endian, so we really don't need any of the endian conversions.

>> -		if (!descr->bus_addr)
>> -			goto iommu_error;
>> +		if (unlikely(dma_mapping_error(dev, descr->bus_addr))) {
> 
> The expectation for dma_mapping_error is that the address is in cpu
> order. So in this case it is partially correct since bus_addr wasn't
> byte swapped, but generally the dma address used should be a CPU byte
> ordered variable.
> 
>> +			dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
>> +				__LINE__);
>> +
>> +			for (i--, descr--; i > 0; i--, descr--) {
>> +				if (descr->bus_addr) {
> 
> So I am pretty sure this is broken. Usually for something like this I
> will resort to just doing a while (i--) as "i == 0" should be a valid
> buffer to have to unmap.
> 
> Maybe something like:
> 			while (i--) {
> 				descr--;
> 
> Also I think you can get rid of the if since descr->bus_addr should be
> valid for all values since you populated it just a few lines above for
> each value of i.

OK, I'll change that.
>> +					dma_unmap_single(ctodev(card),
>> +						descr->bus_addr,
>> +						GELIC_DESCR_SIZE,
>> +						DMA_BIDIRECTIONAL);
>> +				}
>> +			}
>> +			return -ENOMEM;
>> +		}
>>  
>>  		descr->next = descr + 1;
>>  		descr->prev = descr - 1;
>> @@ -334,8 +346,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
>>  	start_descr->prev = (descr - 1);
>>  
>>  	/* chain bus addr of hw descriptor */
>> -	descr = start_descr;
>> -	for (i = 0; i < no; i++, descr++) {
>> +	for (i = 0, descr = start_descr; i < no; i++, descr++) {
>>  		descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
>>  	}
>>  
> 
> This seems like an unrelated change that was snuck in.

I'll remove that.

Once again, thanks for the review.

-Geoff