IPv6 over Firewire

IPv6 over Firewire

[Stephan Gatzka]

Hi!

I'm currently implementing IPv6 over firewire.

To make the address mapping to the firewire link layer RFC3146 mandates 
to use neighbor discovery with a certain format of the source/target 
link-layer address option.

While it is not too complicated to build that up, I'm wondering how I 
can reserve enough memory in the corresponding skb.

One possibility is to introduce some option padding in 
ndisc_addr_option_pad() but I think that's somehow weird.

The second option I see is to set needed_tailroom in struct netdevice 
for firewire net devices. That's the way I would go for.

Because I'm not really familiar with the whole network infrastructure in 
Linux, a confirmation for the way to go would be nice.

Regards,

Stephan

Re: IPv6 over Firewire

[YOSHIFUJI Hideaki]

(2012年12月22日 02:03), Stephan Gatzka wrote:

> To make the address mapping to the firewire link layer RFC3146 mandates to use neighbor discovery with a certain format of the source/target link-layer address option.
> 
> While it is not too complicated to build that up, I'm wondering how I can reserve enough memory in the corresponding skb.
> 
> One possibility is to introduce some option padding in ndisc_addr_option_pad() but I think that's somehow weird.
> 
> The second option I see is to set needed_tailroom in struct netdevice for firewire net devices. That's the way I would go for.
> 
> Because I'm not really familiar with the whole network infrastructure in Linux, a confirmation for the way to go would be nice.

If you are talking about how to build NS/NA/RS/Redirect messages, you
can just use ndisc_opt_addr_space() and ndisc_fill_addr_option() here.

--yoshfuji

Re: IPv6 over Firewire

[Stephan Gatzka]

> If you are talking about how to build NS/NA/RS/Redirect messages, you
> can just use ndisc_opt_addr_space() and ndisc_fill_addr_option() here.

Thanks, these functions are certainly helpful. But 
ndisc_opt_addr_space() calculates the required space from dev->addr_len 
and ndisc_addr_option_pad(dev->type). The latter is 0 for IEEE1394 
(firewire). So the required option space just comes from dev->addr_len, 
which is 8 for firewire, resulting in an option address space of 16 (2 
octets).

But rfc3146 requires an option address space of 3 octets. So my main 
question is if in such a situation the best is to reserve additional skb 
tail room using needed_tailroom in struct netdevice. This directly 
affects the memory allocated in ndisc_build_skb().

Regards,

Stephan

Re: IPv6 over Firewire

[YOSHIFUJI Hideaki]

Stephan Gatzka wrote:
> 
>> If you are talking about how to build NS/NA/RS/Redirect messages, you
>> can just use ndisc_opt_addr_space() and ndisc_fill_addr_option() here.
> 
> Thanks, these functions are certainly helpful. But ndisc_opt_addr_space() calculates the required space from dev->addr_len and ndisc_addr_option_pad(dev->type). The latter is 0 for IEEE1394 (firewire). So the required option space just comes from dev->addr_len, which is 8 for firewire, resulting in an option address space of 16 (2 octets).
> 
> But rfc3146 requires an option address space of 3 octets. So my main question is if in such a situation the best is to reserve additional skb tail room using needed_tailroom in struct netdevice. This directly affects the memory allocated in ndisc_build_skb().

Something like this:

 static inline int ndisc_opt_addr_space(struct net_device *dev)
 {
-       return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
+       switch (dev->type) {
+       case ARPHRD_IEEE1394:
+               return sizeof(struct ndisc_opt_ieee1394_llinfo);
+       default:
+               return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
+       }
 }

--yoshfuji

Re: IPv6 over Firewire

[Stefan Richter]

On Dec 22 YOSHIFUJI Hideaki wrote:
> Stephan Gatzka wrote:
> > 
> >> If you are talking about how to build NS/NA/RS/Redirect messages, you
> >> can just use ndisc_opt_addr_space() and ndisc_fill_addr_option() here.
> > 
> > Thanks, these functions are certainly helpful. But ndisc_opt_addr_space() calculates the required space from dev->addr_len and ndisc_addr_option_pad(dev->type). The latter is 0 for IEEE1394 (firewire). So the required option space just comes from dev->addr_len, which is 8 for firewire, resulting in an option address space of 16 (2 octets).
> > 
> > But rfc3146 requires an option address space of 3 octets. So my main question is if in such a situation the best is to reserve additional skb tail room using needed_tailroom in struct netdevice. This directly affects the memory allocated in ndisc_build_skb().
> 
> Something like this:
> 
>  static inline int ndisc_opt_addr_space(struct net_device *dev)
>  {
> -       return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
> +       switch (dev->type) {
> +       case ARPHRD_IEEE1394:
> +               return sizeof(struct ndisc_opt_ieee1394_llinfo);
> +       default:
> +               return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
> +       }
>  }

Can't we increase dev->addr_len for RFC 3146 interfaces?
Can't we add another dev->type besides ARPHRD_IEEE1394 (RFC 2734)?

Is a single dev instance transporting both IPv4 and IPv6 or will there be
separate instances for those?
-- 
Stefan Richter
-=====-===-- ==-- =-==-
http://arcgraph.de/sr/

Re: IPv6 over Firewire

[Stephan Gatzka]

>
> Can't we increase dev->addr_len for RFC 3146 interfaces?
> Can't we add another dev->type besides ARPHRD_IEEE1394 (RFC 2734)?
>
> Is a single dev instance transporting both IPv4 and IPv6 or will there be
> separate instances for those?
>

Right now there is a single dev instance for both IPv4 and IPv6. Two 
instances means there will be two separate device, doesn't it?

Stephan

------------------------------------------------------------------------------
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d

Re: IPv6 over Firewire

[Stephan Gatzka]

> Something like this:
>
>   static inline int ndisc_opt_addr_space(struct net_device *dev)
>   {
> -       return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
> +       switch (dev->type) {
> +       case ARPHRD_IEEE1394:
> +               return sizeof(struct ndisc_opt_ieee1394_llinfo);
> +       default:
> +               return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
> +       }
>   }
>
> --yoshfuji
>

O.k., this has the advantage that only ndisc packets get some more 
memory, but the question is if we are under such a hard memory pressure 
that we don't allow that.

Your solution has the disadvantage that now I have to publish struct 
ndisc_opt_ieee1394_llinfo to the ndisc stuff. Nobody in ndisc.c really 
wants to deal with that structure, only the size is of interest. So 
keeping this struct private is less invasive to the rest of linux. Just 
my two cents.

Regards,

Stephan

Re: IPv6 over Firewire

[Stefan Richter]

On Dec 22 Stephan Gatzka wrote:
> 
> > Something like this:
> >
> >   static inline int ndisc_opt_addr_space(struct net_device *dev)
> >   {
> > -       return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
> > +       switch (dev->type) {
> > +       case ARPHRD_IEEE1394:
> > +               return sizeof(struct ndisc_opt_ieee1394_llinfo);
> > +       default:
> > +               return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
> > +       }
> >   }
> >
> > --yoshfuji
> >
> 
> O.k., this has the advantage that only ndisc packets get some more 
> memory, but the question is if we are under such a hard memory pressure 
> that we don't allow that.
> 
> Your solution has the disadvantage that now I have to publish struct 
> ndisc_opt_ieee1394_llinfo to the ndisc stuff. Nobody in ndisc.c really 
> wants to deal with that structure, only the size is of interest. So 
> keeping this struct private is less invasive to the rest of linux. Just 
> my two cents.

You could add another case to include/net/ndisc.h::ndisc_addr_option_pad()
with a hardcoded size, couldn't you?
-- 
Stefan Richter
-=====-===-- ==-- =-==-
http://arcgraph.de/sr/

Re: IPv6 over Firewire

[Stephan Gatzka]

>
> You could add another case to include/net/ndisc.h::ndisc_addr_option_pad()
> with a hardcoded size, couldn't you?
>
No, I think that is almost certainly not a good idea. The address space 
option is handed over to the firewire_net driver like this:
type, length, soure/target link address (GUID)

If I add another case in ndisc_addr_option_pad() I think the option will 
look like this:
pad, type, length, soure/target link address (GUID)

Because pad, type and GUID are already at the correct position for the 
3146 link layer option. So with padding I have to copy them to the 
correct position.

All I need is some (8 bytes) of additional tail room in the ndisc skb. 
This could be achieved either by specifying needed_tailroom in the 
firewire netdevice struct at the expense that now every skb allocated 
might get 8 bytes more allocated.

The second option is yoshfuji suggestion to pimp ndisc_opt_addr_space a 
bit. His solution only allocates additional memory for ndisc packets at 
the expense to introduce a dependency to the struct 
ndisc_opt_ieee1394_llinfo.

These are the two option we can go for. Personally I think reserving a 
bit more tail room looks cleaner if nobody votes against it...

Regards,

Stephan

Re: IPv6 over Firewire

[YOSHIFUJI Hideaki]

(2012年12月22日 15:10), Stephan Gatzka wrote:
> 
>> Something like this:
>>
>>   static inline int ndisc_opt_addr_space(struct net_device *dev)
>>   {
>> -       return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
>> +       switch (dev->type) {
>> +       case ARPHRD_IEEE1394:
>> +               return sizeof(struct ndisc_opt_ieee1394_llinfo);
>> +       default:
>> +               return NDISC_OPT_SPACE(dev->addr_len + ndisc_addr_option_pad(dev->type));
>> +       }
>>   }
>>
>> --yoshfuji
>>
> 
> O.k., this has the advantage that only ndisc packets get some more memory, but the question is if we are under such a hard memory pressure that we don't allow that.
> 
> Your solution has the disadvantage that now I have to publish struct ndisc_opt_ieee1394_llinfo to the ndisc stuff. Nobody in ndisc.c really wants to deal with that structure, only the size is of interest. So keeping this struct private is less invasive to the rest of linux. Just my two cents.

net/ipv6/ndisc.c SHOULD build full NDP messages for IPv6
over IEEE1394 as we do it for Infiniband.

Please, please do not try to mangle them in the driver.

--yoshfuji

Re: IPv6 over Firewire

[Stephan Gatzka]

> net/ipv6/ndisc.c SHOULD build full NDP messages for IPv6
> over IEEE1394 as we do it for Infiniband.
>
> Please, please do not try to mangle them in the driver.
>
As far as I understand the code for Infiniband (and the corresponding 
RFC4391) I just see the introduction of two pad bytes. Moreover, I see 
that ndisc_build_skb calls ndisc_fill_addr_option which copies 
dev->dev_addr. Maybe the so called Queue Pair Number (QPN) is already 
included in dev->dev_addr. If not, I guess the Infiniband driver will 
also mangle the QPN into the link layer option. If not, this seems only 
possible because the format for IPv6 link layer option (IB) and IPv4/ARP 
(IB) has the same format.

This is not true IPv4/ARP and IPv6 link layer option for firewire. 
Moreover, firewire link layer address mapping (IPv4 and IPv6) requires 
some very firewire specific information like speed, max_rec and 
especially the so called unicast fifo address.

 From my point of view the generic ndisc code shall not cope with these 
nasty details of the specific link layers. I also haven't found a driver 
specific hook that might fill these information in. That's why I think I 
_have_ to mangle the NDP stuff in the driver.

Stephan

Re: IPv6 over Firewire

[YOSHIFUJI Hideaki]

Stephan Gatzka wrote:
>> net/ipv6/ndisc.c SHOULD build full NDP messages for IPv6
>> over IEEE1394 as we do it for Infiniband.
>>
>> Please, please do not try to mangle them in the driver.
>>
> As far as I understand the code for Infiniband (and the corresponding RFC4391) I just see the introduction of two pad bytes. Moreover, I see that ndisc_build_skb calls ndisc_fill_addr_option which copies dev->dev_addr. Maybe the so called Queue Pair Number (QPN) is already included in dev->dev_addr. If not, I guess the Infiniband driver will also mangle the QPN into the link layer option. If not, this seems only possible because the format for IPv6 link layer option (IB) and IPv4/ARP (IB) has the same format.

Please, please try best not to mangle packets
and keep/make IPsec, SEND right.

Well, Infiniband's hardware length (INFINIBAND_ALEN) is 20,
which means that it includes reserved and QPN field.
Moreover Infiniband driver uses neighbor notification
mechanism.

I think Firewire can use that as well.
What do you think?

--yoshfuji

Re: IPv6 over Firewire

[Stephan Gatzka]

>
> Please, please try best not to mangle packets
> and keep/make IPsec, SEND right.
>

O.k., a small excerpt from RFC 3146:
The Source/Target Link-layer Address option has the following form when 
the link layer is IEEE1394.
                          1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |     Type      |  Length = 3   |                               |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+                            ---+
    |                    node_unique_ID (EUI-64 identifier)         |
    +---                            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                               |    max_rec    |      spd      |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                          unicast_FIFO                         |
    +---                            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                               |            reserved           |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                            reserved                           |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

This is what I _have_ to send to comply with RFC 3146. In the skb I get 
in the firewire net driver I have: Type, Length and node_unique_ID (the 
hardware address), all tail padded to a 8 byte boundary:

                          1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |     Type      |  Length = 3   |                               |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+                            ---+
    |                    node_unique_ID (EUI-64 identifier)         |
    +---                            +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                               |    0x00     |      0x00       |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                          0x00000000                           |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

So may aim is to rewrite the LL address option both while transmitting 
and receiving IPv6 data on firewire. While transmitting, I append the 
required information, while receiving I cut away the the firewire 
specific stuff. So the upper layers will not see the firewire related 
information. Yes, that mangling also involves recalculation of the 
ICMPv6 checksum.

Again, if I shall not mangle the ndisc packets in the driver I have to 
build RFC 3146 conformant packets in ndisc_build_skb(). But I see no 
(general) way to get the firewire specific information into the linux 
ndisc stuff. Of course I can add some if/else into 
ndisc_fill_addr_option() and ndisc_recv_na(). But then I have to pull a 
lot of firewire related stuff into the ndisc code.

I think this makes no sense, especially because I need that firewire 
specific information in the firewire net driver to send correct firewire 
packets. Besides that, the IPv4 portion of the firewire net drivers does 
exactly the same with ARP packets what I want to do with IPv6 LL address 
options.

Regards,

Stephan

Re: IPv6 over Firewire

[YOSHIFUJI Hideaki]

Stephan Gatzka wrote:

> So may aim is to rewrite the LL address option both while transmitting and receiving IPv6 data on firewire. While transmitting, I append the required information, while receiving I cut away the the firewire specific stuff. So the upper layers will not see the firewire related information. Yes, that mangling also involves recalculation of the ICMPv6 checksum.

No, it is not enough.  For example, Signature Option of SEND
(RFC3971) takes link-layer address options into account.
If anything is happened to be altered, verification will
fail.

> Again, if I shall not mangle the ndisc packets in the driver I have to build RFC 3146 conformant packets in ndisc_build_skb(). But I see no (general) way to get the firewire specific information into the linux ndisc stuff. Of course I can add some if/else into ndisc_fill_addr_option() and ndisc_recv_na(). But then I have to pull a lot of firewire related stuff into the ndisc code.
>
> I think this makes no sense, especially because I need that firewire specific information in the firewire net driver to send correct firewire packets. Besides that, the IPv4 portion of the firewire net drivers does exactly the same with ARP packets what I want to do with IPv6 LL address options.

Please, please do not alter bits in your driver, to maintain
extensibility of protocols.

I am definitely okay to have firewire supporting stuff in
net/ipv6/ndisc.c.  Since we already have notification
mechanism, I don't think it will be large.

--yoshfuji

Re: IPv6 over Firewire

[Stephan Gatzka]

