linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Generic HDLC interface continued
@ 2002-09-27 21:45 Krzysztof Halasa
  2002-09-28 18:21 ` Francois Romieu
  0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Halasa @ 2002-09-27 21:45 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alan Cox, Jeff Garzik

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

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: Generic HDLC interface continued
@ 2002-10-02 16:30 Kevin Curtis
  0 siblings, 0 replies; 11+ messages in thread
From: Kevin Curtis @ 2002-10-02 16:30 UTC (permalink / raw)
  To: 'Francois Romieu', linux-kernel

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/

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: Generic HDLC interface continued
@ 2002-10-10 16:27 Pavel Selivanov
  2002-10-10 19:08 ` Francois Romieu
  2002-10-15 20:20 ` Krzysztof Halasa
  0 siblings, 2 replies; 11+ messages in thread
From: Pavel Selivanov @ 2002-10-10 16:27 UTC (permalink / raw)
  To: linux-kernel

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2002-10-16 13:37 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2002-09-27 21:45 Generic HDLC interface continued Krzysztof Halasa
2002-09-28 18:21 ` Francois Romieu
2002-09-29 13:49   ` Krzysztof Halasa
2002-09-30 20:54     ` Francois Romieu
2002-09-30 23:48       ` Krzysztof Halasa
2002-10-01 18:01         ` Francois Romieu
2002-10-01 22:09           ` Krzysztof Halasa
2002-10-02 16:30 Kevin Curtis
2002-10-10 16:27 Pavel Selivanov
2002-10-10 19:08 ` Francois Romieu
2002-10-15 20:20 ` Krzysztof Halasa

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).