netdev - RE: [PATCH V4 net 3/5] net: stmmac: fix dma physical address of descriptor when display ring

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:   Sat, 20 Feb 2021 07:43:33 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "peppe.cavallaro@...com" <peppe.cavallaro@...com>,
        "alexandre.torgue@...com" <alexandre.torgue@...com>,
        "joabreu@...opsys.com" <joabreu@...opsys.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH V4 net 3/5] net: stmmac: fix dma physical address of
 descriptor when display ring


> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: 2021年2月7日 4:29
> To: Joakim Zhang <qiangqing.zhang@....com>
> Cc: peppe.cavallaro@...com; alexandre.torgue@...com;
> joabreu@...opsys.com; davem@...emloft.net; netdev@...r.kernel.org;
> dl-linux-imx <linux-imx@....com>
> Subject: Re: [PATCH V4 net 3/5] net: stmmac: fix dma physical address of
> descriptor when display ring
> 
> On Thu,  4 Feb 2021 19:21:42 +0800 Joakim Zhang wrote:
> > Driver uses dma_alloc_coherent to allocate dma memory for descriptors,
> > dma_alloc_coherent will return both the virtual address and physical
> > address. AFAIK, virt_to_phys could not convert virtual address to
> > physical address, for which memory is allocated by dma_alloc_coherent.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@....com>
> 
> What does this patch fix? Theoretically incorrect value in a debug dump or are
> you actually observing incorrect behavior?

Hi Jakub,

Sorry for late response since I am on holiday. 
It is incorrect value in a debug dump. When I debug DMA issue, I found the descriptor address is incorrect, which could be indicated by hardware register.
Correct descriptor address could help use locate that which is the issue descriptor, and then could further debug.

> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> > index c6540b003b43..6f951adc5f90 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> > @@ -402,7 +402,8 @@ static void dwmac4_rd_set_tx_ic(struct dma_desc
> *p)
> >  	p->des2 |= cpu_to_le32(TDES2_INTERRUPT_ON_COMPLETION);
> >  }
> >
> > -static void dwmac4_display_ring(void *head, unsigned int size, bool
> > rx)
> > +static void dwmac4_display_ring(void *head, unsigned int size, bool rx,
> > +				dma_addr_t dma_rx_phy, unsigned int desc_size)
> >  {
> >  	struct dma_desc *p = (struct dma_desc *)head;
> >  	int i;
> > @@ -410,8 +411,8 @@ static void dwmac4_display_ring(void *head,
> unsigned int size, bool rx)
> >  	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",
so it's necessary to pass desc_size to compatible all cases.
 
> would work correctly? Also please use the correct print format for dma_addr_t,
> you shouldn't have to cast.

OK, I will change to use "%pad" for dma_addr_t.


Best Regards,
Joakim Zhang