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
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 <[email protected]>
---
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
The function vnt_receive_frame always returns true, so checking its
return value is redundant. Remove it.
Signed-off-by: Nam Cao <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Find in this email a comment from Greg about RFC:
https://lore.kernel.org/linux-gpio/[email protected]/
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.
On Thu, Sep 15, 2022 at 10:58 PM Philipp Hortmann
<[email protected]> 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 <[email protected]>
Thank you, I really appreciate your help.
>
>
> Find in this email a comment from Greg about RFC:
> https://lore.kernel.org/linux-gpio/[email protected]/
> 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
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/[email protected]/
> > 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
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 <[email protected]>
> ---
> 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
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
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/[email protected]/
> > > 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
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > ---
> > 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