[RFC PATCH 0/5] staging: vt6655: Implement allocation failure handling

[RFC PATCH 0/5] staging: vt6655: Implement allocation failure handling

[Nam Cao]

This driver does not handle allocation failure when receiving data very well.
This patchset implements better handling in the case of allocation failure.

Nam Cao (5):
  staging: vt6655: remove redundant if condition
  staging: vt6655: change vnt_receive_frame return type to void
  staging: vt6655: split device_alloc_rx_buf
  staging: vt6655: change device_alloc_rx_buf's argument
  staging: vt6655: implement allocation failure handling

 drivers/staging/vt6655/device_main.c | 41 ++++++++++++++++++----------
 drivers/staging/vt6655/dpc.c         |  8 +++---
 drivers/staging/vt6655/dpc.h         |  2 +-
 3 files changed, 31 insertions(+), 20 deletions(-)

-- 
2.25.1

[RFC PATCH 1/5] staging: vt6655: remove redundant if condition

[Nam Cao]

The function vnt_receive_frame always returns true, so checking its
return value is redundant. Remove it.

Signed-off-by: Nam Cao <namcaov@gmail.com>
---
 drivers/staging/vt6655/device_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index 04d737012cef..79325a693857 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -832,12 +832,12 @@ static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
 		if (!rd->rd_info->skb)
 			break;
 
-		if (vnt_receive_frame(priv, rd)) {
-			if (!device_alloc_rx_buf(priv, rd)) {
-				dev_err(&priv->pcid->dev,
-					"can not allocate rx buf\n");
-				break;
-			}
+		vnt_receive_frame(priv, rd);
+
+		if (!device_alloc_rx_buf(priv, rd)) {
+			dev_err(&priv->pcid->dev,
+				"can not allocate rx buf\n");
+			break;
 		}
 		rd->rd0.owner = OWNED_BY_NIC;
 	}
-- 
2.25.1

[RFC PATCH 2/5] staging: vt6655: change vnt_receive_frame return type to void

[Nam Cao]

The function vnt_receive_frame's returned value is not used anywhere.
Furthermore, it always return true. Change its return type to void.

Signed-off-by: Nam Cao <namcaov@gmail.com>
---
 drivers/staging/vt6655/dpc.c | 8 ++++----
 drivers/staging/vt6655/dpc.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vt6655/dpc.c b/drivers/staging/vt6655/dpc.c
index c6ed3537f439..9f2128f5d3c3 100644
--- a/drivers/staging/vt6655/dpc.c
+++ b/drivers/staging/vt6655/dpc.c
@@ -115,7 +115,7 @@ static bool vnt_rx_data(struct vnt_private *priv, struct sk_buff *skb,
 	return true;
 }
 
-bool vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd)
+void vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd)
 {
 	struct vnt_rd_info *rd_info = curr_rd->rd_info;
 	struct sk_buff *skb;
@@ -133,13 +133,13 @@ bool vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd)
 		/* Frame Size error drop this packet.*/
 		dev_dbg(&priv->pcid->dev, "Wrong frame size %d\n", frame_size);
 		dev_kfree_skb_irq(skb);
-		return true;
+		return;
 	}
 
 	if (vnt_rx_data(priv, skb, frame_size))
-		return true;
+		return;
 
 	dev_kfree_skb_irq(skb);
 
-	return true;
+	return;
 }
diff --git a/drivers/staging/vt6655/dpc.h b/drivers/staging/vt6655/dpc.h
index 40364c0ab7f6..f67c1ef23171 100644
--- a/drivers/staging/vt6655/dpc.h
+++ b/drivers/staging/vt6655/dpc.h
@@ -16,6 +16,6 @@
 
 #include "device.h"
 
-bool vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd);
+void vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd);
 
 #endif /* __RXTX_H__ */
-- 
2.25.1

[RFC PATCH 3/5] staging: vt6655: split device_alloc_rx_buf

[Nam Cao]

The function device_alloc_rx_buf does 2 things: allocating rx buffer
and initializing the rx descriptor's values. Split this function into
two, with each does one job.

This split is preparation for implementing correct out-of-memory error
handling.

Signed-off-by: Nam Cao <namcaov@gmail.com>
---
 drivers/staging/vt6655/device_main.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index 79325a693857..27fe28156257 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -133,6 +133,7 @@ static int device_init_td1_ring(struct vnt_private *priv);
 static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
 static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
 static bool device_alloc_rx_buf(struct vnt_private *, struct vnt_rx_desc *);
