drivers/net/ethernet/dlink/dl2k.c | 49 ++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 11 deletions(-)
Add error handling by checking `dma_mapping_error()` and cleaning up
the `skb` using the appropriate `dev_kfree_skb*()` variant.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
Tested-on: D-Link DGE-550T Rev-A3
---
drivers/net/ethernet/dlink/dl2k.c | 49 ++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
index 1996d2e4e3e2..a821c9921745 100644
--- a/drivers/net/ethernet/dlink/dl2k.c
+++ b/drivers/net/ethernet/dlink/dl2k.c
@@ -508,6 +508,7 @@ static int alloc_list(struct net_device *dev)
for (i = 0; i < RX_RING_SIZE; i++) {
/* Allocated fixed size of skbuff */
struct sk_buff *skb;
+ dma_addr_t addr;
skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
np->rx_skbuff[i] = skb;
@@ -516,13 +517,19 @@ static int alloc_list(struct net_device *dev)
return -ENOMEM;
}
+ addr = dma_map_single(&np->pdev->dev, skb->data,
+ np->rx_buf_sz, DMA_FROM_DEVICE);
+ if (dma_mapping_error(&np->pdev->dev, addr)) {
+ dev_kfree_skb(skb);
+ np->rx_skbuff[i] = NULL;
+ free_list(dev);
+ return -ENOMEM;
+ }
np->rx_ring[i].next_desc = cpu_to_le64(np->rx_ring_dma +
((i + 1) % RX_RING_SIZE) *
sizeof(struct netdev_desc));
/* Rubicon now supports 40 bits of addressing space. */
- np->rx_ring[i].fraginfo =
- cpu_to_le64(dma_map_single(&np->pdev->dev, skb->data,
- np->rx_buf_sz, DMA_FROM_DEVICE));
+ np->rx_ring[i].fraginfo = cpu_to_le64(addr);
np->rx_ring[i].fraginfo |= cpu_to_le64((u64)np->rx_buf_sz << 48);
}
@@ -674,6 +681,7 @@ rio_timer (struct timer_list *t)
/* Re-allocate skbuffs to fill the descriptor ring */
for (; np->cur_rx - np->old_rx > 0; np->old_rx++) {
struct sk_buff *skb;
+ dma_addr_t addr;
entry = np->old_rx % RX_RING_SIZE;
/* Dropped packets don't need to re-allocate */
if (np->rx_skbuff[entry] == NULL) {
@@ -686,10 +694,16 @@ rio_timer (struct timer_list *t)
dev->name, entry);
break;
}
+ addr = dma_map_single(&np->pdev->dev, skb->data,
+ np->rx_buf_sz,
+ DMA_FROM_DEVICE);
+ if (dma_mapping_error(&np->pdev->dev, addr)) {
+ dev_kfree_skb_irq(skb);
+ np->rx_ring[entry].fraginfo = 0;
+ break;
+ }
np->rx_skbuff[entry] = skb;
- np->rx_ring[entry].fraginfo =
- cpu_to_le64 (dma_map_single(&np->pdev->dev, skb->data,
- np->rx_buf_sz, DMA_FROM_DEVICE));
+ np->rx_ring[entry].fraginfo = cpu_to_le64(addr);
}
np->rx_ring[entry].fraginfo |=
cpu_to_le64((u64)np->rx_buf_sz << 48);
@@ -720,6 +734,7 @@ start_xmit (struct sk_buff *skb, struct net_device *dev)
struct netdev_private *np = netdev_priv(dev);
void __iomem *ioaddr = np->ioaddr;
struct netdev_desc *txdesc;
+ dma_addr_t addr;
unsigned entry;
u64 tfc_vlan_tag = 0;
@@ -743,8 +758,14 @@ start_xmit (struct sk_buff *skb, struct net_device *dev)
((u64)np->vlan << 32) |
((u64)skb->priority << 45);
}
- txdesc->fraginfo = cpu_to_le64 (dma_map_single(&np->pdev->dev, skb->data,
- skb->len, DMA_TO_DEVICE));
+ addr = dma_map_single(&np->pdev->dev, skb->data, skb->len,
+ DMA_TO_DEVICE);
+ if (dma_mapping_error(&np->pdev->dev, addr)) {
+ dev_kfree_skb_any(skb);
+ np->tx_skbuff[entry] = NULL;
+ return NETDEV_TX_OK;
+ }
+ txdesc->fraginfo = cpu_to_le64(addr);
txdesc->fraginfo |= cpu_to_le64((u64)skb->len << 48);
/* DL2K bug: DMA fails to get next descriptor ptr in 10Mbps mode
@@ -1007,6 +1028,7 @@ receive_packet (struct net_device *dev)
entry = np->old_rx;
while (entry != np->cur_rx) {
struct sk_buff *skb;
+ dma_addr_t addr;
/* Dropped packets don't need to re-allocate */
if (np->rx_skbuff[entry] == NULL) {
skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
@@ -1018,10 +1040,15 @@ receive_packet (struct net_device *dev)
dev->name, entry);
break;
}
+ addr = dma_map_single(&np->pdev->dev, skb->data,
+ np->rx_buf_sz, DMA_FROM_DEVICE);
+ if (dma_mapping_error(&np->pdev->dev, addr)) {
+ dev_kfree_skb_irq(skb);
+ np->rx_ring[entry].fraginfo = 0;
+ break;
+ }
np->rx_skbuff[entry] = skb;
- np->rx_ring[entry].fraginfo =
- cpu_to_le64(dma_map_single(&np->pdev->dev, skb->data,
- np->rx_buf_sz, DMA_FROM_DEVICE));
+ np->rx_ring[entry].fraginfo = cpu_to_le64(addr);
}
np->rx_ring[entry].fraginfo |=
cpu_to_le64((u64)np->rx_buf_sz << 48);
--
2.51.0On Fri, Oct 03, 2025 at 12:26:38AM +0900, Yeounsu Moon wrote:
> Add error handling by checking `dma_mapping_error()` and cleaning up
> the `skb` using the appropriate `dev_kfree_skb*()` variant.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
> Tested-on: D-Link DGE-550T Rev-A3
> ---
> drivers/net/ethernet/dlink/dl2k.c | 49 ++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
> index 1996d2e4e3e2..a821c9921745 100644
> --- a/drivers/net/ethernet/dlink/dl2k.c
> +++ b/drivers/net/ethernet/dlink/dl2k.c
> @@ -508,6 +508,7 @@ static int alloc_list(struct net_device *dev)
> for (i = 0; i < RX_RING_SIZE; i++) {
> /* Allocated fixed size of skbuff */
> struct sk_buff *skb;
> + dma_addr_t addr;
>
> skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
> np->rx_skbuff[i] = skb;
> @@ -516,13 +517,19 @@ static int alloc_list(struct net_device *dev)
> return -ENOMEM;
> }
>
> + addr = dma_map_single(&np->pdev->dev, skb->data,
> + np->rx_buf_sz, DMA_FROM_DEVICE);
> + if (dma_mapping_error(&np->pdev->dev, addr)) {
> + dev_kfree_skb(skb);
> + np->rx_skbuff[i] = NULL;
> + free_list(dev);
> + return -ENOMEM;
> + }
> np->rx_ring[i].next_desc = cpu_to_le64(np->rx_ring_dma +
> ((i + 1) % RX_RING_SIZE) *
> sizeof(struct netdev_desc));
> /* Rubicon now supports 40 bits of addressing space. */
> - np->rx_ring[i].fraginfo =
> - cpu_to_le64(dma_map_single(&np->pdev->dev, skb->data,
> - np->rx_buf_sz, DMA_FROM_DEVICE));
> + np->rx_ring[i].fraginfo = cpu_to_le64(addr);
> np->rx_ring[i].fraginfo |= cpu_to_le64((u64)np->rx_buf_sz << 48);
> }
>
Thanks.
I agree that this fixes a problem.
And I agree that these problems were introduced by the cited commit,
at a time when pci_map_single() was used instead of dma_map_single().
But I wonder if it would be slightly nicer to make this change by
using the idiomatic pattern of an unwind ladder of goto labels.
In the case of alloc_list(), something like the following.
(Compile tested only!)
diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
index 1996d2e4e3e2..ba81373bbca8 100644
--- a/drivers/net/ethernet/dlink/dl2k.c
+++ b/drivers/net/ethernet/dlink/dl2k.c
@@ -508,25 +508,36 @@ static int alloc_list(struct net_device *dev)
for (i = 0; i < RX_RING_SIZE; i++) {
/* Allocated fixed size of skbuff */
struct sk_buff *skb;
+ dma_addr_t addr;
skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
np->rx_skbuff[i] = skb;
- if (!skb) {
- free_list(dev);
- return -ENOMEM;
- }
+ if (!skb)
+ goto err_free_list;
+
+ addr = dma_map_single(&np->pdev->dev, skb->data,
+ np->rx_buf_sz, DMA_FROM_DEVICE);
+ if (dma_mapping_error(&np->pdev->dev, addr))
+ goto err_kfree_skb;
np->rx_ring[i].next_desc = cpu_to_le64(np->rx_ring_dma +
((i + 1) % RX_RING_SIZE) *
sizeof(struct netdev_desc));
/* Rubicon now supports 40 bits of addressing space. */
- np->rx_ring[i].fraginfo =
- cpu_to_le64(dma_map_single(&np->pdev->dev, skb->data,
- np->rx_buf_sz, DMA_FROM_DEVICE));
+ np->rx_ring[i].fraginfo = cpu_to_le64(addr);
np->rx_ring[i].fraginfo |= cpu_to_le64((u64)np->rx_buf_sz << 48);
}
return 0;
+
+err_kfree_skb:
+ dev_kfree_skb(np->rx_skbuff[i]);
+ np->rx_skbuff[i] = NULL;
+err_free_list:
+ free_list(dev);
+
+ return -ENOMEM;
+
}
static void rio_hw_init(struct net_device *dev)
Hello Simon.
I'm currenly re-writing the code as you suggested. I think `alloc_list()`
can easily adopt the `goto` pattern, but for others functions, it's not
that straightforward.
My question is whether a style combining `goto`, `continue`, and `break`
would be acceptable in this context:
```c
if (np->cur_rx - np->old_rx >= RX_RING_SIZE) {
printk(KERN_INFO "Try to recover rx ring exhausted...\n");
/* Re-allocate skbuffs to fill the descriptor ring */
for (; np->cur_rx - np->old_rx > 0; np->old_rx++) {
struct sk_buff *skb;
dma_addr_t addr;
entry = np->old_rx % RX_RING_SIZE;
/* Dropped packets don't need to re-allocate */
if (np->rx_skbuff[entry])
goto fill_entry;
skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
if (skb == NULL)
goto out_clear_fraginfo;
addr = dma_map_single(&np->pdev->dev, skb->data,
np->rx_buf_sz,
DMA_FROM_DEVICE);
if (dma_mapping_error(&np->pdev->dev, addr))
goto out_kfree_skb;
np->rx_skbuff[entry] = skb;
np->rx_ring[entry].fraginfo = cpu_to_le64(addr);
fill_entry:
np->rx_ring[entry].fraginfo |=
cpu_to_le64((u64)np->rx_buf_sz << 48);
np->rx_ring[entry].status = 0;
continue;
out_kfree_skb:
dev_kfree_skb_irq(skb);
out_clear_fraginfo:
np->rx_ring[entry].fraginfo = 0;
printk(KERN_INFO
"%s: Still unable to re-allocate Rx skbuff.#%d\n"
, dev->name, entry);
break;
} /* end for */
} /* end if */
spin_unlock_irqrestore (&np->rx_lock, flags);
np->timer.expires = jiffies + next_tick;
add_timer(&np->timer);
}
```
Or is there any better way to handle errors here?
I'd appreciate your guidance.
On Sun, Oct 05, 2025 at 02:22:43PM +0900, Yeounsu Moon wrote:
> Hello Simon.
>
> I'm currenly re-writing the code as you suggested. I think `alloc_list()`
> can easily adopt the `goto` pattern, but for others functions, it's not
> that straightforward.
>
> My question is whether a style combining `goto`, `continue`, and `break`
> would be acceptable in this context:
>
> ```c
> if (np->cur_rx - np->old_rx >= RX_RING_SIZE) {
> printk(KERN_INFO "Try to recover rx ring exhausted...\n");
> /* Re-allocate skbuffs to fill the descriptor ring */
> for (; np->cur_rx - np->old_rx > 0; np->old_rx++) {
> struct sk_buff *skb;
> dma_addr_t addr;
> entry = np->old_rx % RX_RING_SIZE;
> /* Dropped packets don't need to re-allocate */
> if (np->rx_skbuff[entry])
> goto fill_entry;
>
> skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
> if (skb == NULL)
> goto out_clear_fraginfo;
>
> addr = dma_map_single(&np->pdev->dev, skb->data,
> np->rx_buf_sz,
> DMA_FROM_DEVICE);
> if (dma_mapping_error(&np->pdev->dev, addr))
> goto out_kfree_skb;
>
> np->rx_skbuff[entry] = skb;
> np->rx_ring[entry].fraginfo = cpu_to_le64(addr);
> fill_entry:
> np->rx_ring[entry].fraginfo |=
> cpu_to_le64((u64)np->rx_buf_sz << 48);
> np->rx_ring[entry].status = 0;
> continue;
>
> out_kfree_skb:
> dev_kfree_skb_irq(skb);
> out_clear_fraginfo:
> np->rx_ring[entry].fraginfo = 0;
> printk(KERN_INFO
> "%s: Still unable to re-allocate Rx skbuff.#%d\n"
> , dev->name, entry);
> break;
> } /* end for */
> } /* end if */
> spin_unlock_irqrestore (&np->rx_lock, flags);
> np->timer.expires = jiffies + next_tick;
> add_timer(&np->timer);
> }
> ```
>
> Or is there any better way to handle errors here?
> I'd appreciate your guidance.
Sorry for the slow response, I've been ill for the past few days.
I did also consider the option above. That is handling the
errors in the loop. And I can see some merit in that approach,
e.g. reduced scope of variables.
But I think the more idiomatic approach is to handle them 'here'.
That is, at the end of the function. So I would lean towards
that option.
> Sorry for the slow response, I've been ill for the past few days. No worries. I hope you're feeling better now. > > I did also consider the option above. That is handling the > errors in the loop. And I can see some merit in that approach, > e.g. reduced scope of variables. > > But I think the more idiomatic approach is to handle them 'here'. > That is, at the end of the function. So I would lean towards > that option. Not only with `goto`, but there are also issues such as replacing `printk()` with `netdev_info()`. So for now, I'll send the patch that fix `alloc_list()` correctly.
On Fri Oct 3, 2025 at 6:44 PM KST, Simon Horman wrote:
> On Fri, Oct 03, 2025 at 12:26:38AM +0900, Yeounsu Moon wrote:
>> Add error handling by checking `dma_mapping_error()` and cleaning up
>> the `skb` using the appropriate `dev_kfree_skb*()` variant.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Yeounsu Moon <yyyynoom@gmail.com>
>> Tested-on: D-Link DGE-550T Rev-A3
>> ---
>> drivers/net/ethernet/dlink/dl2k.c | 49 ++++++++++++++++++++++++-------
>> 1 file changed, 38 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
>> index 1996d2e4e3e2..a821c9921745 100644
>> --- a/drivers/net/ethernet/dlink/dl2k.c
>> +++ b/drivers/net/ethernet/dlink/dl2k.c
>> @@ -508,6 +508,7 @@ static int alloc_list(struct net_device *dev)
>> for (i = 0; i < RX_RING_SIZE; i++) {
>> /* Allocated fixed size of skbuff */
>> struct sk_buff *skb;
>> + dma_addr_t addr;
>>
>> skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
>> np->rx_skbuff[i] = skb;
>> @@ -516,13 +517,19 @@ static int alloc_list(struct net_device *dev)
>> return -ENOMEM;
>> }
>>
>> + addr = dma_map_single(&np->pdev->dev, skb->data,
>> + np->rx_buf_sz, DMA_FROM_DEVICE);
>> + if (dma_mapping_error(&np->pdev->dev, addr)) {
>> + dev_kfree_skb(skb);
>> + np->rx_skbuff[i] = NULL;
>> + free_list(dev);
>> + return -ENOMEM;
>> + }
>> np->rx_ring[i].next_desc = cpu_to_le64(np->rx_ring_dma +
>> ((i + 1) % RX_RING_SIZE) *
>> sizeof(struct netdev_desc));
>> /* Rubicon now supports 40 bits of addressing space. */
>> - np->rx_ring[i].fraginfo =
>> - cpu_to_le64(dma_map_single(&np->pdev->dev, skb->data,
>> - np->rx_buf_sz, DMA_FROM_DEVICE));
>> + np->rx_ring[i].fraginfo = cpu_to_le64(addr);
>> np->rx_ring[i].fraginfo |= cpu_to_le64((u64)np->rx_buf_sz << 48);
>> }
>>
>
> Thanks.
>
> I agree that this fixes a problem.
> And I agree that these problems were introduced by the cited commit,
> at a time when pci_map_single() was used instead of dma_map_single().
Thank you for pointing that out. I spent some time making sure that the
`Fixes:` tag you mentioned was applied correctly.
>
> But I wonder if it would be slightly nicer to make this change by
> using the idiomatic pattern of an unwind ladder of goto labels.
I agree that your approach is much cleaner. However, I structured the
code as intuitively and simply as possible, since making it more complex
could make the review harder. I'm curious about the opinions of other
reviewers and the maintainer. If an immediate change is required, I can
submit a v2; otherwise; I'll first post the fix patch to `net` and incorporate
your suggestion in `net-next`.
>
> In the case of alloc_list(), something like the following.
> (Compile tested only!)
>
> diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
> index 1996d2e4e3e2..ba81373bbca8 100644
> --- a/drivers/net/ethernet/dlink/dl2k.c
> +++ b/drivers/net/ethernet/dlink/dl2k.c
> @@ -508,25 +508,36 @@ static int alloc_list(struct net_device *dev)
> for (i = 0; i < RX_RING_SIZE; i++) {
> /* Allocated fixed size of skbuff */
> struct sk_buff *skb;
> + dma_addr_t addr;
>
> skb = netdev_alloc_skb_ip_align(dev, np->rx_buf_sz);
> np->rx_skbuff[i] = skb;
> - if (!skb) {
> - free_list(dev);
> - return -ENOMEM;
> - }
> + if (!skb)
> + goto err_free_list;
> +
> + addr = dma_map_single(&np->pdev->dev, skb->data,
> + np->rx_buf_sz, DMA_FROM_DEVICE);
> + if (dma_mapping_error(&np->pdev->dev, addr))
> + goto err_kfree_skb;
>
> np->rx_ring[i].next_desc = cpu_to_le64(np->rx_ring_dma +
> ((i + 1) % RX_RING_SIZE) *
> sizeof(struct netdev_desc));
> /* Rubicon now supports 40 bits of addressing space. */
> - np->rx_ring[i].fraginfo =
> - cpu_to_le64(dma_map_single(&np->pdev->dev, skb->data,
> - np->rx_buf_sz, DMA_FROM_DEVICE));
> + np->rx_ring[i].fraginfo = cpu_to_le64(addr);
> np->rx_ring[i].fraginfo |= cpu_to_le64((u64)np->rx_buf_sz << 48);
> }
>
> return 0;
> +
> +err_kfree_skb:
> + dev_kfree_skb(np->rx_skbuff[i]);
> + np->rx_skbuff[i] = NULL;
> +err_free_list:
> + free_list(dev);
> +
> + return -ENOMEM;
> +
> }
>
> static void rio_hw_init(struct net_device *dev)
© 2016 - 2026 Red Hat, Inc.