* [PATCH] bgmac: don't update slot on skb alloc/dma mapping error
@ 2013-10-29 6:44 Nathan Hintz
2013-10-29 6:52 ` Rafał Miłecki
0 siblings, 1 reply; 6+ messages in thread
From: Nathan Hintz @ 2013-10-29 6:44 UTC (permalink / raw)
To: netdev; +Cc: zajec5
Don't update the slot in "bgmac_dma_rx_skb_for_slot" unless both the
skb alloc and dma mapping are successful; and free the newly allocated
skb if a dma mapping error occurs. This will prevent an skb leak upon
returning when an error occurs.
Signed-off-by: Nathan Hintz <nlhintz@hotmail.com>
---
drivers/net/ethernet/broadcom/bgmac.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 7eca5a1..11e5d8d 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -252,25 +252,33 @@ static int bgmac_dma_rx_skb_for_slot(struct bgmac *bgmac,
struct bgmac_slot_info *slot)
{
struct device *dma_dev = bgmac->core->dma_dev;
+ struct sk_buff *skb;
+ dma_addr_t dma_addr;
struct bgmac_rx_header *rx;
/* Alloc skb */
- slot->skb = netdev_alloc_skb(bgmac->net_dev, BGMAC_RX_BUF_SIZE);
- if (!slot->skb)
+ skb = netdev_alloc_skb(bgmac->net_dev, BGMAC_RX_BUF_SIZE);
+ if (!skb)
return -ENOMEM;
/* Poison - if everything goes fine, hardware will overwrite it */
- rx = (struct bgmac_rx_header *)slot->skb->data;
+ rx = (struct bgmac_rx_header *)skb->data;
rx->len = cpu_to_le16(0xdead);
rx->flags = cpu_to_le16(0xbeef);
/* Map skb for the DMA */
- slot->dma_addr = dma_map_single(dma_dev, slot->skb->data,
- BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
- if (dma_mapping_error(dma_dev, slot->dma_addr)) {
+ dma_addr = dma_map_single(dma_dev, skb->data,
+ BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
+ if (dma_mapping_error(dma_dev, dma_addr)) {
bgmac_err(bgmac, "DMA mapping error\n");
+ dev_kfree_skb(skb);
return -ENOMEM;
}
+
+ /* Update the slot */
+ slot->skb = skb;
+ slot->dma_addr = dma_addr;
+
if (slot->dma_addr & 0xC0000000)
bgmac_warn(bgmac, "DMA address using 0xC0000000 bit(s), it may need translation trick\n");
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bgmac: don't update slot on skb alloc/dma mapping error
2013-10-29 6:44 [PATCH] bgmac: don't update slot on skb alloc/dma mapping error Nathan Hintz
@ 2013-10-29 6:52 ` Rafał Miłecki
2013-10-29 8:22 ` Nathan Hintz
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2013-10-29 6:52 UTC (permalink / raw)
To: Nathan Hintz; +Cc: Network Development
2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
> Don't update the slot in "bgmac_dma_rx_skb_for_slot" unless both the
> skb alloc and dma mapping are successful; and free the newly allocated
> skb if a dma mapping error occurs. This will prevent an skb leak upon
> returning when an error occurs.
In case of bgmac_dma_rx_skb_for_slot failure we're giving up anyway
(and freeing everything), but with your patch code is simpler to
understand, so I'm OK with that.
Acked-by: Rafał Miłecki <zajec5@gmail.com>
--
Rafał
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bgmac: don't update slot on skb alloc/dma mapping error
2013-10-29 6:52 ` Rafał Miłecki
@ 2013-10-29 8:22 ` Nathan Hintz
2013-10-29 8:28 ` Rafał Miłecki
2013-10-29 19:39 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Nathan Hintz @ 2013-10-29 8:22 UTC (permalink / raw)
To: Rafał Miłecki; +Cc: Network Development
On Tue, 29 Oct 2013 07:52:58 +0100
Rafał Miłecki <zajec5@gmail.com> wrote:
> 2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
> > Don't update the slot in "bgmac_dma_rx_skb_for_slot" unless both the
> > skb alloc and dma mapping are successful; and free the newly allocated
> > skb if a dma mapping error occurs. This will prevent an skb leak upon
> > returning when an error occurs.
>
> In case of bgmac_dma_rx_skb_for_slot failure we're giving up anyway
> (and freeing everything), but with your patch code is simpler to
> understand, so I'm OK with that.
>
> Acked-by: Rafał Miłecki <zajec5@gmail.com>
>
I might be misunderstanding; but it in the case of failure, it appeared to me
that the currently received packet was dropped and the old skb would continue
to be assigned to the slot and would be used to receive future packets (this
would continue until bgmac_dma_rx_skb_for_slot was successful).
--
Nathan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bgmac: don't update slot on skb alloc/dma mapping error
2013-10-29 8:22 ` Nathan Hintz
@ 2013-10-29 8:28 ` Rafał Miłecki
2013-10-29 15:20 ` Nathan Hintz
2013-10-29 19:39 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2013-10-29 8:28 UTC (permalink / raw)
To: Nathan Hintz; +Cc: Network Development
2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
> On Tue, 29 Oct 2013 07:52:58 +0100
> Rafał Miłecki <zajec5@gmail.com> wrote:
>
>> 2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
>> > Don't update the slot in "bgmac_dma_rx_skb_for_slot" unless both the
>> > skb alloc and dma mapping are successful; and free the newly allocated
>> > skb if a dma mapping error occurs. This will prevent an skb leak upon
>> > returning when an error occurs.
>>
>> In case of bgmac_dma_rx_skb_for_slot failure we're giving up anyway
>> (and freeing everything), but with your patch code is simpler to
>> understand, so I'm OK with that.
>>
>> Acked-by: Rafał Miłecki <zajec5@gmail.com>
>>
>
> I might be misunderstanding; but it in the case of failure, it appeared to me
> that the currently received packet was dropped and the old skb would continue
> to be assigned to the slot and would be used to receive future packets (this
> would continue until bgmac_dma_rx_skb_for_slot was successful).
I was commenting on current usage (bgmac_dma_alloc), not my WIP patch
for bgmac_dma_rx_read :)
Your patch will be helpful for my bgmac_dma_rx_read rework.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bgmac: don't update slot on skb alloc/dma mapping error
2013-10-29 8:28 ` Rafał Miłecki
@ 2013-10-29 15:20 ` Nathan Hintz
0 siblings, 0 replies; 6+ messages in thread
From: Nathan Hintz @ 2013-10-29 15:20 UTC (permalink / raw)
To: Rafał Miłecki; +Cc: Network Development
On Tue, 29 Oct 2013 09:28:56 +0100
Rafał Miłecki <zajec5@gmail.com> wrote:
> 2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
> > On Tue, 29 Oct 2013 07:52:58 +0100
> > Rafał Miłecki <zajec5@gmail.com> wrote:
> >
> >> 2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
> >> > Don't update the slot in "bgmac_dma_rx_skb_for_slot" unless both the
> >> > skb alloc and dma mapping are successful; and free the newly allocated
> >> > skb if a dma mapping error occurs.
> >> > returning when an error occurs.
> >>
> >> In case of bgmac_dma_rx_skb_for_slot failure we're giving up anyway
> >> (and freeing everything), but with your patch code is simpler to
> >> understand, so I'm OK with that.
> >>
> >> Acked-by: Rafał Miłecki <zajec5@gmail.com>
> >>
> >
> > I might be misunderstanding; but it in the case of failure, it appeared to me
> > that the currently received packet was dropped and the old skb would continue
> > to be assigned to the slot and would be used to receive future packets (this
> > would continue until bgmac_dma_rx_skb_for_slot was successful).
>
> I was commenting on current usage (.), not my WIP patch
> for bgmac_dma_rx_read :)
>
> Your patch will be helpful for my bgmac_dma_rx_read rework.
>
You're right, I was commenting to you WIP. The commit message should probably
be changed to remove the statement "This will prevent an skb leak upon returning
when an error occurs", as this doesn't occur with the usage in bgmac_dma_alloc.
Unfortunately, I won't be able to send a revised patch until tonight.
Nathan
--
Nathan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bgmac: don't update slot on skb alloc/dma mapping error
2013-10-29 8:22 ` Nathan Hintz
2013-10-29 8:28 ` Rafał Miłecki
@ 2013-10-29 19:39 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2013-10-29 19:39 UTC (permalink / raw)
To: nlhintz; +Cc: zajec5, netdev
From: Nathan Hintz <nlhintz@hotmail.com>
Date: Tue, 29 Oct 2013 01:22:55 -0700
> On Tue, 29 Oct 2013 07:52:58 +0100
> Rafał Miłecki <zajec5@gmail.com> wrote:
>
>> 2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
>> > Don't update the slot in "bgmac_dma_rx_skb_for_slot" unless both the
>> > skb alloc and dma mapping are successful; and free the newly allocated
>> > skb if a dma mapping error occurs. This will prevent an skb leak upon
>> > returning when an error occurs.
>>
>> In case of bgmac_dma_rx_skb_for_slot failure we're giving up anyway
>> (and freeing everything), but with your patch code is simpler to
>> understand, so I'm OK with that.
>>
>> Acked-by: Rafał Miłecki <zajec5@gmail.com>
>>
>
> I might be misunderstanding; but it in the case of failure, it appeared to me
> that the currently received packet was dropped and the old skb would continue
> to be assigned to the slot and would be used to receive future packets (this
> would continue until bgmac_dma_rx_skb_for_slot was successful).
That's exactly the wanted, and most desirable, behavior for any network
driver.
If you can't allocate a new SKB, reuse the old one, because the worst
thing you can do is prioritize packet reception over making sure the
device doesn't end up with no RX slots to DMA into.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-29 19:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-29 6:44 [PATCH] bgmac: don't update slot on skb alloc/dma mapping error Nathan Hintz
2013-10-29 6:52 ` Rafał Miłecki
2013-10-29 8:22 ` Nathan Hintz
2013-10-29 8:28 ` Rafał Miłecki
2013-10-29 15:20 ` Nathan Hintz
2013-10-29 19:39 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).