Generic HDLC interface continued

Generic HDLC interface continued

[Krzysztof Halasa]

Hi,

I just want to return to the generic HDLC ioctl interface issue.

Currently (in 2.5) we have:

struct ifreq 
{
  char ifrn_name[IFNAMSIZ];

  union {
    ...

    struct if_settings {
        unsigned int type;      /* Type of physical device or protocol */
        union {
            /* {atm/eth/dsl}_settings anyone ? */

            union hdlc_settings {
                raw_hdlc_proto          raw_hdlc;
                cisco_proto             cisco;
                fr_proto                fr;
                fr_proto_pvc            fr_pvc;
            }ifsu_hdlc;

            union line_settings {
                sync_serial_settings    sync;
                te1_settings            te1;
            }ifsu_line;
        } ifs_ifsu;

    } *ifru_settings;
  } ifr_ifru;
}

As the previous discussion showed, the ifs_ifsu union is just an
artificial structure - it's used only for the purpose of filling
the space in if_settings, as both sides (user and kernel) use
the members hdlc_settings or line_settings with their real length
(as opposed to the union length).

This situation creates some problems. First, IMHO the presence
of artificial union is a problem (one could think we're really using
the union and not variable length int type with *_proto / *_settings).

Second, the user-level utility has to supply the full length union
if it want to query the kernel (with IF_GET_PROTO etc) - as it doesn't
know what protocol has been set. It might not be a problem now, but
will be when a bigger member struct is added (imagine an ioctl setting
/getting a long encryption key etc).

Third, we are using complicated data types instead of simple flat ones,
which negatively affect code readability.


Solution:
struct ifreq 
{
    char ifrn_name[IFNAMSIZ];

    union ifr_ifru {
        ...
        struct {
            int type;
            int size;
            union {
                raw_hdlc_proto *;
                cisco_proto *;
                etc_proto *;
                sync_serial_settings *;
                etc_line_settings *;

Addressing the second problem (unknown data length) requires the caller
(user-space utils) to supply allocated space size. The kernel would
update the size to reflect the actual amount of data required, allowing
the caller to allocate more space and try again (or ignore the unknown
interface).

What is important here is that inner union consists of pointers
to *_proto / *_settings structs and not of the structs themselves.


Another solution - using a different ifreq structs for different tasks
(something like the sockaddr_*) - sort of:

struct ifreq_raw_hdlc_proto {
        char ifrn_name[IFNAMSIZ];
        int type;
        int size;
        raw_hdlc_proto settings;
};

struct ifreq_cisco_proto {
        char ifrn_name[IFNAMSIZ];
        int type;
        int size;
        raw_hdlc_proto settings;
};

The first 3 members would be common to all tasks (we would use a macro
for that, of course).


Comments?
-- 
Krzysztof Halasa
Network Administrator

Re: Generic HDLC interface continued

[Francois Romieu]

<disclaimer>
It's ok for me to trim the Cc: list if noboy objects.
Don't hesistate to ask for clarification if my english comes from Mars.
</disclaimer>

Krzysztof Halasa <khc@pm.waw.pl> :
[...]
> Addressing the second problem (unknown data length) requires the caller
> (user-space utils) to supply allocated space size. The kernel would
> update the size to reflect the actual amount of data required, allowing
> the caller to allocate more space and try again (or ignore the unknown
> interface).

If size/limit of an underlying object is in a structure for other purpose
than debygging, it means something (S) is working with an object and it (S)
doesn't know what it is. Design proble: always work with an object whose 
identity you know or simply pass a reference to someone knowing better.

I don't see why a 'size' parameter is required. Thus I'm fine with the
following (we are lucky enough that there is enough space in union ifr_ifru 
to hold ifru_settings):

struct ifreq
{
#define IFHWADDRLEN 6
#define IFNAMSIZ    16
    union
    {
        char    ifrn_name[IFNAMSIZ];        /* if name, e.g. "en0" */
    } ifr_ifrn;

