| Message ID | 20190306040846.21746-1-rosenp@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [OpenWrt-Devel] ramips: ethernet: fix to interrupt handling | expand |
the original code use status register to keep there still have some pkts in buffer. need next napi call to receive it. if 128 packets in buffer. you clear status first. because napi max handle 64 packets in buffer. so 64 packets need to handle in next napi poll. if no new packet comming. the status register will not set. so fe_poll function will not call fe_poll_tx or fe_poll_rx. that would be a problem. the status register also use to control napi interrupt enable. must make sure no packets need to handle then enable interrupt. Rosen Penev <rosenp@gmail.com> 於 2019年3月6日 週三 下午12:08寫道： > > From: NeilBrown <neil@brown.name> > > The current code acknowledged interrupts *after* polling. > This is the wrong way around, and could cause an interrupt to > be missed. > This is not likely to be fatal as another packet, and so another > interrupt, should come along soon. But maybe it is causing > problems, so let's fix it anyway. > > Signed-off-by: NeilBrown <neil@brown.name> > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > .../drivers/net/ethernet/mediatek/mtk_eth_soc.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c > index e0bc0ab818..2e0c8f94ca 100644 > --- a/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c > +++ b/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c > @@ -876,6 +876,8 @@ static int fe_poll_rx(struct napi_struct *napi, int budget, > struct fe_rx_dma *rxd, trxd; > int done = 0, pad; > > + fe_reg_w32(rx_intr, FE_REG_FE_INT_STATUS); > + > if (netdev->features & NETIF_F_RXCSUM) > checksum_bit = soc->checksum_bit; > else > @@ -963,9 +965,6 @@ release_desc: > done++; > } > > - if (done < budget) > - fe_reg_w32(rx_intr, FE_REG_FE_INT_STATUS); > - > return done; > } > > @@ -981,6 +980,8 @@ static int fe_poll_tx(struct fe_priv *priv, int budget, u32 tx_intr, > u32 idx, hwidx; > struct fe_tx_ring *ring = &priv->tx_ring; > > + fe_reg_w32(tx_intr, FE_REG_FE_INT_STATUS); > + > idx = ring->tx_free_idx; > hwidx = fe_reg_r32(FE_REG_TX_DTX_IDX0); > > @@ -1004,9 +1005,7 @@ static int fe_poll_tx(struct fe_priv *priv, int budget, u32 tx_intr, > if (idx == hwidx) { > /* read hw index again make sure no new tx packet */ > hwidx = fe_reg_r32(FE_REG_TX_DTX_IDX0); > - if (idx == hwidx) > - fe_reg_w32(tx_intr, FE_REG_FE_INT_STATUS); > - else > + if (idx != hwidx) > *tx_again = 1; > } else { > *tx_again = 1; > -- > 2.17.1 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On Wed, Mar 6, 2019 at 12:37 AM Mingyu Li <igvtee@gmail.com> wrote: > > the original code use status register to keep there still have some > pkts in buffer. > need next napi call to receive it. > > if 128 packets in buffer. you clear status first. because napi max > handle 64 packets in buffer. > so 64 packets need to handle in next napi poll. if no new packet > comming. the status register will not set. > so fe_poll function will not call fe_poll_tx or fe_poll_rx. that would > be a problem. > > the status register also use to control napi interrupt enable. must > make sure no packets need to > handle then enable interrupt. I took a look at this again. The upstream kernel driver does the same as this patch: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/net/ethernet/mediatek/mtk_eth_soc.c?h=v4.14.151#n1189 > > Rosen Penev <rosenp@gmail.com> 於 2019年3月6日 週三 下午12:08寫道： > > > > From: NeilBrown <neil@brown.name> > > > > The current code acknowledged interrupts *after* polling. > > This is the wrong way around, and could cause an interrupt to > > be missed. > > This is not likely to be fatal as another packet, and so another > > interrupt, should come along soon. But maybe it is causing > > problems, so let's fix it anyway. > > > > Signed-off-by: NeilBrown <neil@brown.name> > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > --- > > .../drivers/net/ethernet/mediatek/mtk_eth_soc.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > index e0bc0ab818..2e0c8f94ca 100644 > > --- a/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > +++ b/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > @@ -876,6 +876,8 @@ static int fe_poll_rx(struct napi_struct *napi, int budget, > > struct fe_rx_dma *rxd, trxd; > > int done = 0, pad; > > > > + fe_reg_w32(rx_intr, FE_REG_FE_INT_STATUS); > > + > > if (netdev->features & NETIF_F_RXCSUM) > > checksum_bit = soc->checksum_bit; > > else > > @@ -963,9 +965,6 @@ release_desc: > > done++; > > } > > > > - if (done < budget) > > - fe_reg_w32(rx_intr, FE_REG_FE_INT_STATUS); > > - > > return done; > > } > > > > @@ -981,6 +980,8 @@ static int fe_poll_tx(struct fe_priv *priv, int budget, u32 tx_intr, > > u32 idx, hwidx; > > struct fe_tx_ring *ring = &priv->tx_ring; > > > > + fe_reg_w32(tx_intr, FE_REG_FE_INT_STATUS); > > + > > idx = ring->tx_free_idx; > > hwidx = fe_reg_r32(FE_REG_TX_DTX_IDX0); > > > > @@ -1004,9 +1005,7 @@ static int fe_poll_tx(struct fe_priv *priv, int budget, u32 tx_intr, > > if (idx == hwidx) { > > /* read hw index again make sure no new tx packet */ > > hwidx = fe_reg_r32(FE_REG_TX_DTX_IDX0); > > - if (idx == hwidx) > > - fe_reg_w32(tx_intr, FE_REG_FE_INT_STATUS); > > - else > > + if (idx != hwidx) > > *tx_again = 1; > > } else { > > *tx_again = 1; > > -- > > 2.17.1 > > > > > > _______________________________________________ > > openwrt-devel mailing list > > openwrt-devel@lists.openwrt.org > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
the upstream kernel. the function mtk_napi_rx always be called if it have packet to receive. it doesn't check status register. but on openwrt code. check the status register first. then call fe_poll_rx. if you clear status register. fe_poll_rx will will not be called next time. Rosen Penev <rosenp@gmail.com> 於 2019年10月30日 週三 上午12:51寫道： > > On Wed, Mar 6, 2019 at 12:37 AM Mingyu Li <igvtee@gmail.com> wrote: > > > > the original code use status register to keep there still have some > > pkts in buffer. > > need next napi call to receive it. > > > > if 128 packets in buffer. you clear status first. because napi max > > handle 64 packets in buffer. > > so 64 packets need to handle in next napi poll. if no new packet > > comming. the status register will not set. > > so fe_poll function will not call fe_poll_tx or fe_poll_rx. that would > > be a problem. > > > > the status register also use to control napi interrupt enable. must > > make sure no packets need to > > handle then enable interrupt. > I took a look at this again. The upstream kernel driver does the same > as this patch: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/net/ethernet/mediatek/mtk_eth_soc.c?h=v4.14.151#n1189 > > > > Rosen Penev <rosenp@gmail.com> 於 2019年3月6日 週三 下午12:08寫道： > > > > > > From: NeilBrown <neil@brown.name> > > > > > > The current code acknowledged interrupts *after* polling. > > > This is the wrong way around, and could cause an interrupt to > > > be missed. > > > This is not likely to be fatal as another packet, and so another > > > interrupt, should come along soon. But maybe it is causing > > > problems, so let's fix it anyway. > > > > > > Signed-off-by: NeilBrown <neil@brown.name> > > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > > --- > > > .../drivers/net/ethernet/mediatek/mtk_eth_soc.c | 11 +++++------ > > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > > > diff --git a/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > index e0bc0ab818..2e0c8f94ca 100644 > > > --- a/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > +++ b/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > @@ -876,6 +876,8 @@ static int fe_poll_rx(struct napi_struct *napi, int budget, > > > struct fe_rx_dma *rxd, trxd; > > > int done = 0, pad; > > > > > > + fe_reg_w32(rx_intr, FE_REG_FE_INT_STATUS); > > > + > > > if (netdev->features & NETIF_F_RXCSUM) > > > checksum_bit = soc->checksum_bit; > > > else > > > @@ -963,9 +965,6 @@ release_desc: > > > done++; > > > } > > > > > > - if (done < budget) > > > - fe_reg_w32(rx_intr, FE_REG_FE_INT_STATUS); > > > - > > > return done; > > > } > > > > > > @@ -981,6 +980,8 @@ static int fe_poll_tx(struct fe_priv *priv, int budget, u32 tx_intr, > > > u32 idx, hwidx; > > > struct fe_tx_ring *ring = &priv->tx_ring; > > > > > > + fe_reg_w32(tx_intr, FE_REG_FE_INT_STATUS); > > > + > > > idx = ring->tx_free_idx; > > > hwidx = fe_reg_r32(FE_REG_TX_DTX_IDX0); > > > > > > @@ -1004,9 +1005,7 @@ static int fe_poll_tx(struct fe_priv *priv, int budget, u32 tx_intr, > > > if (idx == hwidx) { > > > /* read hw index again make sure no new tx packet */ > > > hwidx = fe_reg_r32(FE_REG_TX_DTX_IDX0); > > > - if (idx == hwidx) > > > - fe_reg_w32(tx_intr, FE_REG_FE_INT_STATUS); > > > - else > > > + if (idx != hwidx) > > > *tx_again = 1; > > > } else { > > > *tx_again = 1; > > > -- > > > 2.17.1 > > > > > > > > > _______________________________________________ > > > openwrt-devel mailing list > > > openwrt-devel@lists.openwrt.org > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff --git a/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c index e0bc0ab818..2e0c8f94ca 100644 --- a/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/target/linux/ramips/files-4.14/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -876,6 +876,8 @@ static int fe_poll_rx(struct napi_struct *napi, int budget, struct fe_rx_dma *rxd, trxd; int done = 0, pad; + fe_reg_w32(rx_intr, FE_REG_FE_INT_STATUS); + if (netdev->features & NETIF_F_RXCSUM) checksum_bit = soc->checksum_bit; else @@ -963,9 +965,6 @@ release_desc: done++; } - if (done < budget) - fe_reg_w32(rx_intr, FE_REG_FE_INT_STATUS); - return done; } @@ -981,6 +980,8 @@ static int fe_poll_tx(struct fe_priv *priv, int budget, u32 tx_intr, u32 idx, hwidx; struct fe_tx_ring *ring = &priv->tx_ring; + fe_reg_w32(tx_intr, FE_REG_FE_INT_STATUS); + idx = ring->tx_free_idx; hwidx = fe_reg_r32(FE_REG_TX_DTX_IDX0); @@ -1004,9 +1005,7 @@ static int fe_poll_tx(struct fe_priv *priv, int budget, u32 tx_intr, if (idx == hwidx) { /* read hw index again make sure no new tx packet */ hwidx = fe_reg_r32(FE_REG_TX_DTX_IDX0); - if (idx == hwidx) - fe_reg_w32(tx_intr, FE_REG_FE_INT_STATUS); - else + if (idx != hwidx) *tx_again = 1; } else { *tx_again = 1;