+static void device_init_rx_desc(struct vnt_private *priv, struct vnt_rx_desc *rd);
 static void device_free_rx_buf(struct vnt_private *priv,
 			       struct vnt_rx_desc *rd);
 static void device_init_registers(struct vnt_private *priv);
@@ -615,6 +616,8 @@ static int device_init_rd0_ring(struct vnt_private *priv)
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 			ret = -ENOMEM;
 			goto err_free_rd;
+		} else {
+			device_init_rx_desc(priv, desc);
 		}
 
 		desc->next = &priv->aRD0Ring[(i + 1) % priv->opts.rx_descs0];
@@ -661,6 +664,8 @@ static int device_init_rd1_ring(struct vnt_private *priv)
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 			ret = -ENOMEM;
 			goto err_free_rd;
+		} else {
+			device_init_rx_desc(priv, desc);
 		}
 
 		desc->next = &priv->aRD1Ring[(i + 1) % priv->opts.rx_descs1];
@@ -838,7 +843,10 @@ static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
 			dev_err(&priv->pcid->dev,
 				"can not allocate rx buf\n");
 			break;
+		} else {
+			device_init_rx_desc(priv, rd);
 		}
+
 		rd->rd0.owner = OWNED_BY_NIC;
 	}
 
@@ -865,15 +873,17 @@ static bool device_alloc_rx_buf(struct vnt_private *priv,
 		rd_info->skb = NULL;
 		return false;
 	}
+	return true;
+}
 
+static void device_init_rx_desc(struct vnt_private *priv, struct vnt_rx_desc *rd)
+{
 	*((unsigned int *)&rd->rd0) = 0; /* FIX cast */
 
 	rd->rd0.res_count = cpu_to_le16(priv->rx_buf_sz);
 	rd->rd0.owner = OWNED_BY_NIC;
 	rd->rd1.req_count = cpu_to_le16(priv->rx_buf_sz);
-	rd->buff_addr = cpu_to_le32(rd_info->skb_dma);
-
-	return true;
+	rd->buff_addr = cpu_to_le32(rd->rd_info->skb_dma);
 }
 
 static void device_free_rx_buf(struct vnt_private *priv,
-- 
2.25.1

[RFC PATCH 4/5] staging: vt6655: change device_alloc_rx_buf's argument

[Nam Cao]

The function device_alloc_rx_buf takes struct vnt_rx_desc as 1 argument,
but only accesses the member rd_info of this struct. Change this
argument to struct vnt_rd_info instead.

This is preparation to implement correct out-of-memory error handling.

Signed-off-by: Nam Cao <namcaov@gmail.com>
---
 drivers/staging/vt6655/device_main.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index 27fe28156257..ca6c4266f010 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -132,7 +132,7 @@ static int device_init_td1_ring(struct vnt_private *priv);
 
 static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
 static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
-static bool device_alloc_rx_buf(struct vnt_private *, struct vnt_rx_desc *);
+static bool device_alloc_rx_buf(struct vnt_private *, struct vnt_rd_info *);
 static void device_init_rx_desc(struct vnt_private *priv, struct vnt_rx_desc *rd);
 static void device_free_rx_buf(struct vnt_private *priv,
 			       struct vnt_rx_desc *rd);
@@ -612,7 +612,7 @@ static int device_init_rd0_ring(struct vnt_private *priv)
 			goto err_free_desc;
 		}
 
