Re: [PATCH net-next v2 13/17] ethernet: ucc_geth: remove bd_mem_part and all associated code

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 20/01/2021 08.17, Christophe Leroy wrote:
> 
> Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit :
>> The bd_mem_part member of ucc_geth_info always has the value
>> MEM_PART_SYSTEM, and AFAICT, there has never been any code setting it
>> to any other value. Moreover, muram is a somewhat precious resource,
>> so there's no point using that when normal memory serves just as well.
>>
>> Apart from removing a lot of dead code, this is also motivated by
>> wanting to clean up the "store result from kmalloc() in a u32" mess.
>>
>> @@ -2195,25 +2179,15 @@ static int ucc_geth_alloc_tx(struct
>> ucc_geth_private *ugeth)
>>           if ((ug_info->bdRingLenTx[j] * sizeof(struct qe_bd)) %
>>               UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT)
>>               length += UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT;
>> -        }
>> +
>> +        ugeth->tx_bd_ring_offset[j] =
>> +            (u32) kmalloc((u32) (length + align), GFP_KERNEL);
> 
> Can't this fit on a single ? Nowadays, max allowed line length is 100
> chars.
> 
>> +
>> +        if (ugeth->tx_bd_ring_offset[j] != 0)
>> +            ugeth->p_tx_bd_ring[j] =
>> +                (u8 __iomem *)((ugeth->tx_bd_ring_offset[j] +
>> +                        align) & ~(align - 1));
> 
> Can we get the above fit on only 2 lines ?
> 
>> +
>> -        }
>> +        ugeth->rx_bd_ring_offset[j] =
>> +            (u32) kmalloc((u32) (length + align), GFP_KERNEL);
> 
> Same.

This is all deliberate: Verifying that this patch merely removes the
dead branch (and thus outdenting the always-taken branch) is easily done
by using "git show -w". That shows hunks like

@@ -2554,20 +2519,11 @@ static int ucc_geth_startup(struct
ucc_geth_private *ugeth)
                endOfRing =
                    ugeth->p_tx_bd_ring[i] + (ug_info->bdRingLenTx[i] -
                                              1) * sizeof(struct qe_bd);
-               if (ugeth->ug_info->uf_info.bd_mem_part ==
MEM_PART_SYSTEM) {
                out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
                         (u32) virt_to_phys(ugeth->p_tx_bd_ring[i]));
                out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
                         last_bd_completed_address,
                         (u32) virt_to_phys(endOfRing));
-               } else if (ugeth->ug_info->uf_info.bd_mem_part ==
-                          MEM_PART_MURAM) {
-
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
-                                (u32)qe_muram_dma(ugeth->p_tx_bd_ring[i]));
-                       out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
-                                last_bd_completed_address,
-                                (u32)qe_muram_dma(endOfRing));
-               }
        }

So I didn't want to rewrap any of the lines.

Rasmus