    union {
        struct  sockaddr ifru_addr;
        struct  sockaddr ifru_dstaddr;
        struct  sockaddr ifru_broadaddr;
        struct  sockaddr ifru_netmask;
        struct  sockaddr ifru_hwaddr;
        short   ifru_flags;
        int ifru_ivalue;
        int ifru_mtu;
        struct  ifmap ifru_map;
        char    ifru_slave[IFNAMSIZ];   /* Just fits the size */
        char    ifru_newname[IFNAMSIZ];
        char *  ifru_data;
        struct {
             int type;
             union {
                 raw_hdlc_proto *;
                 ...
                 sync_serial_settings *;
                 etc_line_settings *;
			}
		} ifru_settings;
    } ifr_ifru;
};

Note however that struct ifreq on amphetamin (wrt lines of code) doesn't 
improve readability for everybody. That's a slightly different problem.

> What is important here is that inner union consists of pointers
> to *_proto / *_settings structs and not of the structs themselves.

Agree on this.

> Another solution - using a different ifreq structs for different tasks
> (something like the sockaddr_*) - sort of:
[...]

I am not too fond of this and, again, what are these 'size' for ?
If it's supposed to replace/duplicate ifreq, the 'settings' part should
be a pointer imho. Same reason as before: size change => compatibility 
problems for tools (we have sources but downgrading tools when returning to
previous kernel sucks).

-- 
Ueimor

Re: Generic HDLC interface continued

[Krzysztof Halasa]

Francois Romieu <romieu@cogenit.fr> writes:

> > Addressing the second problem (unknown data length) requires the caller
> > (user-space utils) to supply allocated space size. The kernel would
> > update the size to reflect the actual amount of data required, allowing
> > the caller to allocate more space and try again (or ignore the unknown
> > interface).
> 
> If size/limit of an underlying object is in a structure for other purpose
> than debygging, it means something (S) is working with an object and it (S)
> doesn't know what it is.
> Design proble: always work with an object whose 
> identity you know or simply pass a reference to someone knowing better.

Not exactly. The caller always knows meaning of the returned value
(or it reports error etc). The caller doesn't just know size of the value
_in_advance_, as it isn't constant. Still, meaning of the variable portion
of the data is defined by the constant part.

It won't be a problem if allocating space is a kernel task.

> struct ifreq
...

>         struct {
>              int type;
>              union {
>                  raw_hdlc_proto *;
>                  ...
>                  sync_serial_settings *;
>                  etc_line_settings *;
> 			}
> 		} ifru_settings;
>     } ifr_ifru;
> };
> 
> Note however that struct ifreq on amphetamin (wrt lines of code) doesn't 
> improve readability for everybody. That's a slightly different problem.

Of course, I didn't want to include all the member structs directly
in ifreq definition (in the header file). I just did it for the purpose
of discussion.

> > What is important here is that inner union consists of pointers
> > to *_proto / *_settings structs and not of the structs themselves.
> 
> Agree on this.
> 
> > Another solution - using a different ifreq structs for different tasks
> > (something like the sockaddr_*) - sort of:
> [...]
> 
> I am not too fond of this and, again, what are these 'size' for ?
> If it's supposed to replace/duplicate ifreq, the 'settings' part should
> be a pointer imho. Same reason as before: size change => compatibility 
> problems for tools (we have sources but downgrading tools when returning to
> previous kernel sucks).

Yes. That's why I want to finally have a well-designed interface,
with no need for changes in the future (except as needed for new
interfaces/protocols etc).
That's why I want to do it in 2.5 and then downport it to 2.4
and possibly even to 2.2.
-- 
Krzysztof Halasa
Network Administrator

Re: Generic HDLC interface continued

[Francois Romieu]

Krzysztof Halasa <khc@pm.waw.pl> :
[...]
> Not exactly. The caller always knows meaning of the returned value
> (or it reports error etc). The caller doesn't just know size of the value
> _in_advance_, as it isn't constant. Still, meaning of the variable portion
> of the data is defined by the constant part.

The caller doesn't know size in advance but he gets 'type' and 'size' at
the same time. Why shouldn't 'size' be deduced from 'type' ?

-- 
Ueimor

Re: Generic HDLC interface continued

[Krzysztof Halasa]

Francois Romieu <romieu@cogenit.fr> writes:

> > Not exactly. The caller always knows meaning of the returned value
> > (or it reports error etc). The caller doesn't just know size of the value
> > _in_advance_, as it isn't constant. Still, meaning of the variable portion
> > of the data is defined by the constant part.
> 
> The caller doesn't know size in advance but he gets 'type' and 'size' at
> the same time. Why shouldn't 'size' be deduced from 'type' ?

The caller don't know the result size. It mallocs some space which is
long enough for usual things. Now, we have new super protocol with 100 KB
of config data, and the caller requests settings of that protocol.

With "size" variable, the caller signals that there are 500 bytes for
the result, and the kernel tells it that 100 KB are required, and that
it's BTW the super protocol itself. The caller can allocate more space
and try again, or just fail if it doesn't know the protocol.

Without "size" variable, the kernel tries to copy 100 KB to random
space, and we get results ranging from SEGV to mysterious behavior.

Please note that there is no call which could return the protocol
(or interface type) without copying the whole data - thus the caller
(utility) is unable to skip unknown interfaces.
-- 
Krzysztof Halasa
Network Administrator

Re: Generic HDLC interface continued

[Francois Romieu]

Krzysztof Halasa <khc@pm.waw.pl> :
[...]
> Please note that there is no call which could return the protocol
> (or interface type) without copying the whole data - thus the caller
> (utility) is unable to skip unknown interfaces.

Ok, the 'type' attribute isn't enough.

I feel like we are trying to do two things at the same time:
a) the size of the allocated area isn't required if we need to do something
   real with the data: if size doesn't match what is expected, we loose 
   anyway
b) if we don't care about the copied data, we actually ask for the subtype
   of the interface

