[PATCH net-next v2 1/2] net: stmmac: remove extra newline from descriptors display

[PATCH net-next v2 1/2] net: stmmac: remove extra newline from descriptors display

[Baruch Siach]

One newline per line should be enough. Reduce the verbosity of
descriptors dump.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3e50fd53a617..39336fe5e89d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6202,7 +6202,6 @@ static void sysfs_display_ring(void *head, int size, int extend_desc,
 				   le32_to_cpu(p->des2), le32_to_cpu(p->des3));
 			p++;
 		}
-		seq_printf(seq, "\n");
 	}
 }
 
-- 
2.42.0

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

[Baruch Siach]

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);
 	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++;
-		}
 	}
 }
 
-- 
2.42.0

Re: [PATCH net-next v2 1/2] net: stmmac: remove extra newline from descriptors display

[Serge Semin]

On Tue, Nov 14, 2023 at 09:03:09AM +0200, Baruch Siach wrote:
> One newline per line should be enough. Reduce the verbosity of
> descriptors dump.

Why not. Thanks.
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3e50fd53a617..39336fe5e89d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6202,7 +6202,6 @@ static void sysfs_display_ring(void *head, int size, int extend_desc,
>  				   le32_to_cpu(p->des2), le32_to_cpu(p->des3));
>  			p++;
>  		}
> -		seq_printf(seq, "\n");
>  	}
>  }
>  
> -- 
> 2.42.0
> 
> 

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

[Serge Semin]

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:

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)

>  }
>  
> -- 
> 2.42.0
> 
> 

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

[Baruch Siach]

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 -

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

[Paolo Abeni]

On Tue, 2023-11-14 at 09:03 +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);
>  	dma_addr_t dma_addr;

Since this is a cleanup refactor, please reorganize the variables
declarations to respect the reverse xmas tree order.

WRT the ternary operator at initialization time, I also feel it should
be better move it out of the declaration.

Cheers,

Paolo