| Message ID | 20210727082147.270734-2-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ravb and sh_eth: Fix descriptor counters' conditions | expand |
> On 07/27/2021 10:21 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > > The descriptor counters ({cur,dirty}_[rt]x) acts as free counters > so that conditions are possible to be incorrect when a left value > was overflowed. > > So, for example, ravb_tx_free() could not free any descriptors > because the following condition was checked as a signed value, > and then "NETDEV WATCHDOG" happened: > > for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { > > To fix the issue, add get_num_desc() to calculate numbers of > remaining descriptors. > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 805397088850..70fbac572036 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -172,6 +172,14 @@ static const struct mdiobb_ops bb_ops = { > .get_mdio_data = ravb_get_mdio_data, > }; > > +static u32 get_num_desc(u32 from, u32 subtract) > +{ > + if (from >= subtract) > + return from - subtract; > + > + return U32_MAX - subtract + 1 + from; > +} This is a very roundabout way to implement an unsigned subtraction. :) I think it would make more sense to simply return 0 if "subtract" is larger than "from". (Likewise for sh_eth). CU Uli
> On 07/27/2021 10:55 AM Ulrich Hecht <uli@fpond.eu> wrote: > > > > On 07/27/2021 10:21 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > > > > > The descriptor counters ({cur,dirty}_[rt]x) acts as free counters > > so that conditions are possible to be incorrect when a left value > > was overflowed. > > > > So, for example, ravb_tx_free() could not free any descriptors > > because the following condition was checked as a signed value, > > and then "NETDEV WATCHDOG" happened: > > > > for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { > > > > To fix the issue, add get_num_desc() to calculate numbers of > > remaining descriptors. > > > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/net/ethernet/renesas/ravb_main.c | 22 +++++++++++++++------- > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > > index 805397088850..70fbac572036 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -172,6 +172,14 @@ static const struct mdiobb_ops bb_ops = { > > .get_mdio_data = ravb_get_mdio_data, > > }; > > > > +static u32 get_num_desc(u32 from, u32 subtract) > > +{ > > + if (from >= subtract) > > + return from - subtract; > > + > > + return U32_MAX - subtract + 1 + from; > > +} > > This is a very roundabout way to implement an unsigned subtraction. :) > I think it would make more sense to simply return 0 if "subtract" is larger than "from". > (Likewise for sh_eth). ...and the tests for "> 0" should be rewritten as "!= 0". Sorry, not fully awake yet. CU Uli
Hi Ulrich-san, > From: Ulrich Hecht, Sent: Tuesday, July 27, 2021 6:14 PM > > > On 07/27/2021 10:55 AM Ulrich Hecht <uli@fpond.eu> wrote: > > > > > > > On 07/27/2021 10:21 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > > > > > > > > The descriptor counters ({cur,dirty}_[rt]x) acts as free counters > > > so that conditions are possible to be incorrect when a left value > > > was overflowed. > > > > > > So, for example, ravb_tx_free() could not free any descriptors > > > because the following condition was checked as a signed value, > > > and then "NETDEV WATCHDOG" happened: > > > > > > for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { > > > > > > To fix the issue, add get_num_desc() to calculate numbers of > > > remaining descriptors. > > > > > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > --- > > > drivers/net/ethernet/renesas/ravb_main.c | 22 +++++++++++++++------- > > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > > > index 805397088850..70fbac572036 100644 > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > > @@ -172,6 +172,14 @@ static const struct mdiobb_ops bb_ops = { > > > .get_mdio_data = ravb_get_mdio_data, > > > }; > > > > > > +static u32 get_num_desc(u32 from, u32 subtract) > > > +{ > > > + if (from >= subtract) > > > + return from - subtract; > > > + > > > + return U32_MAX - subtract + 1 + from; > > > +} > > > > This is a very roundabout way to implement an unsigned subtraction. :) I agree :) However... > > I think it would make more sense to simply return 0 if "subtract" is larger than "from". > > (Likewise for sh_eth). > > ...and the tests for "> 0" should be rewritten as "!= 0". Sorry, not fully awake yet. such a change could not fix the issue, IIUC. cur_tx = 0x00000000 dirty_tx = 0xffffffff In that case, numbers of remaining descriptors is 1. So, the patch can return 1. However, if the function return 0, this could not fix the issue because the code could not run into the for statement. --- + for (; get_num_desc(priv->cur_tx[q], priv->dirty_tx[q]) != 0; priv->dirty_tx[q]++) { bool txed; entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] * --- I guess returning 1 instead is possible to be simple. But, the following condition requires actual numbers of descriptors so that the current patch is better, I believe... --- + if (get_num_desc(priv->cur_tx[q], priv->dirty_tx[q]) > + (priv->num_tx_ring[q] - 1) * num_tx_desc) { netif_err(priv, tx_queued, ndev, "still transmitting with the full ring!\n"); netif_stop_subqueue(ndev, q); --- Best regards, Yoshihiro Shimoda
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 805397088850..70fbac572036 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -172,6 +172,14 @@ static const struct mdiobb_ops bb_ops = { .get_mdio_data = ravb_get_mdio_data, }; +static u32 get_num_desc(u32 from, u32 subtract) +{ + if (from >= subtract) + return from - subtract; + + return U32_MAX - subtract + 1 + from; +} + /* Free TX skb function for AVB-IP */ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only) { @@ -183,7 +191,7 @@ static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only) int entry; u32 size; - for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { + for (; get_num_desc(priv->cur_tx[q], priv->dirty_tx[q]) > 0; priv->dirty_tx[q]++) { bool txed; entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] * @@ -536,8 +544,8 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int q) { struct ravb_private *priv = netdev_priv(ndev); int entry = priv->cur_rx[q] % priv->num_rx_ring[q]; - int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) - - priv->cur_rx[q]; + int boguscnt = get_num_desc(priv->dirty_rx[q], priv->cur_rx[q]) + + priv->num_rx_ring[q]; struct net_device_stats *stats = &priv->stats[q]; struct ravb_ex_rx_desc *desc; struct sk_buff *skb; @@ -613,7 +621,7 @@ static bool ravb_rx(struct net_device *ndev, int *quota, int q) } /* Refill the RX ring buffers. */ - for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) { + for (; get_num_desc(priv->cur_rx[q], priv->dirty_rx[q]) > 0; priv->dirty_rx[q]++) { entry = priv->dirty_rx[q] % priv->num_rx_ring[q]; desc = &priv->rx_ring[q][entry]; desc->ds_cc = cpu_to_le16(RX_BUF_SZ); @@ -1499,8 +1507,8 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev) u32 len; spin_lock_irqsave(&priv->lock, flags); - if (priv->cur_tx[q] - priv->dirty_tx[q] > (priv->num_tx_ring[q] - 1) * - num_tx_desc) { + if (get_num_desc(priv->cur_tx[q], priv->dirty_tx[q]) > + (priv->num_tx_ring[q] - 1) * num_tx_desc) { netif_err(priv, tx_queued, ndev, "still transmitting with the full ring!\n"); netif_stop_subqueue(ndev, q); @@ -1598,7 +1606,7 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev) ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q); priv->cur_tx[q] += num_tx_desc; - if (priv->cur_tx[q] - priv->dirty_tx[q] > + if (get_num_desc(priv->cur_tx[q], priv->dirty_tx[q]) > (priv->num_tx_ring[q] - 1) * num_tx_desc && !ravb_tx_free(ndev, q, true)) netif_stop_subqueue(ndev, q);
The descriptor counters ({cur,dirty}_[rt]x) acts as free counters so that conditions are possible to be incorrect when a left value was overflowed. So, for example, ravb_tx_free() could not free any descriptors because the following condition was checked as a signed value, and then "NETDEV WATCHDOG" happened: for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { To fix the issue, add get_num_desc() to calculate numbers of remaining descriptors. Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper") Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/ethernet/renesas/ravb_main.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)