| Message ID | 20220112173700.873002-5-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Xilinx axienet fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/checkpatch | warning | WARNING: line length of 94 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
> -----Original Message----- > From: Robert Hancock <robert.hancock@calian.com> > Sent: Wednesday, January 12, 2022 11:07 PM > To: netdev@vger.kernel.org > Cc: Radhey Shyam Pandey <radheys@xilinx.com>; davem@davemloft.net; > kuba@kernel.org; linux-arm-kernel@lists.infradead.org; Michal Simek > <michals@xilinx.com>; ariane.keller@tik.ee.ethz.ch; daniel@iogearbox.net; > Robert Hancock <robert.hancock@calian.com> > Subject: [PATCH net v2 4/9] net: axienet: add missing memory barriers > > This driver was missing some required memory barriers: > > Use dma_rmb to ensure we see all updates to the descriptor after we see that > an entry has been completed. > > Use wmb and rmb to avoid stale descriptor status between the TX path and TX > complete IRQ path. > > Fixes: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet > driver") > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > index f4ae035bed35..de8f85175a6c 100644 > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > @@ -632,6 +632,8 @@ static int axienet_free_tx_chain(struct net_device > *ndev, u32 first_bd, > if (nr_bds == -1 && !(status & > XAXIDMA_BD_STS_COMPLETE_MASK)) > break; > > + /* Ensure we see complete descriptor update */ > + dma_rmb(); > phys = desc_get_phys_addr(lp, cur_p); > dma_unmap_single(ndev->dev.parent, phys, > (cur_p->cntrl & > XAXIDMA_BD_CTRL_LENGTH_MASK), @@ -645,8 +647,10 @@ static int > axienet_free_tx_chain(struct net_device *ndev, u32 first_bd, > cur_p->app1 = 0; > cur_p->app2 = 0; > cur_p->app4 = 0; > - cur_p->status = 0; > cur_p->skb = NULL; > + /* ensure our transmit path and device don't prematurely see > status cleared */ > + wmb(); > + cur_p->status = 0; Any reason for moving status initialization down? > > if (sizep) > *sizep += status & > XAXIDMA_BD_STS_ACTUAL_LEN_MASK; @@ -704,6 +708,9 @@ static inline > int axienet_check_tx_bd_space(struct axienet_local *lp, > int num_frag) > { > struct axidma_bd *cur_p; > + > + /* Ensure we see all descriptor updates from device or TX IRQ path */ > + rmb(); > cur_p = &lp->tx_bd_v[(lp->tx_bd_tail + num_frag) % lp->tx_bd_num]; > if (cur_p->status & XAXIDMA_BD_STS_ALL_MASK) > return NETDEV_TX_BUSY; > @@ -843,6 +850,8 @@ static void axienet_recv(struct net_device *ndev) > > tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci; > > + /* Ensure we see complete descriptor update */ > + dma_rmb(); > phys = desc_get_phys_addr(lp, cur_p); > dma_unmap_single(ndev->dev.parent, phys, lp- > >max_frm_size, > DMA_FROM_DEVICE); Ideally we would also need a write barrier in xmit function just before updating tail descriptor. > -- > 2.31.1
On Thu, 2022-01-13 at 12:09 +0000, Radhey Shyam Pandey wrote: > > -----Original Message----- > > From: Robert Hancock <robert.hancock@calian.com> > > Sent: Wednesday, January 12, 2022 11:07 PM > > To: netdev@vger.kernel.org > > Cc: Radhey Shyam Pandey <radheys@xilinx.com>; davem@davemloft.net; > > kuba@kernel.org; linux-arm-kernel@lists.infradead.org; Michal Simek > > <michals@xilinx.com>; ariane.keller@tik.ee.ethz.ch; daniel@iogearbox.net; > > Robert Hancock <robert.hancock@calian.com> > > Subject: [PATCH net v2 4/9] net: axienet: add missing memory barriers > > > > This driver was missing some required memory barriers: > > > > Use dma_rmb to ensure we see all updates to the descriptor after we see > > that > > an entry has been completed. > > > > Use wmb and rmb to avoid stale descriptor status between the TX path and TX > > complete IRQ path. > > > > Fixes: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI > > Ethernet > > driver") > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > --- > > drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > index f4ae035bed35..de8f85175a6c 100644 > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > @@ -632,6 +632,8 @@ static int axienet_free_tx_chain(struct net_device > > *ndev, u32 first_bd, > > if (nr_bds == -1 && !(status & > > XAXIDMA_BD_STS_COMPLETE_MASK)) > > break; > > > > + /* Ensure we see complete descriptor update */ > > + dma_rmb(); > > phys = desc_get_phys_addr(lp, cur_p); > > dma_unmap_single(ndev->dev.parent, phys, > > (cur_p->cntrl & > > XAXIDMA_BD_CTRL_LENGTH_MASK), @@ -645,8 +647,10 @@ static int > > axienet_free_tx_chain(struct net_device *ndev, u32 first_bd, > > cur_p->app1 = 0; > > cur_p->app2 = 0; > > cur_p->app4 = 0; > > - cur_p->status = 0; > > cur_p->skb = NULL; > > + /* ensure our transmit path and device don't prematurely see > > status cleared */ > > + wmb(); > > + cur_p->status = 0; > > Any reason for moving status initialization down? Probably not strictly necessary, but the idea was to ensure that any of the other writes to the descriptor were visible before the device saw the status being cleared (indicating it is available to be read by the device). > > > if (sizep) > > *sizep += status & > > XAXIDMA_BD_STS_ACTUAL_LEN_MASK; @@ -704,6 +708,9 @@ static inline > > int axienet_check_tx_bd_space(struct axienet_local *lp, > > int num_frag) > > { > > struct axidma_bd *cur_p; > > + > > + /* Ensure we see all descriptor updates from device or TX IRQ path */ > > + rmb(); > > cur_p = &lp->tx_bd_v[(lp->tx_bd_tail + num_frag) % lp->tx_bd_num]; > > if (cur_p->status & XAXIDMA_BD_STS_ALL_MASK) > > return NETDEV_TX_BUSY; > > @@ -843,6 +850,8 @@ static void axienet_recv(struct net_device *ndev) > > > > tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci; > > > > + /* Ensure we see complete descriptor update */ > > + dma_rmb(); > > phys = desc_get_phys_addr(lp, cur_p); > > dma_unmap_single(ndev->dev.parent, phys, lp- > > > max_frm_size, > > DMA_FROM_DEVICE); > > Ideally we would also need a write barrier in xmit function just before > updating tail descriptor. I don't think it should be needed there because there is an implicit barrier on the MMIO write. > > > -- > > 2.31.1
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index f4ae035bed35..de8f85175a6c 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -632,6 +632,8 @@ static int axienet_free_tx_chain(struct net_device *ndev, u32 first_bd, if (nr_bds == -1 && !(status & XAXIDMA_BD_STS_COMPLETE_MASK)) break; + /* Ensure we see complete descriptor update */ + dma_rmb(); phys = desc_get_phys_addr(lp, cur_p); dma_unmap_single(ndev->dev.parent, phys, (cur_p->cntrl & XAXIDMA_BD_CTRL_LENGTH_MASK), @@ -645,8 +647,10 @@ static int axienet_free_tx_chain(struct net_device *ndev, u32 first_bd, cur_p->app1 = 0; cur_p->app2 = 0; cur_p->app4 = 0; - cur_p->status = 0; cur_p->skb = NULL; + /* ensure our transmit path and device don't prematurely see status cleared */ + wmb(); + cur_p->status = 0; if (sizep) *sizep += status & XAXIDMA_BD_STS_ACTUAL_LEN_MASK; @@ -704,6 +708,9 @@ static inline int axienet_check_tx_bd_space(struct axienet_local *lp, int num_frag) { struct axidma_bd *cur_p; + + /* Ensure we see all descriptor updates from device or TX IRQ path */ + rmb(); cur_p = &lp->tx_bd_v[(lp->tx_bd_tail + num_frag) % lp->tx_bd_num]; if (cur_p->status & XAXIDMA_BD_STS_ALL_MASK) return NETDEV_TX_BUSY; @@ -843,6 +850,8 @@ static void axienet_recv(struct net_device *ndev) tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci; + /* Ensure we see complete descriptor update */ + dma_rmb(); phys = desc_get_phys_addr(lp, cur_p); dma_unmap_single(ndev->dev.parent, phys, lp->max_frm_size, DMA_FROM_DEVICE);
This driver was missing some required memory barriers: Use dma_rmb to ensure we see all updates to the descriptor after we see that an entry has been completed. Use wmb and rmb to avoid stale descriptor status between the TX path and TX complete IRQ path. Fixes: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver") Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)