On Tue, Sep 16, 2014 at 11:28:06AM +0100, Martin Townsend wrote:
>
> On 16/09/14 11:17, Alexander Aring wrote:
> > On Tue, Sep 16, 2014 at 11:04:13AM +0100, Martin Townsend wrote:
> >> Hi Alex,
> >>
> >> On the lowpan_give_skb_to_devices change.
> >>
> >> As we are iterating over a list of lowpan_devices and could potentially copy the skb more than once, what happens if the first device returns NET_RX_DROP and then the second time it return NET_RX_SUCCESS? The stat variable is overwritten so stat only ever reflects the return value of netif_rx for the last device?
> >>
> >> Maybe it's better to completely remove the if else at the end and always consume the skb? For the case whereskb_copy fails then we should kfree_skb,
> >> e.g.
> >>
> >> static int lowpan_give_skb_to_devices(struct sk_buff *skb,
> >> struct net_device *dev)
> >> {
> >> struct lowpan_dev_record *entry;
> >> struct sk_buff *skb_cp;
> >> int stat = NET_RX_SUCCESS;
> >>
> >> rcu_read_lock();
> >> list_for_each_entry_rcu(entry, &lowpan_devices, list)
> >> if (lowpan_dev_info(entry->ldev)->real_dev == skb->dev) {
> >> skb_cp = skb_copy(skb, GFP_ATOMIC);
> >> if (!skb_cp) {
> >> kfree_skb(skb);
> >> rcu_read_unlock();
> >> return NET_RX_DROP;
> >> }
> >>
> >> skb_cp->dev = entry->ldev;
> >> stat = netif_rx(skb_cp);
> > here we should do a:
> >
> > if (stat == NET_RX_DROP)
> > kfree_skb(skb_cp);
> >
> > or? It doesn't deliver and then we "could" lost the pointer.
> Doesn't netif_rx always free the buffer?
yes, you are right. [0] Now other things makes more sense for me.
Thanks.
I mean there is another deliver function netif_receive_skb and on
comment always stand "Return values (usually ignored)", depends on
context what you need. But netif_rx in this context is right.
Here we should not ignore the return value, because we already are in
the packet layer (the packet layer func callback). The netif_receive_skb
function we need for putting frames from the driver (some tasklet context)
into the packet layer.
[0] is some function which is called mainly after "netif_rx" function,
"netif_receive_skb" will call this function, too.
- Alex
[0] http://lxr.free-electrons.com/source/net/core/dev.c#L3280
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