-> a) and b) are two different operations imho.

-- 
Ueimor

Re: Generic HDLC interface continued

[Krzysztof Halasa]

Francois Romieu <romieu@cogenit.fr> writes:

> Ok, the 'type' attribute isn't enough.
> 
> I feel like we are trying to do two things at the same time:
> a) the size of the allocated area isn't required if we need to do something
>    real with the data: if size doesn't match what is expected, we loose 
>    anyway
> b) if we don't care about the copied data, we actually ask for the subtype
>    of the interface
> 
> -> a) and b) are two different operations imho.

Depends on the point of view, but generally it might be true.
However, it is quite practical to do a+b in one call:
- utilities will always do it in exactly that order,
- we don't need to worry about races (process A gets type/size; process
  B changes protocol; process A gets invalid data).
-- 
Krzysztof Halasa
Network Administrator

Re: Generic HDLC interface continued

[Krzysztof Halasa]

Pavel Selivanov <pavel-linux-kernel@parabel.inc.ru> writes:

>  I'm sure that hdlc stack must provide only protocols support (and a
>  configuration utility to configure protocol's parameters only).
>  All other job is developer's problem (even crc type).

That's what the code does - the hdlc_*.c code knows only protocol
ioctls, and passes other things straight to hardware driver.

But the interface (structure definitions) must be the same for all
devices, as we don't want different tools for different cards.

>  While placing interface type in hdlc struct's isn't good,
>  but to place it in net_device is a good idea (I think).

I don't think so. Most cards (not necesarilly WAN ones) have only
one interface (type).

>  2) If I've understand author's ideology, he is going to implement
>  hdlc stack "near" current network stack.
>  I mean hdlc_device appends new fields to net_device, so hdlc device
>  is still the same device as ethernet, and that's fine.

I used to think of it as of inherited structure from, say, C++.

