[PATCH 0/2] ravb and sh_eth: Fix descriptor counters' conditions

[PATCH 0/2] ravb and sh_eth: Fix descriptor counters' conditions

[Yoshihiro Shimoda]

When we change the type of {cur,dirty}_tx from u32 to u8, we can
reproduce this issue easily.

Yoshihiro Shimoda (2):
  ravb: Fix descriptor counters' conditions
  sh_eth: Fix descriptor counters' conditions

 drivers/net/ethernet/renesas/ravb_main.c | 22 +++++++++++++++-------
 drivers/net/ethernet/renesas/sh_eth.c    | 16 ++++++++++++----
 2 files changed, 27 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH 1/2] ravb: Fix descriptor counters' conditions

[Yoshihiro Shimoda]

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;
+}
+
 /* 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);
-- 
2.25.1

[PATCH 2/2] sh_eth: Fix descriptor counters' conditions

[Yoshihiro Shimoda]

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, sh_eth_tx_free() could not free any descriptors
because the following condition was checked as a signed value,
and then "NETDEV WATCHDOG" happened:

    for (; mdp->cur_tx - mdp->dirty_tx > 0; mdp->dirty_tx++) {

To fix the issue, add get_num_desc() to calculate numbers of
remaining descriptors.

Fixes: 86a74ff21a7a ("net: sh_eth: add support for Renesas SuperH Ethernet")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 840478692a37..7c9445ad684b 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1227,6 +1227,14 @@ static const struct mdiobb_ops bb_ops = {
 	.get_mdio_data = sh_get_mdio,
 };
 
+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 */
 static int sh_eth_tx_free(struct net_device *ndev, bool sent_only)
 {
@@ -1236,7 +1244,7 @@ static int sh_eth_tx_free(struct net_device *ndev, bool sent_only)
 	int entry;
 	bool sent;
 
-	for (; mdp->cur_tx - mdp->dirty_tx > 0; mdp->dirty_tx++) {
+	for (; get_num_desc(mdp->cur_tx, mdp->dirty_tx) > 0; mdp->dirty_tx++) {
 		entry = mdp->dirty_tx % mdp->num_tx_ring;
 		txdesc = &mdp->tx_ring[entry];
 		sent = !(txdesc->status & cpu_to_le32(TD_TACT));
@@ -1587,7 +1595,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 	struct sh_eth_rxdesc *rxdesc;
 
 	int entry = mdp->cur_rx % mdp->num_rx_ring;
-	int boguscnt = (mdp->dirty_rx + mdp->num_rx_ring) - mdp->cur_rx;
+	int boguscnt = get_num_desc(mdp->dirty_rx, mdp->cur_rx) + mdp->num_rx_ring;
 	int limit;
 	struct sk_buff *skb;
 	u32 desc_status;
@@ -1667,7 +1675,7 @@ static int sh_eth_rx(struct net_device *ndev, u32 intr_status, int *quota)
 	}
 
 	/* Refill the Rx ring buffers. */
-	for (; mdp->cur_rx - mdp->dirty_rx > 0; mdp->dirty_rx++) {
+	for (; get_num_desc(mdp->cur_rx, mdp->dirty_rx) > 0; mdp->dirty_rx++) {
 		entry = mdp->dirty_rx % mdp->num_rx_ring;
 		rxdesc = &mdp->rx_ring[entry];
 		/* The size of the buffer is 32 byte boundary. */
@@ -2499,7 +2507,7 @@ static netdev_tx_t sh_eth_start_xmit(struct sk_buff *skb,
 	unsigned long flags;
 
 	spin_lock_irqsave(&mdp->lock, flags);
-	if ((mdp->cur_tx - mdp->dirty_tx) >= (mdp->num_tx_ring - 4)) {
+	if (get_num_desc(mdp->cur_tx, mdp->dirty_tx) >= (mdp->num_tx_ring - 4)) {
 		if (!sh_eth_tx_free(ndev, true)) {
 			netif_warn(mdp, tx_queued, ndev, "TxFD exhausted.\n");
 			netif_stop_queue(ndev);
-- 
2.25.1

Re: [PATCH 1/2] ravb: Fix descriptor counters' conditions

[Ulrich Hecht]

> 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

Re: [PATCH 1/2] ravb: Fix descriptor counters' conditions

[Ulrich Hecht]

> 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

RE: [PATCH 1/2] ravb: Fix descriptor counters' conditions

[Yoshihiro Shimoda]

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

RE: [PATCH 0/2] ravb and sh_eth: Fix descriptor counters' conditions

[Yoshihiro Shimoda]

Hi all,

> From: Yoshihiro Shimoda, Sent: Tuesday, July 27, 2021 5:22 PM
> 
> When we change the type of {cur,dirty}_tx from u32 to u8, we can
> reproduce this issue easily.

I'm afraid but I would like to recall this patch series because
the original code didn't have any issue around these counters
so that the patch series should not be applied.

For just a record, I tried to write why I misunderstood these counters are incorrect below:
- I got a report locally about an issue happen when the system sends/receives data in long time transfer
- So, I doubted these counters' overflow.
- To reproduce the issue quickly, I changed the type from u32 to u8.
- And then, the following condition will not work correctly (as I mentioned on the patch).

       for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { --- [1]

- Also, I found if we used "> 0U" instead of "0", the issue disappeared.

       for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0U; priv->dirty_tx[q]++) { --- [2]
                                                   ~~
- However, today I got a report from our team member locally. The object code of
  the get_num_desc() is just "from - subtract".

- If we use u32 as the original code, I realized the object codes between [1] and [2]
  are the same.
- Also, I realized just priv->cur_tx[q] - priv->dirty_tx[q] is no problem.
- So, I would like to recall this patch series.

Best regards,
Yoshihiro Shimoda