| Message ID | 20210204112144.24163-4-qiangqing.zhang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethernet: fixes for stmmac driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: mcoquelin.stm32@gmail.com linux-stm32@st-md-mailman.stormreply.com linux-arm-kernel@lists.infradead.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 22 this patch: 22 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 88 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 22 this patch: 22 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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@nxp.com> What does this patch fix? Theoretically incorrect value in a debug dump or are you actually observing incorrect behavior? > 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) would work correctly? Also please use the correct print format for dma_addr_t, you shouldn't have to cast.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年2月7日 4:29 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.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@nxp.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
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? > 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?
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: 2021年2月23日 3:46 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: peppe.cavallaro@st.com; alexandre.torgue@st.com; > joabreu@synopsys.com; davem@davemloft.net; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@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
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++; diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c index d02cec296f51..eaea4cf02386 100644 --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c @@ -417,7 +417,8 @@ static int enh_desc_get_rx_timestamp_status(void *desc, void *next_desc, } } -static void enh_desc_display_ring(void *head, unsigned int size, bool rx) +static void enh_desc_display_ring(void *head, unsigned int size, bool rx, + dma_addr_t dma_rx_phy, unsigned int desc_size) { struct dma_extended_desc *ep = (struct dma_extended_desc *)head; int i; @@ -428,8 +429,8 @@ static void enh_desc_display_ring(void *head, unsigned int size, bool rx) u64 x; x = *(u64 *)ep; - pr_info("%03d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n", - i, (unsigned int)virt_to_phys(ep), + pr_info("%03d [0x%llx]: 0x%x 0x%x 0x%x 0x%x\n", + i, (unsigned long long)(dma_rx_phy + i * desc_size), (unsigned int)x, (unsigned int)(x >> 32), ep->basic.des2, ep->basic.des3); ep++; diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h index b40b2e0667bb..7417db31402f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h @@ -78,7 +78,8 @@ struct stmmac_desc_ops { /* get rx timestamp status */ int (*get_rx_timestamp_status)(void *desc, void *next_desc, u32 ats); /* Display ring */ - void (*display_ring)(void *head, unsigned int size, bool rx); + void (*display_ring)(void *head, unsigned int size, bool rx, + dma_addr_t dma_rx_phy, unsigned int desc_size); /* set MSS via context descriptor */ void (*set_mss)(struct dma_desc *p, unsigned int mss); /* get descriptor skbuff address */ diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c index f083360e4ba6..36e769d2e312 100644 --- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c +++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c @@ -269,7 +269,8 @@ static int ndesc_get_rx_timestamp_status(void *desc, void *next_desc, u32 ats) return 1; } -static void ndesc_display_ring(void *head, unsigned int size, bool rx) +static void ndesc_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; @@ -280,8 +281,8 @@ static void ndesc_display_ring(void *head, unsigned int size, bool rx) u64 x; x = *(u64 *)p; - pr_info("%03d [0x%x]: 0x%x 0x%x 0x%x 0x%x", - i, (unsigned int)virt_to_phys(p), + pr_info("%03d [0x%llx]: 0x%x 0x%x 0x%x 0x%x", + i, (unsigned long long)(dma_rx_phy + i * desc_size), (unsigned int)x, (unsigned int)(x >> 32), p->des2, p->des3); p++; diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 12ed337a239b..b1950fd4eb80 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1133,6 +1133,7 @@ static int stmmac_phy_setup(struct stmmac_priv *priv) static void stmmac_display_rx_rings(struct stmmac_priv *priv) { u32 rx_cnt = priv->plat->rx_queues_to_use; + unsigned int desc_size; void *head_rx; u32 queue; @@ -1142,19 +1143,24 @@ static void stmmac_display_rx_rings(struct stmmac_priv *priv) pr_info("\tRX Queue %u rings\n", queue); - if (priv->extend_desc) + if (priv->extend_desc) { head_rx = (void *)rx_q->dma_erx; - else + desc_size = sizeof(struct dma_extended_desc); + } else { head_rx = (void *)rx_q->dma_rx; + desc_size = sizeof(struct dma_desc); + } /* Display RX ring */ - stmmac_display_ring(priv, head_rx, priv->dma_rx_size, true); + stmmac_display_ring(priv, head_rx, priv->dma_rx_size, true, + rx_q->dma_rx_phy, desc_size); } } static void stmmac_display_tx_rings(struct stmmac_priv *priv) { u32 tx_cnt = priv->plat->tx_queues_to_use; + unsigned int desc_size; void *head_tx; u32 queue; @@ -1164,14 +1170,19 @@ static void stmmac_display_tx_rings(struct stmmac_priv *priv) pr_info("\tTX Queue %d rings\n", queue); - if (priv->extend_desc) + if (priv->extend_desc) { head_tx = (void *)tx_q->dma_etx; - else if (tx_q->tbs & STMMAC_TBS_AVAIL) + desc_size = sizeof(struct dma_extended_desc); + } else if (tx_q->tbs & STMMAC_TBS_AVAIL) { head_tx = (void *)tx_q->dma_entx; - else + desc_size = sizeof(struct dma_edesc); + } else { head_tx = (void *)tx_q->dma_tx; + desc_size = sizeof(struct dma_desc); + } - stmmac_display_ring(priv, head_tx, priv->dma_tx_size, false); + stmmac_display_ring(priv, head_tx, priv->dma_tx_size, false, + tx_q->dma_tx_phy, desc_size); } } @@ -3736,18 +3747,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) unsigned int count = 0, error = 0, len = 0; int status = 0, coe = priv->hw->rx_csum; unsigned int next_entry = rx_q->cur_rx; + unsigned int desc_size; struct sk_buff *skb = NULL; if (netif_msg_rx_status(priv)) { void *rx_head; netdev_dbg(priv->dev, "%s: descriptor ring:\n", __func__); - if (priv->extend_desc) + if (priv->extend_desc) { rx_head = (void *)rx_q->dma_erx; - else + desc_size = sizeof(struct dma_extended_desc); + } else { rx_head = (void *)rx_q->dma_rx; + desc_size = sizeof(struct dma_desc); + } - stmmac_display_ring(priv, rx_head, priv->dma_rx_size, true); + stmmac_display_ring(priv, rx_head, priv->dma_rx_size, true, + rx_q->dma_rx_phy, desc_size); } while (count < limit) { unsigned int buf1_len = 0, buf2_len = 0; @@ -4315,7 +4331,7 @@ static int stmmac_set_mac_address(struct net_device *ndev, void *addr) static struct dentry *stmmac_fs_dir; static void sysfs_display_ring(void *head, int size, int extend_desc, - struct seq_file *seq) + struct seq_file *seq, dma_addr_t dma_phy_addr) { int i; struct dma_extended_desc *ep = (struct dma_extended_desc *)head; @@ -4323,16 +4339,16 @@ static void sysfs_display_ring(void *head, int size, int extend_desc, for (i = 0; i < size; i++) { if (extend_desc) { - seq_printf(seq, "%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n", - i, (unsigned int)virt_to_phys(ep), + seq_printf(seq, "%d [0x%llx]: 0x%x 0x%x 0x%x 0x%x\n", + i, (unsigned long long)(dma_phy_addr + i * sizeof(ep)), 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 { - seq_printf(seq, "%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n", - i, (unsigned int)virt_to_phys(p), + seq_printf(seq, "%d [0x%llx]: 0x%x 0x%x 0x%x 0x%x\n", + i, (unsigned long long)(dma_phy_addr + i * sizeof(p)), le32_to_cpu(p->des0), le32_to_cpu(p->des1), le32_to_cpu(p->des2), le32_to_cpu(p->des3)); p++; @@ -4360,11 +4376,11 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v) if (priv->extend_desc) { seq_printf(seq, "Extended descriptor ring:\n"); sysfs_display_ring((void *)rx_q->dma_erx, - priv->dma_rx_size, 1, seq); + priv->dma_rx_size, 1, seq, rx_q->dma_rx_phy); } else { seq_printf(seq, "Descriptor ring:\n"); sysfs_display_ring((void *)rx_q->dma_rx, - priv->dma_rx_size, 0, seq); + priv->dma_rx_size, 0, seq, rx_q->dma_rx_phy); } } @@ -4376,11 +4392,11 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v) if (priv->extend_desc) { seq_printf(seq, "Extended descriptor ring:\n"); sysfs_display_ring((void *)tx_q->dma_etx, - priv->dma_tx_size, 1, seq); + priv->dma_tx_size, 1, seq, tx_q->dma_tx_phy); } else if (!(tx_q->tbs & STMMAC_TBS_AVAIL)) { seq_printf(seq, "Descriptor ring:\n"); sysfs_display_ring((void *)tx_q->dma_tx, - priv->dma_tx_size, 0, seq); + priv->dma_tx_size, 0, seq, tx_q->dma_tx_phy); } }
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@nxp.com> --- .../ethernet/stmicro/stmmac/dwmac4_descs.c | 7 +-- .../net/ethernet/stmicro/stmmac/enh_desc.c | 7 +-- drivers/net/ethernet/stmicro/stmmac/hwif.h | 3 +- .../net/ethernet/stmicro/stmmac/norm_desc.c | 7 +-- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 54 ++++++++++++------- 5 files changed, 49 insertions(+), 29 deletions(-)