>  I dont understant what is it for (for this ideology)?
>  >>>>>>>>
>  if.h
> struct if_settings
> {
>         unsigned int type;      /* Type of physical device or protocol */
>         union {
>                 /* {atm/eth/dsl}_settings anyone ? */
>                 union hdlc_settings ifsu_hdlc;
>                 union line_settings ifsu_line;
>         } ifs_ifsu;
> };  
> .....
> in struct ifreq
>                 struct  if_settings *ifru_settings;
>  <<<<<<<<
> Is it really necessary ?
> At my opinion it would be better to use char *,

It was done that way at some point and then redesigned, but now I think
we should rather use just a union of struct pointers.

The hdlc/ioctl.h may seem to be a little overkill, I understand the
definitions are there for performance reasons (while building the kernel).

> If someone will have to support one more protocol, he have to modify
> hdlc.h (to add new struct in the union)...

Yes.

> As long as sethdlc tell to hdlc stack: I want ppp-over-fr, hdlc to:
> - search proto_list for ppp, search for fr.
> - attaches ppp to prot pointer in hdlc_device
> - calls fr->attach (which allocated priv pointer)
> - calls ppp->attach (which allocates priv pointer)
> - calls fr->enslave
> 
> This model seems bulki, but It's much more flexible (without changin
> API, what is very important at my opinion.)

That's not about changing API, that's only adding _new_ parts to the API,
it doesn't break anything. Anyway, the current code:
- exist
- is quite simple and thus reliable
- does the job.
So I'm not going to spend time on changing it once again, except for
the union change mentioned earlier (a cosmetic change).

> 5) It's bad to handle get_stats by hdlc stack.
> Some chips provides some info, which I shoud ask(for ifconfig)...

hdlc_* basically only increments rx_errors counter... We could drop
the get_stats() (move it to hw drivers) when we have a driver which
needs that. It's not a problem, it doesn't really change anything.

> It's very comfortable in cisco that I have full protocol's info
> (broadcasts, protocol errors, ...).

Cisco = IOS software or Cisco = protocol?
Yes, it would be better to have another set of stats for hw and another
one for upper layers. That's true for most interface types, not only
WAN HDLC-like ones.

Or should we add new fields to existing struct? Possibly.

> I just want for hdlc stack in Linux to be as usefull as it's
> possible, and to have a single API (HDLC API changing in second
> time...).

That's exactly what I want, too.
-- 
Krzysztof Halasa
Network Administrator

Re: Generic HDLC interface continued

[Francois Romieu]

Greetings,

Pavel Selivanov <pavel-linux-kernel@parabel.inc.ru> :
[...]
> 1) I think that hdlc stack should know nothing about my interface type
> (whether it's V.35, DSL, E1/T1, HPPI, ...).
>  I'm sure that hdlc stack must provide only protocols support (and a
>  configuration utility to configure protocol's parameters only).
>  All other job is developer's problem (even crc type).

Then every driver must implement it's own ioctl() and utility. 
Ahum...

[...]
>  Possible it's reasonable to make another utility like ethertool (called, for
>  example, e1tool, mdsltool,...) and complementary header (ioctls, structs,...)
>  for configuring interface(for E1: crc4, timeslots,
>  ...), and getting stats.

mv sethdlc.c hdlctool.c ?

>  2) If I've understand author's ideology, he is going to implement
>  hdlc stack "near" current network stack.
>  I mean hdlc_device appends new fields to net_device, so hdlc device
>  is still the same device as ethernet, and that's fine.

I'd rather have hdlc_device pointing to it's own net_device instead of 
embedding it directly but I won't fight too much about this (so far :o) ).

[struct if_settings and friends]
> Is it really necessary ?

Yes, it's used. It may/will be done slightly differently.

> At my opinion it would be better to use char *,

It provides less type-checking at compile time.

> and to define it in hdlc.h, hdlc/ioctl.h

{hdlc/line}_settings are defined in hdlc/ioctl.h
Others layer 2 protocols may use if_settings (they don't seem to be in a
hurry though :o) ).

