Unused function mac802154_header_parse()

Unused function mac802154_header_parse()

[Varka Bhadram]

Hi,

There are two header_ops operations defined for mac802154 at [1]. One is for creating header another for parsing.

Creation of mac802154_header_create() happening at [2] by using dev_hard_header(), but i did not find dev_parse_header()
for header parse by mac802154_header_parse().

Am i missing anything.?



[1]:http://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/mac802154/iface.c#n368  
[2]:http://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/ieee802154/dgram.c#n274

-- 
Thanks and Regards,
Varka Bhadram.

Re: Unused function mac802154_header_parse()

[Alexander Aring]

Hi,

On Fri, Nov 21, 2014 at 09:35:21PM +0530, Varka Bhadram wrote:
> Hi,
> 
> There are two header_ops operations defined for mac802154 at [1]. One is for creating header another for parsing.
> 
> Creation of mac802154_header_create() happening at [2] by using dev_hard_header(), but i did not find dev_parse_header()
> for header parse by mac802154_header_parse().
> 
> Am i missing anything.?
> 

Parsing frames are completely done at [0]. With call of
"netif_receive_skb(skb);" the frame is deliverd into packet layer.

Currently there exist several issues with parsing frames which I don't
will explain now.



Now the header_ops structure:

These callbacks are many used by arp or ndisc. For me there exist no
reason why a wpan interface implements such functionallity. The lowpan
interface needs these callbacks. For the rework I removed dev_hard_header
calls for 802154 upper layers. Not for the lowpan interface.

What we now doing inside the 802154 layer is to use dev_hard_header and pass
additional parameters over skb->cb, but we don't need that for the wpan interface.

At lowpan interface the ndisc IPv6 cache will tell us the destination
address, that's why need this there.

For af802154: this is currently a complete weird implementation.


For more documentation about that the header_ops callback structure
does, read the implementation for ethernet. [1]


Sorry I am busy right now and it's also not easy to doing some new
mechanism into the frame parsing/mac header generation because it's used
everywhere.

- Alex

[0] http://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/mac802154/rx.c?id=24ccb9f4f7a3a5a867bbc880019cdb4b41176b63#n196
[1] http://lxr.free-electrons.com/source/net/ethernet/eth.c#L345

Re: Unused function mac802154_header_parse()

[Alexander Aring]

On Fri, Nov 21, 2014 at 09:35:21PM +0530, Varka Bhadram wrote:
> Hi,
> 
> There are two header_ops operations defined for mac802154 at [1]. One is for creating header another for parsing.
> 
> Creation of mac802154_header_create() happening at [2] by using dev_hard_header(), but i did not find dev_parse_header()
> for header parse by mac802154_header_parse().
> 
> Am i missing anything.?
> 

For the dev_parse_header usage:

`grep -r -n "dev_parse_header" net`:

net/packet/af_packet.c:1823:	sll->sll_halen = dev_parse_header(skb, sll->sll_addr);
net/packet/af_packet.c:2030:	sll->sll_halen = dev_parse_header(skb, sll->sll_addr);

This is used by the packet layer at several places.



I need to dig more into this. It seems that these callback structure are
optional because dev_hard_header and dev_parse_header header return 0 if
they are not implemented.

Also it seems that when af_packet uses dev_hard_header header it could
be that the control block information is overwritten by packet_skb_cb.
So it's _maybe_ a bug to add additional parameters over control block
information for the header_create callback.

- Alex

Re: Unused function mac802154_header_parse()

[Varka Bhadram]

On Friday 21 November 2014 10:40 PM, Alexander Aring wrote:
> On Fri, Nov 21, 2014 at 09:35:21PM +0530, Varka Bhadram wrote:
>> Hi,
>>
>> There are two header_ops operations defined for mac802154 at [1]. One is for creating header another for parsing.
>>
>> Creation of mac802154_header_create() happening at [2] by using dev_hard_header(), but i did not find dev_parse_header()
>> for header parse by mac802154_header_parse().
>>
>> Am i missing anything.?
>>
> For the dev_parse_header usage:
>
> `grep -r -n "dev_parse_header" net`:
>
> net/packet/af_packet.c:1823:	sll->sll_halen = dev_parse_header(skb, sll->sll_addr);
> net/packet/af_packet.c:2030:	sll->sll_halen = dev_parse_header(skb, sll->sll_addr);
>
> This is used by the packet layer at several places.
>
>
>
> I need to dig more into this. It seems that these callback structure are
> optional because dev_hard_header and dev_parse_header header return 0 if
> they are not implemented.
>
> Also it seems that when af_packet uses dev_hard_header header it could
> be that the control block information is overwritten by packet_skb_cb.
> So it's _maybe_ a bug to add additional parameters over control block
> information for the header_create callback.

