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.1On 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
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
© 2016 - 2026 Red Hat, Inc.