[include/linux/if.h - IF_IFACE_XYZ/IF_PROTO_ABC]
> As I understand, it will be never able to make something like
> ppp-over-framerelay...
> But it is widely used, and (for example) negraph in xBSD provide's
> such functionality...

Imho hacking drivers/net/wan/hdlc_fr.c::fr_rx() and hdlc/ioctl.h isn't
the hardest part of ppp-over-fr for those who really need it.

[struct hdlc_device]
> If someone will have to support one more protocol, he have to modify
> hdlc.h (to add new struct in the union)...

Do you have a specific protocol in mind ?

> Why don't we do like this:
[snip, snip]

Admitting there is a heap of ppp-over-fr code waiting to be included and
even if someone is ready to code this all, I'm for a lazyer solution. :o)

-- 
Ueimor

RE: Generic HDLC interface continued

[Pavel Selivanov]

Hi.

I'm using hdlc stack for a E1 adapter since Aug. 2001,
and I have some suggestions/questions:
1) I think that hdlc stack should know nothing about my interface type
(whether it's V.35, DSL, E1/T1, HPPI, ...).
 I'm sure that hdlc stack must provide only protocols support (and a
 configuration utility to configure protocol's parameters only).
 All other job is developer's problem (even crc type).

 While placing interface type in hdlc struct's isn't good,
 but to place it in net_device is a good idea (I think).

 Possible it's reasonable to make another utility like ethertool (called, for
 example, e1tool, mdsltool,...) and complementary header (ioctls, structs,...)
 for configuring interface(for E1: crc4, timeslots,
 ...), and getting stats.

 2) If I've understand author's ideology, he is going to implement
 hdlc stack "near" current network stack.
 I mean hdlc_device appends new fields to net_device, so hdlc device
 is still the same device as ethernet, and that's fine.

 I dont understant what is it for (for this ideology)?
 >>>>>>>>
 if.h
struct if_settings
{
        unsigned int type;      /* Type of physical device or protocol */
        union {
                /* {atm/eth/dsl}_settings anyone ? */
                union hdlc_settings ifsu_hdlc;
                union line_settings ifsu_line;
        } ifs_ifsu;
};  
.....
in struct ifreq
                struct  if_settings *ifru_settings;
 <<<<<<<<
Is it really necessary ?
At my opinion it would be better to use char *,
and to define it in hdlc.h, hdlc/ioctl.h

3)
>>>>>>>>>>>
/* For definitions see hdlc.h */
#define IF_IFACE_V35    0x1000          /* V.35 serial interface        */
#define IF_IFACE_V24    0x1001          /* V.24 serial interface        */
#define IF_IFACE_X21    0x1002          /* X.21 serial interface        */
#define IF_IFACE_T1     0x1003          /* T1 telco serial interface    */
#define IF_IFACE_E1     0x1004          /* E1 telco serial interface    */
#define IF_IFACE_SYNC_SERIAL 0x1005     /* cant'b be set by software    */

/* For definitions see hdlc.h */
#define IF_PROTO_HDLC   0x2000          /* raw HDLC protocol            */
#define IF_PROTO_PPP    0x2001          /* PPP protocol                 */
#define IF_PROTO_CISCO  0x2002          /* Cisco HDLC protocol          */
#define IF_PROTO_FR     0x2003          /* Frame Relay protocol         */
#define IF_PROTO_FR_ADD_PVC 0x2004      /*    Create FR PVC             */
#define IF_PROTO_FR_DEL_PVC 0x2005      /*    Delete FR PVC             */
#define IF_PROTO_X25    0x2006          /* X.25                         */
<<<<<<<<<<<

As I understand, it will be never able to make something like
ppp-over-framerelay...
But it is widely used, and (for example) negraph in xBSD provide's
such functionality...

4)
>>>>>>>>>>>
        union {
                struct {
                        fr_proto settings;
                        pvc_device *first_pvc;
                        int pvc_count;

                        struct timer_list timer;
                        int last_poll;
                        int reliable;
                        int changed;
                        int request;
                        int fullrep_sent;
                        u32 last_errors; /* last errors bit list */
                        u8 n391cnt;
                        u8 txseq; /* TX sequence number */
                        u8 rxseq; /* RX sequence number */
                }fr;

                struct {
                        cisco_proto settings;

                        struct timer_list timer;
                        int last_poll;
                        int up;
                        u32 txseq; /* TX sequence number */
                        u32 rxseq; /* RX sequence number */
                }cisco;

                struct {
                        raw_hdlc_proto settings;
                }raw_hdlc;

                struct {
                        struct ppp_device pppdev;
                        struct ppp_device *syncppp_ptr;
                        int (*old_change_mtu)(struct net_device *dev,
                                              int new_mtu);
                }ppp;
        }state; 
<<<<<<<<<<<<<<<

If someone will have to support one more protocol, he have to modify
hdlc.h (to add new struct in the union)...
Why don't we do like this:

struct hdlc_proto{
       attach
       detach
       rcv
       enslave
       void *priv; //Protocol's specific data
       char proto_type; //protocol (ppp, fr,...) in a chain
       struct proto *next;
};
struct hdlc_proto *proto_list;//Supported protocol's list.

typedef struct hdlc_device_struct {
....
    struct hdlc_proto *prot; //pointer to current protocol's struct
}

As long as sethdlc tell to hdlc stack: I want ppp-over-fr, hdlc to:
- search proto_list for ppp, search for fr.
- attaches ppp to prot pointer in hdlc_device
- calls fr->attach (which allocated priv pointer)
- calls ppp->attach (which allocates priv pointer)
- calls fr->enslave

This model seems bulki, but It's much more flexible (without changin
API, what is very important at my opinion.)

5) It's bad to handle get_stats by hdlc stack.
Some chips provides some info, which I shoud ask(for ifconfig)...

I think hdlc should care own statistics (specific for every
protocol).

It's very comfortable in cisco that I have full protocol's info
(broadcasts, protocol errors, ...).
Hdlc stack must take no care on HW errors, it's my problem, but it
must have it's own statistics.

Sorry for my criticism.
My opinion isn't obsolete, I'm sure I'm wrong in many cases :-)
I just want for hdlc stack in Linux to be as usefull as it's
possible, and to have a single API (HDLC API changing in second
time...).

If I've understant something incorrectle - please tell me.

PS: Sorry for my English.

RE: Generic HDLC interface continued

[Kevin Curtis]

Hi,
	sorry that I am miles behind the discussion.  I'll try and keep up
in future.  I think I raised this with Krzysztof on Friday when I noted that
hdlc-2.4.19a.patch seemed to change the configuration interface, and the
utility we use to configure the FarSync card now no longer worked.

	We used the type, data pointer and length in the following way:

If the type indicated that the information was for the card (i.e. not the
driver), then this was passed by a pointer and length to the driver, which
then copied it into shared memory so that the card could then pick it up.
The format of the information was between the utility and the card itself.

Now, when I looked at mapping this into the new structure I had a bit of
difficulty.  I asked if the mechanism could be re-instated.

Surely we could find room for these three items somewhere in the new
structure???

struct if_settings
{
	unsigned int type;	/* Type of physical device or protocol */
	unsigned int data_length; /* device/protocol data length */
	void * data;		/* pointer to data, ignored if length = 0 */
};



Kevin

-----Original Message-----
From: Francois Romieu [mailto:romieu@cogenit.fr]
Sent: 30 September 2002 21:55
To: linux-kernel@vger.kernel.org
Subject: Re: Generic HDLC interface continued


Krzysztof Halasa <khc@pm.waw.pl> :
[...]
> Not exactly. The caller always knows meaning of the returned value
> (or it reports error etc). The caller doesn't just know size of the value
> _in_advance_, as it isn't constant. Still, meaning of the variable portion
> of the data is defined by the constant part.

The caller doesn't know size in advance but he gets 'type' and 'size' at
the same time. Why shouldn't 'size' be deduced from 'type' ?

-- 
Ueimor
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/