-		if (!device_alloc_rx_buf(priv, desc)) {
+		if (!device_alloc_rx_buf(priv, desc->rd_info)) {
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 			ret = -ENOMEM;
 			goto err_free_rd;
@@ -660,7 +660,7 @@ static int device_init_rd1_ring(struct vnt_private *priv)
 			goto err_free_desc;
 		}
 
-		if (!device_alloc_rx_buf(priv, desc)) {
+		if (!device_alloc_rx_buf(priv, desc->rd_info)) {
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 			ret = -ENOMEM;
 			goto err_free_rd;
@@ -839,7 +839,7 @@ static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
 
 		vnt_receive_frame(priv, rd);
 
-		if (!device_alloc_rx_buf(priv, rd)) {
+		if (!device_alloc_rx_buf(priv, rd->rd_info)) {
 			dev_err(&priv->pcid->dev,
 				"can not allocate rx buf\n");
 			break;
@@ -856,10 +856,8 @@ static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
 }
 
 static bool device_alloc_rx_buf(struct vnt_private *priv,
-				struct vnt_rx_desc *rd)
+				struct vnt_rd_info *rd_info)
 {
-	struct vnt_rd_info *rd_info = rd->rd_info;
-
 	rd_info->skb = dev_alloc_skb((int)priv->rx_buf_sz);
 	if (!rd_info->skb)
 		return false;
-- 
2.25.1

[RFC PATCH 5/5] staging: vt6655: implement allocation failure handling

[Nam Cao]

The function device_rx_srv does not handle allocation failure very well.
Currently, it performs these steps:
        - Unmap DMA buffer and hand over the buffer to mac80211
        - Allocate and dma-map new buffer
        - If allocation fails, abort

The problem is that, it aborts while still marking the buffer as
OWNED_BY_HOST. So when this function is called again in the future, it
incorrectly perceives the same buffer as valid and dma-unmap and hand
over this buffer to mac80211 again.

Re-implement this function to do things in a different order:
        - Allocate and dma-map new buffer
        - If allocation fails, abort and give up the ownership of the
          buffer (so that the device can re-use this buffer)
        - If allocation does not fail: unmap dma buffer and hand over
          the buffer to mac80211

Thus, when the driver cannot allocate new buffer, it simply discards the
received data and re-use the current buffer.

Signed-off-by: Nam Cao <namcaov@gmail.com>
---
 drivers/staging/vt6655/device_main.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index ca6c4266f010..8ae4ecca2ee3 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -826,6 +826,7 @@ static void device_free_td1_ring(struct vnt_private *priv)
 static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
 {
 	struct vnt_rx_desc *rd;
+	struct vnt_rd_info new_info;
 	int works = 0;
 
 	for (rd = priv->pCurrRD[idx];
@@ -837,16 +838,18 @@ static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
 		if (!rd->rd_info->skb)
 			break;
 
-		vnt_receive_frame(priv, rd);
-
-		if (!device_alloc_rx_buf(priv, rd->rd_info)) {
+		if (!device_alloc_rx_buf(priv, &new_info)) {
 			dev_err(&priv->pcid->dev,
 				"can not allocate rx buf\n");
+			rd->rd0.owner = OWNED_BY_NIC;
 			break;
-		} else {
-			device_init_rx_desc(priv, rd);
 		}
 
+		vnt_receive_frame(priv, rd);
+
+		memcpy(rd->rd_info, &new_info, sizeof(new_info));
+		device_init_rx_desc(priv, rd);
+
 		rd->rd0.owner = OWNED_BY_NIC;
 	}
 
-- 
2.25.1

Re: [RFC PATCH 0/5] staging: vt6655: Implement allocation failure handling

[Philipp Hortmann]

On 9/15/22 22:29, Nam Cao wrote:
> This driver does not handle allocation failure when receiving data very well.
> This patchset implements better handling in the case of allocation failure.
> 
> Nam Cao (5):
>    staging: vt6655: remove redundant if condition
>    staging: vt6655: change vnt_receive_frame return type to void
>    staging: vt6655: split device_alloc_rx_buf
>    staging: vt6655: change device_alloc_rx_buf's argument
>    staging: vt6655: implement allocation failure handling
> 
>   drivers/staging/vt6655/device_main.c | 41 ++++++++++++++++++----------
>   drivers/staging/vt6655/dpc.c         |  8 +++---
>   drivers/staging/vt6655/dpc.h         |  2 +-
>   3 files changed, 31 insertions(+), 20 deletions(-)
> 

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>


Find in this email a comment from Greg about RFC:
https://lore.kernel.org/linux-gpio/YwS4WDekXM3UQ7Yo@kroah.com/
This patch is marked as "RFC" but I don't see any questions that you
have here.  Please resolve anything you think needs to be handled and
submit a "this series is ok to be merged" version.

May be this is applicable to this patch as well.

Re: [RFC PATCH 0/5] staging: vt6655: Implement allocation failure handling

[Nam Cao]

On Thu, Sep 15, 2022 at 10:58 PM Philipp Hortmann
<philipp.g.hortmann@gmail.com> wrote:
>
> On 9/15/22 22:29, Nam Cao wrote:
> > This driver does not handle allocation failure when receiving data very well.
> > This patchset implements better handling in the case of allocation failure.
> >
> > Nam Cao (5):
> >    staging: vt6655: remove redundant if condition
> >    staging: vt6655: change vnt_receive_frame return type to void
> >    staging: vt6655: split device_alloc_rx_buf
> >    staging: vt6655: change device_alloc_rx_buf's argument
> >    staging: vt6655: implement allocation failure handling
> >
> >   drivers/staging/vt6655/device_main.c | 41 ++++++++++++++++++----------
> >   drivers/staging/vt6655/dpc.c         |  8 +++---
> >   drivers/staging/vt6655/dpc.h         |  2 +-
> >   3 files changed, 31 insertions(+), 20 deletions(-)
> >
>
> Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>

Thank you, I really appreciate your help.

>
>
> Find in this email a comment from Greg about RFC:
> https://lore.kernel.org/linux-gpio/YwS4WDekXM3UQ7Yo@kroah.com/
> This patch is marked as "RFC" but I don't see any questions that you
> have here.  Please resolve anything you think needs to be handled and
> submit a "this series is ok to be merged" version.
>
> May be this is applicable to this patch as well.

I add the RFC tag to "tells maintainers should review your patch thoroughly,
and provide feedback. RFC is typically used when sending feature patches for
the first time, or anytime the patch is more than just a simple bug fix."
(from https://kernelnewbies.org/PatchTipsAndTricks). I was not aware that this
tag may be interpreted differently. I can send a new patchset if necessary.

Best regards,
Nam

Re: [RFC PATCH 0/5] staging: vt6655: Implement allocation failure handling

[Dan Carpenter]

On Fri, Sep 16, 2022 at 09:11:58AM +0200, Nam Cao wrote:
> > Find in this email a comment from Greg about RFC:
> > https://lore.kernel.org/linux-gpio/YwS4WDekXM3UQ7Yo@kroah.com/
> > This patch is marked as "RFC" but I don't see any questions that you
> > have here.  Please resolve anything you think needs to be handled and
> > submit a "this series is ok to be merged" version.
> >
> > May be this is applicable to this patch as well.
> 
> I add the RFC tag to "tells maintainers should review your patch thoroughly,
> and provide feedback. RFC is typically used when sending feature patches for
> the first time, or anytime the patch is more than just a simple bug fix."
> (from https://kernelnewbies.org/PatchTipsAndTricks). I was not aware that this
> tag may be interpreted differently. I can send a new patchset if necessary.

Clean up patches are much simpler than bug fixes.  No need for an RFC.

But this patch does too many things and Greg will not apply it.

regards,
dan carpenter

Re: [RFC PATCH 3/5] staging: vt6655: split device_alloc_rx_buf

[Dan Carpenter]

On Thu, Sep 15, 2022 at 10:29:34PM +0200, Nam Cao wrote:
> The function device_alloc_rx_buf does 2 things: allocating rx buffer
> and initializing the rx descriptor's values. Split this function into
> two, with each does one job.
> 
> This split is preparation for implementing correct out-of-memory error
> handling.
> 
> Signed-off-by: Nam Cao <namcaov@gmail.com>
> ---
>  drivers/staging/vt6655/device_main.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index 79325a693857..27fe28156257 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -133,6 +133,7 @@ static int device_init_td1_ring(struct vnt_private *priv);
>  static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
>  static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
>  static bool device_alloc_rx_buf(struct vnt_private *, struct vnt_rx_desc *);
> +static void device_init_rx_desc(struct vnt_private *priv, struct vnt_rx_desc *rd);
>  static void device_free_rx_buf(struct vnt_private *priv,
>  			       struct vnt_rx_desc *rd);
>  static void device_init_registers(struct vnt_private *priv);
> @@ -615,6 +616,8 @@ static int device_init_rd0_ring(struct vnt_private *priv)
>  			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
>  			ret = -ENOMEM;
>  			goto err_free_rd;
> +		} else {
> +			device_init_rx_desc(priv, desc);
>  		}

None of these else statements make sense.  It should be:

		ret = -ENOMEM;
		goto err_free_rd;
	}

	device_init_rx_desc(priv, desc);
	desc->next = &priv->aRD0Ring[(i + 1) % priv->opts.rx_descs0];

I haven't reviewed the patch totally.  I don't understand why it's doing
this here instead of at the end.  But then I don't understand why it
needs to be in a separate function at all.

This patch does not make sense.  The commit description says that this
is a "preparation" patch.  Maybe fold it in with patch 5?  The rule is
"one thing per patch" not "half a thing per patch".

regards,
dan carpenter

Re: [RFC PATCH 0/5] staging: vt6655: Implement allocation failure handling

[Dan Carpenter]

On Fri, Sep 16, 2022 at 10:38:19AM +0300, Dan Carpenter wrote:
> On Fri, Sep 16, 2022 at 09:11:58AM +0200, Nam Cao wrote:
> > > Find in this email a comment from Greg about RFC:
> > > https://lore.kernel.org/linux-gpio/YwS4WDekXM3UQ7Yo@kroah.com/
> > > This patch is marked as "RFC" but I don't see any questions that you
> > > have here.  Please resolve anything you think needs to be handled and
> > > submit a "this series is ok to be merged" version.
> > >
> > > May be this is applicable to this patch as well.
> > 
> > I add the RFC tag to "tells maintainers should review your patch thoroughly,
> > and provide feedback. RFC is typically used when sending feature patches for
> > the first time, or anytime the patch is more than just a simple bug fix."
> > (from https://kernelnewbies.org/PatchTipsAndTricks). I was not aware that this
> > tag may be interpreted differently. I can send a new patchset if necessary.
> 
> Clean up patches are much simpler than bug fixes.  No need for an RFC.
> 
> But this patch does too many things and Greg will not apply it.

Sorry, when I wrote this email I thought I was responding to a different
thread.

regards,
dan carpenter

Re: [RFC PATCH 2/5] staging: vt6655: change vnt_receive_frame return type to void

[Dan Carpenter]

On Thu, Sep 15, 2022 at 10:29:33PM +0200, Nam Cao wrote:
> -bool vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd)
> +void vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd)
>  {
>  	struct vnt_rd_info *rd_info = curr_rd->rd_info;
>  	struct sk_buff *skb;
> @@ -133,13 +133,13 @@ bool vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd)
>  		/* Frame Size error drop this packet.*/
>  		dev_dbg(&priv->pcid->dev, "Wrong frame size %d\n", frame_size);
>  		dev_kfree_skb_irq(skb);
> -		return true;
> +		return;
>  	}
>  
>  	if (vnt_rx_data(priv, skb, frame_size))
> -		return true;
> +		return;
>  
>  	dev_kfree_skb_irq(skb);
>  
> -	return true;
> +	return;

Just delete this last return (it's pointless now).

>  }

regards,
dan carpenter

Re: [RFC PATCH 5/5] staging: vt6655: implement allocation failure handling

[Dan Carpenter]

On Thu, Sep 15, 2022 at 10:29:36PM +0200, Nam Cao wrote:
> The function device_rx_srv does not handle allocation failure very well.
> Currently, it performs these steps:
>         - Unmap DMA buffer and hand over the buffer to mac80211

Does the unmapping happens in vnt_receive_frame();?

>         - Allocate and dma-map new buffer

Is the new buffer for the next frame?  So in your patch if we don't
have space for the next frame then we do not process the current frame?
(Rhetorical questions are a bad idea on development lists.  I genuinely
don't know the answers to these questions).

>         - If allocation fails, abort
> 
> The problem is that, it aborts while still marking the buffer as
> OWNED_BY_HOST. So when this function is called again in the future, it
> incorrectly perceives the same buffer as valid and dma-unmap and hand
> over this buffer to mac80211 again.

Where is it supposed to get marked as OWNED_BY_HOST?

> 
> Re-implement this function to do things in a different order:
>         - Allocate and dma-map new buffer
>         - If allocation fails, abort and give up the ownership of the
>           buffer (so that the device can re-use this buffer)
>         - If allocation does not fail: unmap dma buffer and hand over
>           the buffer to mac80211
> 
> Thus, when the driver cannot allocate new buffer, it simply discards the
> received data and re-use the current buffer.
> 
> Signed-off-by: Nam Cao <namcaov@gmail.com>
> ---
>  drivers/staging/vt6655/device_main.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index ca6c4266f010..8ae4ecca2ee3 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -826,6 +826,7 @@ static void device_free_td1_ring(struct vnt_private *priv)
>  static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
>  {
>  	struct vnt_rx_desc *rd;
> +	struct vnt_rd_info new_info;
>  	int works = 0;
>  
>  	for (rd = priv->pCurrRD[idx];
> @@ -837,16 +838,18 @@ static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
>  		if (!rd->rd_info->skb)
>  			break;
>  
> -		vnt_receive_frame(priv, rd);
> -
> -		if (!device_alloc_rx_buf(priv, rd->rd_info)) {
> +		if (!device_alloc_rx_buf(priv, &new_info)) {
>  			dev_err(&priv->pcid->dev,
>  				"can not allocate rx buf\n");
> +			rd->rd0.owner = OWNED_BY_NIC;
>  			break;
> -		} else {
> -			device_init_rx_desc(priv, rd);
>  		}
>  
> +		vnt_receive_frame(priv, rd);
> +
> +		memcpy(rd->rd_info, &new_info, sizeof(new_info));
> +		device_init_rx_desc(priv, rd);
> +
>  		rd->rd0.owner = OWNED_BY_NIC;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The device_init_rx_desc() function sets it to OWNED_BY_NIC so this line
can be deleted.

regards,
dan carpenter

Re: [RFC PATCH 2/5] staging: vt6655: change vnt_receive_frame return type to void

[Nam Cao]

On Mon, Sep 19, 2022 at 12:45:11PM +0300, Dan Carpenter wrote:
> On Thu, Sep 15, 2022 at 10:29:33PM +0200, Nam Cao wrote:
> > -bool vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd)
> > +void vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd)
> >  {
> >  	struct vnt_rd_info *rd_info = curr_rd->rd_info;
> >  	struct sk_buff *skb;
> > @@ -133,13 +133,13 @@ bool vnt_receive_frame(struct vnt_private *priv, struct vnt_rx_desc *curr_rd)
> >  		/* Frame Size error drop this packet.*/
> >  		dev_dbg(&priv->pcid->dev, "Wrong frame size %d\n", frame_size);
> >  		dev_kfree_skb_irq(skb);
> > -		return true;
> > +		return;
> >  	}
> >  
> >  	if (vnt_rx_data(priv, skb, frame_size))
> > -		return true;
> > +		return;
> >  
> >  	dev_kfree_skb_irq(skb);
> >  
> > -	return true;
> > +	return;
> 
> Just delete this last return (it's pointless now).

Will be changed, thanks.

Best regards,
Nam

Re: [RFC PATCH 3/5] staging: vt6655: split device_alloc_rx_buf

[Nam Cao]

On Mon, Sep 19, 2022 at 12:36:05PM +0300, Dan Carpenter wrote:
> On Thu, Sep 15, 2022 at 10:29:34PM +0200, Nam Cao wrote:
> > The function device_alloc_rx_buf does 2 things: allocating rx buffer
> > and initializing the rx descriptor's values. Split this function into
> > two, with each does one job.
> > 
> > This split is preparation for implementing correct out-of-memory error
> > handling.
> > 
> > Signed-off-by: Nam Cao <namcaov@gmail.com>
> > ---
> >  drivers/staging/vt6655/device_main.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> > index 79325a693857..27fe28156257 100644
> > --- a/drivers/staging/vt6655/device_main.c
> > +++ b/drivers/staging/vt6655/device_main.c
> > @@ -133,6 +133,7 @@ static int device_init_td1_ring(struct vnt_private *priv);
> >  static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
> >  static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
> >  static bool device_alloc_rx_buf(struct vnt_private *, struct vnt_rx_desc *);
> > +static void device_init_rx_desc(struct vnt_private *priv, struct vnt_rx_desc *rd);
> >  static void device_free_rx_buf(struct vnt_private *priv,
> >  			       struct vnt_rx_desc *rd);
> >  static void device_init_registers(struct vnt_private *priv);
> > @@ -615,6 +616,8 @@ static int device_init_rd0_ring(struct vnt_private *priv)
> >  			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
> >  			ret = -ENOMEM;
> >  			goto err_free_rd;
> > +		} else {
> > +			device_init_rx_desc(priv, desc);
> >  		}
> 
> None of these else statements make sense.  It should be:
> 
> 		ret = -ENOMEM;
> 		goto err_free_rd;
> 	}
> 
> 	device_init_rx_desc(priv, desc);
> 	desc->next = &priv->aRD0Ring[(i + 1) % priv->opts.rx_descs0];

That does look better, will be changed.

> I haven't reviewed the patch totally.  I don't understand why it's doing
> this here instead of at the end.  But then I don't understand why it
> needs to be in a separate function at all.
> 
> This patch does not make sense.  The commit description says that this
> is a "preparation" patch.  Maybe fold it in with patch 5?  The rule is
> "one thing per patch" not "half a thing per patch".

I thought splitting it like this would make it easier to review. But if
these preparation patches are not welcomed, I will squash them and
resend.

Thank you for spending time reviewing the patches.

Best regards,
Nam

Re: [RFC PATCH 5/5] staging: vt6655: implement allocation failure handling

[Nam Cao]

On Mon, Sep 19, 2022 at 01:12:22PM +0300, Dan Carpenter wrote:
> On Thu, Sep 15, 2022 at 10:29:36PM +0200, Nam Cao wrote:
> > The function device_rx_srv does not handle allocation failure very well.
> > Currently, it performs these steps:
> >         - Unmap DMA buffer and hand over the buffer to mac80211
> 
> Does the unmapping happens in vnt_receive_frame();?

Yes.

> 
> >         - Allocate and dma-map new buffer
> 
> Is the new buffer for the next frame?  So in your patch if we don't
> have space for the next frame then we do not process the current frame?
> (Rhetorical questions are a bad idea on development lists.  I genuinely
> don't know the answers to these questions).

Almost correct: if we don't have space for next frame, we _drop_ the
current frame. Note that this is also how it is implemented in similar
drivers, such as:
  - adm8211_interrupt_rci() in drivers/net/wireless/admtek/adm8211.c
  - rtl8180_handle_rx() in drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c

> 
> >         - If allocation fails, abort
> > 
> > The problem is that, it aborts while still marking the buffer as
> > OWNED_BY_HOST. So when this function is called again in the future, it
> > incorrectly perceives the same buffer as valid and dma-unmap and hand
> > over this buffer to mac80211 again.
> 
> Where is it supposed to get marked as OWNED_BY_HOST?

By the device/hardware.

The basic idea how this driver works is that: the cpu allocates buffers
and marks them as OWNED_BY_NIC, basically telling the device that "here
are some buffers that you can use". When there is new frame, the device
looks for buffer marked with OWNED_BY_NIC and write data in there; then
it marks the buffer as OWNED_BY_HOST, basically saying "there is some
valid data in this buffer, you should read it".

> 
> > 
> > Re-implement this function to do things in a different order:
> >         - Allocate and dma-map new buffer
> >         - If allocation fails, abort and give up the ownership of the
> >           buffer (so that the device can re-use this buffer)
> >         - If allocation does not fail: unmap dma buffer and hand over
> >           the buffer to mac80211
> > 
> > Thus, when the driver cannot allocate new buffer, it simply discards the
> > received data and re-use the current buffer.
> > 
> > Signed-off-by: Nam Cao <namcaov@gmail.com>
> > ---
> >  drivers/staging/vt6655/device_main.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> > index ca6c4266f010..8ae4ecca2ee3 100644
> > --- a/drivers/staging/vt6655/device_main.c
> > +++ b/drivers/staging/vt6655/device_main.c
> > @@ -826,6 +826,7 @@ static void device_free_td1_ring(struct vnt_private *priv)
> >  static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
> >  {
> >  	struct vnt_rx_desc *rd;
> > +	struct vnt_rd_info new_info;
> >  	int works = 0;
> >  
> >  	for (rd = priv->pCurrRD[idx];
> > @@ -837,16 +838,18 @@ static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
> >  		if (!rd->rd_info->skb)
> >  			break;
> >  
> > -		vnt_receive_frame(priv, rd);
> > -
> > -		if (!device_alloc_rx_buf(priv, rd->rd_info)) {
> > +		if (!device_alloc_rx_buf(priv, &new_info)) {
> >  			dev_err(&priv->pcid->dev,
> >  				"can not allocate rx buf\n");
> > +			rd->rd0.owner = OWNED_BY_NIC;
> >  			break;
> > -		} else {
> > -			device_init_rx_desc(priv, rd);
> >  		}
> >  
> > +		vnt_receive_frame(priv, rd);
> > +
> > +		memcpy(rd->rd_info, &new_info, sizeof(new_info));
> > +		device_init_rx_desc(priv, rd);
> > +
> >  		rd->rd0.owner = OWNED_BY_NIC;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> The device_init_rx_desc() function sets it to OWNED_BY_NIC so this line
> can be deleted.

Noted. Thanks.

Best regards,
Nam