Re: [PATCH net-next v2 2/2] net: stmmac: reduce dma ring display code duplication

[Baruch Siach <baruch@tkos.co.il>]

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@tkos.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@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -