netdev - Re: [PATCH net-next v2 13/17] ethernet: ucc_geth: remove bd_mem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8c31cc00-b1bc-cefa-8ea8-5907b8fbe6ef@prevas.dk>
Date:   Wed, 20 Jan 2021 12:33:33 +0100
From:   Rasmus Villemoes <rasmus.villemoes@...vas.dk>
To:     Christophe Leroy <christophe.leroy@...roup.eu>,
        netdev@...r.kernel.org
Cc:     Li Yang <leoyang.li@....com>,
        "David S . Miller" <davem@...emloft.net>,
        Zhao Qiang <qiang.zhao@....com>, Andrew Lunn <andrew@...n.ch>,
        Jakub Kicinski <kuba@...nel.org>,
        Joakim Tjernlund <Joakim.Tjernlund@...inera.com>
Subject: Re: [PATCH net-next v2 13/17] ethernet: ucc_geth: remove bd_mem_part
 and all associated code

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