> Please, please do not alter bits in your driver, to maintain
> extensibility of protocols.
>
> I am definitely okay to have firewire supporting stuff in
> net/ipv6/ndisc.c.  Since we already have notification
> mechanism, I don't think it will be large.
>
O.k., let's try it your way.

I believe the the changes in the transmit portions for ndisc are rather 
simple. All firewire specific information is stored in the private data 
of struct net_device. For the receive section I think I have to change 
something in ndisc_recv_ns(), I'll figure that out. Maybe I need some 
(small) support from you, but I think we can do that off list.

I'll try it out but after Christmas. :)

But I have to emphasize that I really need (read) access to the firewire 
specific portions of the link layer option in the firewire net driver. 
That _is_ necessary to build the relation between the firewire hardware 
address (GUID) and the corresponding firewire node.

Because you mentioned IPSEC: Are the ndisc packets also encrypted? If 
so, I just can't imagine how I can do the mapping of firewire hardware 
addresses and firewire nodes.

@Stefan: Are you o.k. if I go that way?

Regards,

Stephan

Re: IPv6 over Firewire

[YOSHIFUJI Hideaki]

Stephan Gatzka wrote:

> I believe the the changes in the transmit portions for ndisc are rather simple. All firewire specific information is stored in the private data of struct net_device. For the receive section I think I have to change something in ndisc_recv_ns(), I'll figure that out. Maybe I need some (small) support from you, but I think we can do that off list.
> 
> I'll try it out but after Christmas. :)
> 
> But I have to emphasize that I really need (read) access to the firewire specific portions of the link layer option in the firewire net driver. That _is_ necessary to build the relation between the firewire hardware address (GUID) and the corresponding firewire node.

If you change the "hardware address length", you can get
notification when you have new neighbor.
Even if you did not change that, you could introduce new
netevent notification.

> Because you mentioned IPSEC: Are the ndisc packets also encrypted? If so, I just can't imagine how I can do the mapping of firewire hardware addresses and firewire nodes.

I don't think we will see encrypted ND messages, but
IP layer should handle encrypted NDISC messages.

--yoshfuji

Re: IPv6 over Firewire

[Stefan Richter]

On Dec 23 Stephan Gatzka wrote:
> @Stefan: Are you o.k. if I go that way?

Sure, go ahead.  If it comes to networking issues, I am certainly not
someone to seek advise at, let alone permission. :-)
-- 
Stefan Richter
-=====-===-- ==-- ==---
http://arcgraph.de/sr/

Re: IPv6 over firewire

[Stefan Richter]

On Jan 12 Stephan Gatzka wrote:
> 
> > And, last but big issue is, well, packet inspection at
> > driver level.  For example, you use ndisc_parse_options()
> > and code depends on CONFIG_IPV6=y.
> 
> Good point. Of course I can make firewire-net dependent from IPv6.
> 
> The other option is to build a separate firewire_ipv6 module. 
> Implementing that looks easy enough. From the original firewire net 
> driver I just need a hook to register a function that investigates 
> incoming packets for IPv6 link layer option. In addition, 
> fwnet_peer_find_by_guid must be exported.
> 
> Stefan, what is your opinion on that?

>From your previous patch it looks like that the RFC 3146 related parts in
drivers/firewire/net.c are just a few places that can be enclosed into
#if/#endif blocks.  That way, firewire-net will be built according to the
same Kconfig switches as before, and gets RFC 3146 capability if
CONFIG_IPV6=y or =m.

#if/#endif are generally not very nice, but in this case I think they can
be kept localized and thus are more beneficial than hurtful.

There is a catch:  Since both net/ipv6/Kconfig::IPV6 and
drivers/firewire/Kconfig::FIREWIRE_NET are tristate variables, we need to
prevent CONFIG_FIREWIRE_NET=y while CONFIG_IPV6=m.  One way to do that
would be like it was done for the infrared remote control support in the
firedtv driver which depends on the CONFIG_INPUT tristate variable.  This
was solved like this:
  - The helper variable drivers/media/firewire/Kconfig::DVB_FIREDTV_INPUT
    was added.
  - This variable is true if CONFIG_INPUT=y,
    or if CONFIG_DVB_FIREDTV=m and CONFIG_INPUT=m.
    In other words, if CONFIG_INPUT=m and CONFIG_DVB_FIREDTV=y, firedtv
    silently loses support for remote control input.
  - The respective firedtv parts are of course made dependent on
    CONFIG_DVB_FIREDTV.  In this case, only
    drivers/media/firewire/firedtv.h and drivers/media/firewire/Makefile
    are minimally affected.

So, if implemented like that, the special case of CONFIG_FIREWIRE_NET=y &&
CONFIG_IPV6=m would mean that firewire-net would be built but would lack
RFC 3146 support.

With a little bit more Kconfig related effort one could solve it such that
CONFIG_FIREWIRE_NET=y && CONFIG_IPV6=m would actually put RFC 3146 support
into firewire-net but would force it to be built modular instead of
statically.

Not sure which way is preferrable.  Cc'ing netdev.

In case of CONFIG_IPV6=n however, people should still be able to build
firewire-net with its current RFC 2734-only feature set.
-- 
Stefan Richter
-=====-===-= ---= -==--
http://arcgraph.de/sr/

Re: IPv6 over firewire

[Stefan Richter]

[Cc'ing netdev.  This is about
http://marc.info/?l=linux1394-devel&m=135723690427469]

On Jan 12 Stephan Gatzka wrote:
> On Jan 10 Stefan Richter wrote:
> > Relative to mentioned preliminary patch, there need to be some cleanups
> > done, in my opinion:
> >   - The move of struct fwnet_device from drivers/firewire/net.c into
> >     include/linux/firewire.h, the inclusion of <linux/firewire.h> into
> >     net/ipv6/ndisc.c, and the use of <linux/firewire.h> definitions in
> >     net/ipv6/ndisc.c need to be undone.
> >   - Instead, the dependencies should be solved such that
> >     include/net/ndisc.h (or whatever net header would be appropriate
> >     for these kinds of definitions) defines RFC 3146 specific structs
> >     or/and functions (or extensions of existing functions by additional
> >     arguments) or/and driver ops (alias methods alias callbacks, in the
> >     style of net_device.header_ops and the likes).  Then, both
> >     net/ipv6/ndisc.c and drivers/firewire/net.c would use these new RFC
> >     3146 related definitions but net/ipv6/ndisc.c would not become a
> >     user of any <linux/firewire.h> definitions.
> 
> 
> Just to know if I understood you correctly. You want to have a function 
> pointer in struct net_device like fill_link_layer_option(). This 
> function pointer has to be set when allocating a struct net_device object.
> 
> This is certainly possible, but if I do so, I'd assume that then _every_ 
> network device (ethernet, infiniband, ...) in linux has to implement 
> such a function. Otherwise, I think the ndisc code only for a firewire 
> net device will call the callback function. So I would introduce a 
> callback routine in struct net_device just for firewire.
> 
> The problem is, that right now, firewire is the _only_ device that 
> requires this special link layer option format. All other network device 
> do not require this right now. So I ask myself if its worth to introduce 
> this callback routine just for firewire.

I haven't looked in detail into what is really required here.  Maybe a new
driver op (callback) isn't actually needed.

But if a new callback is needed, it doesn't have to burden the kernel much:

  - In the linux-kernel OOP model, there are mandatory methods as well
    as optional methods.  A new method for RFC 3146 Neighbor Discovery
    should of course be an optional one.

  - There is a downside to optional methods:  The core code which may
    have to use the method first needs to do a NULL check of the
    method's function pointer or needs to check a correlated
    capabilities flag or something like that.  Such additional checks
    are undesirable in hot paths, but I suppose IPv6 Neighbor Discovery
    is not a particularly hot path.

  - There is another downside:  Each new driver method increases the
    memory footprint of instances of respective function pointer tables
    by 4 or 8 bytes.

Both downsides can be countered somewhat by enclosing the respective
code into #ifdef CONFIG_RFC3146_NDISC...#endif and have something like
    select RFC3146_NDISC if IPV6 = y || (IPV6 = m && FIREWIRE_NET = m)
in the "config FIREWIRE_NET" section.

But the new callback (if one is really needed) doesn't have to be a driver
method.  It could also look about like this:

    include/net/ndisc.h:
    -extern struct ndisc_options *ndisc_parse_options(u8 *opt, int opt_len,
    -		struct ndisc_options *ndopts);
    +extern struct ndisc_options *__ndisc_parse_options(u8 *opt,
    +		int opt_len, struct ndisc_options *ndopts,
    +		whatever_type *callback);
    +
    +static inline struct ndisc_options *ndisc_parse_options(u8 *opt,
    +		int opt_len, struct ndisc_options *ndopts)
    +{
    +	return __ndisc_parse_options(opt, len, ndopts, NULL);
    +}

    net/ipv6/ndisc.c:
    -struct ndisc_options *ndisc_parse_options(u8 *opt, int opt_len,
    -		struct ndisc_options *ndopts)
    +struct ndisc_options *__ndisc_parse_options(u8 *opt, int opt_len,
    +		struct ndisc_options *ndopts, whatever_type *callback)
     {

Or the (optional!) callback could be a new member of struct ndisc_options.

However, does net/ipv6/ndisc.c really need to be aware of the RFC 3146
Neighbor Discovery related packet header layouts?  Isn't it possible to
rewrite these headers in-place in drivers/firewire/net.c?
-- 
Stefan Richter
-=====-===-= ---= -==--
http://arcgraph.de/sr/

Re: IPv6 over firewire

[Stephan Gatzka]

> But if a new callback is needed, it doesn't have to burden the kernel much:
>
>    - In the linux-kernel OOP model, there are mandatory methods as well
>      as optional methods.  A new method for RFC 3146 Neighbor Discovery
>      should of course be an optional one.
>
>    - There is a downside to optional methods:  The core code which may
>      have to use the method first needs to do a NULL check of the
>      method's function pointer or needs to check a correlated
>      capabilities flag or something like that.  Such additional checks
>      are undesirable in hot paths, but I suppose IPv6 Neighbor Discovery
>      is not a particularly hot path.

Yes, that should be true.

>
>    - There is another downside:  Each new driver method increases the
>      memory footprint of instances of respective function pointer tables
>      by 4 or 8 bytes.

I can't imagine that this is a show stopper. How many instances of 
struct netdev do we have o a typical system? I guess not the much.

>
> Both downsides can be countered somewhat by enclosing the respective
> code into #ifdef CONFIG_RFC3146_NDISC...#endif and have something like
>      select RFC3146_NDISC if IPV6 = y || (IPV6 = m && FIREWIRE_NET = m)
> in the "config FIREWIRE_NET" section.
>
> But the new callback (if one is really needed) doesn't have to be a driver
> method.  It could also look about like this:
>
>      include/net/ndisc.h:
>      -extern struct ndisc_options *ndisc_parse_options(u8 *opt, int opt_len,
>      -		struct ndisc_options *ndopts);
>      +extern struct ndisc_options *__ndisc_parse_options(u8 *opt,
>      +		int opt_len, struct ndisc_options *ndopts,
>      +		whatever_type *callback);
>      +
>      +static inline struct ndisc_options *ndisc_parse_options(u8 *opt,
>      +		int opt_len, struct ndisc_options *ndopts)
>      +{
>      +	return __ndisc_parse_options(opt, len, ndopts, NULL);
>      +}
>
>      net/ipv6/ndisc.c:
>      -struct ndisc_options *ndisc_parse_options(u8 *opt, int opt_len,
>      -		struct ndisc_options *ndopts)
>      +struct ndisc_options *__ndisc_parse_options(u8 *opt, int opt_len,
>      +		struct ndisc_options *ndopts, whatever_type *callback)
>       {
>
> Or the (optional!) callback could be a new member of struct ndisc_options.

Hm, right now I have no opinion on that. Where does
whatever_type *callback comes from? Is it an exported method of the 
firewire net driver or the new function pointer in struct netdevice?

> However, does net/ipv6/ndisc.c really need to be aware of the RFC 3146
> Neighbor Discovery related packet header layouts?  Isn't it possible to
> rewrite these headers in-place in drivers/firewire/net.c?


Yes, it it possible, but yoshfuji strongly voted against rewriting ndisc 
packets in firewire net driver to maintain extensibility to protocols. 
Especially IPSEC can just not work if I rewrite the packets in the driver.

Regards,

Stephan


-- 
Welchen Unterschied gibt es zwischen einem toten Hund auf der
Straße und einer überfahrenen Bratsche auf der Straße? Vor dem
toten Hund gibt es Bremsspuren.

Re: IPv6 over firewire

[Stefan Richter]

On Jan 12 Stephan Gatzka wrote:
> >    - There is another downside:  Each new driver method increases the
> >      memory footprint of instances of respective function pointer tables
> >      by 4 or 8 bytes.
> 
> I can't imagine that this is a show stopper. How many instances of 
> struct netdev do we have o a typical system? I guess not the much.

There aren't many instances, but these instances are used in hot paths
where cache utilization matters.  Of course such structures can be ordered
so that often used parts are located close to each other.
-- 
Stefan Richter
-=====-===-= ---= -==--
http://arcgraph.de/sr/

Re: IPv6 over firewire

[Stefan Richter]

On Jan 12 Stephan Gatzka wrote:
> > Or the (optional!) callback could be a new member of struct ndisc_options.
> 
> Hm, right now I have no opinion on that. Where does
> whatever_type *callback comes from? Is it an exported method of the 
> firewire net driver or the new function pointer in struct netdevice?

The networking core or IPv6 core would never ever use an EXPORT() from an
interface driver like firewire-net or from a bus architecture driver like
firewire-core.

The interface driver uses definitions and declarations from the networking/
IP/ ARP... core:
  - functions implemented in the core and EXPORTed from there,
  - data types defined in core headers and filled with data by the
    interface driver,
  - callback function types defined in core headers, used in data types
    which are defined in core headers; respective callbacks implemented in
    interface drivers, pointers to the implementations filled in by the
    interface drivers into respective data objects.
So the firewire-net bridge driver uses the firewire-core API below it and
the networking and IP APIs above it.

Whether a callback is needed at all is not obvious to me.  If one is
needed, then it is not obvious to me whether it should be reachable via a
function pointer in struct net_device, or via a function pointer in one of
the pointer table structs that are appended to struct net_device, or via a
function pointer in e.g. struct ndisc_options, or via a function pointer
typed argument of a function which is EXPORTed by the networking/ IPv6
core.

> > However, does net/ipv6/ndisc.c really need to be aware of the RFC 3146
> > Neighbor Discovery related packet header layouts?  Isn't it possible to
> > rewrite these headers in-place in drivers/firewire/net.c?
> 
> 
> Yes, it it possible, but yoshfuji strongly voted against rewriting ndisc 
> packets in firewire net driver to maintain extensibility to protocols. 
> Especially IPSEC can just not work if I rewrite the packets in the driver.

OK, then net/ipv6/ndisc.c shall be taught to write (and read?) those
packets, and drivers/firewire/net.c shall fill in all data that are going
to be needed by ndisc.c into new arguments of existing or new exported
core functions, or into new members of whichever networking struct
would be most suitable for that.  Or the networking core gets to those
data indirectly by calling a callback.

Apparently the Linux IPv6 core needs to learn a little bit about RFC 3146.
But it doesn't need to (and should not) learn anything about the Linux
IEEE 1394 implementation, that's what I am trying to convey. :-)
-- 
Stefan Richter
-=====-===-= ---= -==--
http://arcgraph.de/sr/

Re: IPv6 over firewire

[Stephan Gatzka]

> Apparently the Linux IPv6 core needs to learn a little bit about RFC 3146.
> But it doesn't need to (and should not) learn anything about the Linux
> IEEE 1394 implementation, that's what I am trying to convey. :-)
>

That is something I'm absolutely with you. :)