| Message ID | 27ad91b102bf9555e61bb1013672c2bc558e97b9.1699945390.git.baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,1/2] net: stmmac: remove extra newline from descriptors display | expand |
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 > >
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) > >> }
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
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++; - } } }
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(-)