> -----Original Message-----
> From: Jakub Kicinski <k...@kernel.org>
> Sent: 2021年2月23日 3:46
> To: Joakim Zhang <qiangqing.zh...@nxp.com>
> Cc: peppe.cavall...@st.com; alexandre.tor...@st.com;
> joab...@synopsys.com; da...@davemloft.net; netdev@vger.kernel.org;
> dl-linux-imx <linux-...@nxp.com>
> Subject: Re: [PATCH V4 net 3/5] net: stmmac: fix dma physical address of
> descriptor when display ring
>
> On Sat, 20 Feb 2021 07:43:33 +0000 Joakim Zhang wrote:
> > > > pr_info("%s descriptor ring:\n", rx ? "RX" : "TX");
> > > >
> > > > for (i = 0; i < size; i++) {
> > > > - pr_info("%03d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n",
> > > > - i, (unsigned int)virt_to_phys(p),
> > > > + pr_info("%03d [0x%llx]: 0x%x 0x%x 0x%x 0x%x\n",
> > > > + i, (unsigned long long)(dma_rx_phy + i *
> > > > desc_size),
> > > > le32_to_cpu(p->des0), le32_to_cpu(p->des1),
> > > > le32_to_cpu(p->des2), le32_to_cpu(p->des3));
> > > > p++;
> > >
> > > Why do you pass the desc_size in? The virt memory pointer is
> > > incremented by
> > > sizeof(*p) surely
> > >
> > > dma_addr + i * sizeof(*p)
> >
> > I think we can't use sizeof(*p), as when display descriptor, only do "
> > struct dma_desc *p = (struct dma_desc *)head;", but driver can pass
> > "struct dma_desc", " struct dma_edesc" or " struct dma_extended_desc",
>
> Looks like some of the functions you change already try to pick the right
> type.
> Which one is problematic?
Yes, some functions have picked the right type:
drivers/net/ethernet/stmicro/stmmac/enh_desc.c -> enh_desc_display_ring()
drivers/net/ethernet/stmicro/stmmac/norm_desc.c -> ndesc_display_ring()
the problematic one is:
drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c -> dwmac4_display_ring()
Since the callback format is the same for them, and used from stmmac_main.c in
the same way.
drivers/net/ethernet/stmicro/stmmac/hwif.h -> void (*display_ring)(void *head,
unsigned int size, bool rx);
So I decide to modify them as a whole to avoid separate them as different
format which would introduce more redundant code. Is it reasonable?
> > so it's necessary to pass desc_size to compatible all cases.
>
> But you still increment the the VMA pointer ('p' in the quote above) but it's
> size,
> so how is that correct if the DMA addr needs a special size increment?
Yes, you are right. It indeed a problem. Seems dwmac4_display_ring() function
has not supported different desc format well.
DMA phy address is just one of its problem. I will fix it together. Thanks.
Best Regards,
Joakim Zhang
