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

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]

Date: Tue, 14 Nov 2023 16:52:11 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Baruch Siach <baruch@...s.co.il>
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

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:

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
> 
>