My point is that no where mac802154_header_parse() function is called.

This has to be called by dev_parse header().

-- 
Thanks and Regards,
Varka Bhadram.

Re: Unused function mac802154_header_parse()

[Alexander Aring]

On Sat, Nov 22, 2014 at 05:55:25AM +0530, Varka Bhadram wrote:
> 
> My point is that no where mac802154_header_parse() function is called.
> 
> This has to be called by dev_parse header().
> 

Okay, I dig more into this code. Aigain

`grep -r -n "dev_parse_header" net` will result:

net/packet/af_packet.c:1823:	sll->sll_halen = dev_parse_header(skb, sll->sll_addr);
net/packet/af_packet.c:2030:	sll->sll_halen = dev_parse_header(skb, sll->sll_addr);

`cat net/packet/Makefile` will result:

obj-$(CONFIG_PACKET) += af_packet.o

so af_packet is selected when CONFIG_PACKET is enabled.

CONFIG_PACKET help:

The Packet protocol is used by applications which communicate
directly with network devices without an intermediate network
protocol implemented in the kernel, e.g. tcpdump.  If you want them
to work, choose Y.

This is enabled on my side.

I did a (inside the mac802154_header_parse function):

printk(KERN_INFO "this crazy function was called!\n");

and start tcpdump on it.

`tcpdump -i wpan0` and ping6 on other node will result:

...
[   25.407216] this crazy function was called!
[   25.417991] this crazy function was called!
[   25.428034] this crazy function was called!
[   25.438051] this crazy function was called!
[   25.448058] this crazy function was called!
[   25.458231] this crazy function was called!
[   25.468280] this crazy function was called!
[   25.478334] this crazy function was called!
[   25.488386] this crazy function was called!
[   25.498445] this crazy function was called!
[   25.509029] this crazy function was called!
  1   0.000000      2001::4 -> ff02::1:ff00:1 ICMPv6 80 Neighbor Solicitation for 2001::1
  2   0.009032      2001::1 -> 2001::4      ICMPv6 96 Neighbor Advertisement 2001::1 (sol, ovr) from a0:b0:74:f3:c9:c8:0b:cc
  3   0.036129 02:12:13:ff:fe:14:15:16 -> a0:b0:74:f3:c9:c8:0b:cc 6LoWPAN 116 Data, Dst: a0:b0:74f3:c9:c80b:cc, Src: 02:12:13ff:fe:1415:16
  4   0.044886 02:12:13:ff:fe:14:15:16 -> a0:b0:74:f3:c9:c8:0b:cc 6LoWPAN 122 Data, Dst: a0:b0:74f3:c9:c80b:cc, Src: 02:12:13ff:fe:1415:16
  5   0.053285 02:12:13:ff:fe:14:15:16 -> a0:b0:74:f3:c9:c8:0b:cc 6LoWPAN 122 Data, Dst: a0:b0:74f3:c9:c80b:cc, Src: 02:12:13ff:fe:1415:16
  6   0.060645 02:12:13:ff:fe:14:15:16 -> a0:b0:74:f3:c9:c8:0b:cc 6LoWPAN 122 Data, Dst: a0:b0:74f3:c9:c80b:cc, Src: 02:12:13ff:fe:1415:16
  7   0.069100 02:12:13:ff:fe:14:15:16 -> a0:b0:74:f3:c9:c8:0b:cc 6LoWPAN 122 Data, Dst: a0:b0:74f3:c9:c80b:cc, Src: 02:12:13ff:fe:1415:16
  8   0.076858 02:12:13:ff:fe:14:15:16 -> a0:b0:74:f3:c9:c8:0b:cc 6LoWPAN 122 Data, Dst: a0:b0:74f3:c9:c80b:cc, Src: 02:12:13ff:fe:1415:16
...

So this function ist used for tcpdump, etc.



New questions are:

What exactly do this function?

-> I have no idea. It parse the source address to a allocated buffer and
   return the length of the address. But what af_packet does with that,
   currently I have no idea.

Should we remove this function?

-> No, if we don't know exactly what this function does.

Should the monitor device implement a header_parse function?

-> No, because we don't parse anything at monitor function.
   Parsing source address will depends on that the frame is correct.

Why I talked about "dev_hard_header" many times in my last mail.

-> Sorry, but I detected now that af_packet.c also call dev_hard_header.
   And calling dev_hard_header require currently to set the right
   control_block information. This seems to be buggy at the moment.
   Inside the rework I removed the header create callback because that
   and this is one more reason to remove this and create "global"
   802.15.4 mac header create functions.

Why I didn't do that already:

-> Currently I am busy because my university and it's not easy to
   replace the frame parsing and frame creation. It's used everywhere,
   the first step for now was to add support for nl802154 and many
   architecture changes. This is mostly done now.

- Alex