[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87zfzgb3te.fsf@tarshish>
Date: Tue, 14 Nov 2023 20:06:14 +0200
From: Baruch Siach <baruch@...s.co.il>
To: Serge Semin <fancer.lancer@...il.com>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu
<joabreu@...opsys.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/2] net: stmmac: reduce dma ring display
code duplication
Hi Serge,
On Tue, Nov 14 2023, Serge Semin wrote:
> On Tue, Nov 14, 2023 at 09:03:10AM +0200, Baruch Siach wrote:
>> The code to show extended descriptor is identical to normal one.
>> Consolidate the code to remove duplication.
>>
>> Signed-off-by: Baruch Siach <baruch@...s.co.il>
>> ---
>> v2: Fix extended descriptor case, and properly test both cases
>> ---
>> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 25 +++++++------------
>> 1 file changed, 9 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 39336fe5e89d..cf818a2bc9d5 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -6182,26 +6182,19 @@ static void sysfs_display_ring(void *head, int size, int extend_desc,
>> int i;
>> struct dma_extended_desc *ep = (struct dma_extended_desc *)head;
>> struct dma_desc *p = (struct dma_desc *)head;
>
>> + unsigned long desc_size = extend_desc ? sizeof(*ep) : sizeof(*p);
>
> From readability point of view it's better to keep the initializers as
> simple as possible: just type casts or container-of-based inits. The
> more complex init-statements including the ternary-based ones is better to
> move to the code section closer to the place of the vars usage. So could
> you please move the initialization statement from the vars declaration
> section to being performed right before the loop entrance? It shall
> improve the readability a tiny bit.
>
>> dma_addr_t dma_addr;
>>
>> for (i = 0; i < size; i++) {
>> - if (extend_desc) {
>> - dma_addr = dma_phy_addr + i * sizeof(*ep);
>> - seq_printf(seq, "%d [%pad]: 0x%x 0x%x 0x%x 0x%x\n",
>> - i, &dma_addr,
>> - le32_to_cpu(ep->basic.des0),
>> - le32_to_cpu(ep->basic.des1),
>> - le32_to_cpu(ep->basic.des2),
>> - le32_to_cpu(ep->basic.des3));
>> - ep++;
>> - } else {
>> - dma_addr = dma_phy_addr + i * sizeof(*p);
>> - seq_printf(seq, "%d [%pad]: 0x%x 0x%x 0x%x 0x%x\n",
>> - i, &dma_addr,
>> - le32_to_cpu(p->des0), le32_to_cpu(p->des1),
>> - le32_to_cpu(p->des2), le32_to_cpu(p->des3));
>> + dma_addr = dma_phy_addr + i * desc_size;
>> + seq_printf(seq, "%d [%pad]: 0x%x 0x%x 0x%x 0x%x\n",
>> + i, &dma_addr,
>> + le32_to_cpu(p->des0), le32_to_cpu(p->des1),
>> + le32_to_cpu(p->des2), le32_to_cpu(p->des3));
>> + if (extend_desc)
>> + p = &(++ep)->basic;
>> + else
>> p++;
>> - }
>> }
>
> If I were simplifying/improving things I would have done it in the
> next way:
Thanks for your thorough review and detailed comments.
I find your suggestion more verbose for little readability
gain. Readability is a matter of taste, I guess.
I don't feel strongly about this patch. I would be fine with any
decision whether to take it in some form or not.
baruch
>
> static void stmmac_display_ring(void *head, int size, int extend_desc,
> struct seq_file *seq, dma_addr_t dma_addr)
> {
> struct dma_desc *p;
> size_t desc_size;
> int i;
>
> if (extend_desc)
> desc_size = sizeof(struct dma_extended_desc);
> else
> desc_size = sizeof(struct dma_desc);
>
> for (i = 0; i < size; i++) {
> if (extend_desc)
> p = &((struct dma_extended_desc *)head)->basic;
> else
> p = head;
>
> seq_printf(seq, "%d [%pad]: 0x%08x 0x%08x 0x%08x 0x%08x\n",
> i, &dma_addr,
> le32_to_cpu(p->des0), le32_to_cpu(p->des1),
> le32_to_cpu(p->des2), le32_to_cpu(p->des3));
>
> dma_addr += desc_size;
> head += desc_size;
> }
> }
>
> 1. Add 0x%08x format to have the aligned data printout.
> 2. Use the desc-size to increment the virt and phys addresses for
> unification.
> 3. Replace sysfs_ prefix with stmmac_ since the method is no longer
> used for sysfs node.
>
> On the other hand having the extended data printed would be also
> useful at the very least for the Rx descriptors, which expose VLAN,
> Timestamp and IPvX related info. Extended Tx descriptors have only the
> timestamp in the extended part.
>
> -Serge(y)
>
>> }
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@...s.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
Powered by blists - more mailing lists