WAN: new PPP code for generic HDLC - Kernel

This is a discussion on WAN: new PPP code for generic HDLC - Kernel ; From: Krzysztof Halasa Date: Thu, 24 Apr 2008 02:48:29 +0200 > David Miller writes: > > > No, it was added as an optimization since the private > > area was allocated together with the struct netdev, and > > ...

+ Reply to Thread
Page 3 of 3 FirstFirst 1 2 3
Results 41 to 55 of 55

Thread: WAN: new PPP code for generic HDLC

  1. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    From: Krzysztof Halasa
    Date: Thu, 24 Apr 2008 02:48:29 +0200

    > David Miller writes:
    >
    > > No, it was added as an optimization since the private
    > > area was allocated together with the struct netdev, and
    > > thus at a constant offset computable a compile time.

    >
    > That's essentially what I meant. And it has changed, call it whatever
    > you wish.


    The core kernel code sets dev->priv to the alloc_netdev() allocated
    memory region.

    It doesn't do that just for fun.

    Any code which overrides that setting is asking for trouble.
    What if the code kernel code that set it, needed to use it
    during device release for some reason?
    --
    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/

  2. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    David Miller writes:

    > The core kernel code sets dev->priv to the alloc_netdev() allocated
    > memory region.
    >
    > It doesn't do that just for fun.
    >
    > Any code which overrides that setting is asking for trouble.
    > What if the code kernel code that set it, needed to use it
    > during device release for some reason?


    You are right, the point is Linux is a moving target, and I try to
    write code as good as I can in given circumstances but never better.
    I accept occasional breakage in obscure cases like syncppp+hdlc,
    but I also need a way to fix it and/or remove the obscureness (or
    whatever the right word is).

    Historically we always have had two pointers:
    - dev->priv
    - first we had struct (net_)device *dev and by embedding it in larger
    structure we were able to have another private data area (even with
    Space.c)
    - then netdev_alloc() and netdev_priv() came
    - since 2.6.23 netdev_priv() returns dev->priv - one pointer is gone.

    Anyway, I guess we should rather concentrate on what to do now with
    the thing. Perhaps you have some idea?
    --
    Krzysztof Halasa
    --
    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/

  3. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    From: Krzysztof Halasa
    Date: Thu, 24 Apr 2008 15:12:17 +0200

    > Anyway, I guess we should rather concentrate on what to do now with
    > the thing. Perhaps you have some idea?


    My current plan is to add an "official" mid-layer pointer for
    these purposes. So that the bug can have a simple fix while
    we think about how better to handle this longer term.
    --
    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/

  4. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    David Miller writes:

    > My current plan is to add an "official" mid-layer pointer for
    > these purposes. So that the bug can have a simple fix while
    > we think about how better to handle this longer term.


    That would be great first step.

    Though we will have to abandon syncppp use anyway, at least for
    generic HDLC (I don't care much about the old ISA drivers).
    --
    Krzysztof Halasa
    --
    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/

  5. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    From: Krzysztof Halasa
    Date: Thu, 24 Apr 2008 15:39:47 +0200

    > Though we will have to abandon syncppp use anyway, at least for
    > generic HDLC (I don't care much about the old ISA drivers).


    That, I still don't understand.

    I would rather see you add small extensions to syncppp to
    provide for HDLC's needs, rather than reimplement it.
    --
    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/

  6. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    David Miller writes:

    > I would rather see you add small extensions to syncppp to
    > provide for HDLC's needs, rather than reimplement it.


    It's not about extensions, syncppp is just unmaintainable. It's
    implementation of two protocols - Cisco HDLC and (sync) PPP intermixed
    together.

    What would be needed is a) splitting them into two files (including
    protocol selection in drivers - somehow), b) replacing existing
    broken syncppp state machines distributed all over the place with a
    single sane state machine, c) perhaps adding IPv6, people want it,
    d) packet flow from hw drivers would need to be upgraded as well
    (*_type_trans() and fast path for IP/IPv6 and perhaps IPX, though
    I don't know anyone ever using IPX with sync PPP).

    I guess then you'd get generic HDLC.

    I think it's easier to a) simply remove syncppp.c, b) apply my patch
    with new PPP, c) blindly port the old drivers to generic HDLC,
    d) hope for the best, with a bit of luck nobody would notice.

    Alternative c) simply remove the old drivers (d) unchanged).

    I don't want the breakage resulting from either action thus I think
    syncppp.c should stay there (basically unchanged, though we could
    at last remove those variables used only by the original FreeBSD
    code in 1994).

    If the new implementation is my patch or if it uses generic PPP is
    debatable. I think simplicity is a good thing, the interface to
    generic PPP would be bigger than the whole sync-only PPP. But I'm not
    against using generic PPP, it's (AFAICS) clean and *working* and
    *maintained* (and I'm not worried about backward compatibility and
    certainly not about users' visions).
    --
    Krzysztof Halasa
    --
    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/

  7. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    > But I'm not
    > against using generic PPP, it's (AFAICS) clean and *working* and
    > *maintained* (and I'm not worried about backward compatibility and
    > certainly not about users' visions).


    IOW I value correctness over backward compatibility and users'
    visions, obviously I try to keep compatibility and meet users'
    needs when it actually makes sense.
    --
    Krzysztof Halasa
    --
    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/

  8. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    > I think it's easier to a) simply remove syncppp.c, b) apply my patch
    > with new PPP, c) blindly port the old drivers to generic HDLC,
    > d) hope for the best, with a bit of luck nobody would notice.


    I would agree with this. There isn't a sane way to sort out the old
    kernel side syncppp (which is useless for todays networks as it doesn't
    do compression, IPv6 or auth protocols) except by going to user space.
    --
    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/

  9. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    Alan Cox writes:

    > I would agree with this. There isn't a sane way to sort out the old
    > kernel side syncppp (which is useless for todays networks as it doesn't
    > do compression, IPv6 or auth protocols) except by going to user space.


    Well, I never used compression or auth on fixed line, and that would
    require generic PPP. Perhaps we should use it, I don't know.
    --
    Krzysztof Halasa
    --
    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/

  10. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    From: Alan Cox
    Date: Thu, 24 Apr 2008 21:44:57 +0100

    > > I think it's easier to a) simply remove syncppp.c, b) apply my patch
    > > with new PPP, c) blindly port the old drivers to generic HDLC,
    > > d) hope for the best, with a bit of luck nobody would notice.

    >
    > I would agree with this. There isn't a sane way to sort out the old
    > kernel side syncppp (which is useless for todays networks as it doesn't
    > do compression, IPv6 or auth protocols) except by going to user space.


    Ok, but what we have in the tree should at least not be a crashing
    mess that has to be marked BROKEN meanwhile :-)

    This patch should address that issue.

    Longer term I hope someone works on the infrastructure necessary that
    would allow us to delete this and use generic PPP kernel+userspace
    over these links.

    Scanning over some of these older WAN drivers was truly scary. No
    locking at all over the HDLC protocol list, yikes!

    commit 4951704b4e23d71b99ac933d8e6993bc6225ac13
    Author: David S. Miller
    Date: Mon May 12 03:29:11 2008 -0700

    syncppp: Fix crashes.

    The syncppp layer wants a mid-level netdev private pointer.

    It was using netdev->priv but that only worked by accident,
    and thus this scheme was broken when the device private
    allocation strategy changed.

    Add a proper mid-layer private pointer for uses like this,
    update syncppp and all users, and remove the HDLC_PPP broken
    tag from drivers/net/wan/Kconfig

    Signed-off-by: David S. Miller

    diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig
    index 8005dd1..d5140ae 100644
    --- a/drivers/net/wan/Kconfig
    +++ b/drivers/net/wan/Kconfig
    @@ -150,11 +150,9 @@ config HDLC_FR

    config HDLC_PPP
    tristate "Synchronous Point-to-Point Protocol (PPP) support"
    - depends on HDLC && BROKEN
    + depends on HDLC
    help
    Generic HDLC driver supporting PPP over WAN connections.
    - This module is currently broken and will cause a kernel panic
    - when a device configured in PPP mode is activated.

    It will be replaced by new PPP implementation in Linux 2.6.26.

    diff --git a/drivers/net/wan/cosa.c b/drivers/net/wan/cosa.c
    index 45ddfc9..b0fce13 100644
    --- a/drivers/net/wan/cosa.c
    +++ b/drivers/net/wan/cosa.c
    @@ -629,7 +629,7 @@ static void sppp_channel_init(struct channel_data *chan)
    d->base_addr = chan->cosa->datareg;
    d->irq = chan->cosa->irq;
    d->dma = chan->cosa->dma;
    - d->priv = chan;
    + d->ml_priv = chan;
    sppp_attach(&chan->pppdev);
    if (register_netdev(d)) {
    printk(KERN_WARNING "%s: register_netdev failed.\n", d->name);
    @@ -650,7 +650,7 @@ static void sppp_channel_delete(struct channel_data *chan)

    static int cosa_sppp_open(struct net_device *d)
    {
    - struct channel_data *chan = d->priv;
    + struct channel_data *chan = d->ml_priv;
    int err;
    unsigned long flags;

    @@ -690,7 +690,7 @@ static int cosa_sppp_open(struct net_device *d)

    static int cosa_sppp_tx(struct sk_buff *skb, struct net_device *dev)
    {
    - struct channel_data *chan = dev->priv;
    + struct channel_data *chan = dev->ml_priv;

    netif_stop_queue(dev);

    @@ -701,7 +701,7 @@ static int cosa_sppp_tx(struct sk_buff *skb, struct net_device *dev)

    static void cosa_sppp_timeout(struct net_device *dev)
    {
    - struct channel_data *chan = dev->priv;
    + struct channel_data *chan = dev->ml_priv;

    if (test_bit(RXBIT, &chan->cosa->rxtx)) {
    chan->stats.rx_errors++;
    @@ -720,7 +720,7 @@ static void cosa_sppp_timeout(struct net_device *dev)

    static int cosa_sppp_close(struct net_device *d)
    {
    - struct channel_data *chan = d->priv;
    + struct channel_data *chan = d->ml_priv;
    unsigned long flags;

    netif_stop_queue(d);
    @@ -800,7 +800,7 @@ static int sppp_tx_done(struct channel_data *chan, int size)

    static struct net_device_stats *cosa_net_stats(struct net_device *dev)
    {
    - struct channel_data *chan = dev->priv;
    + struct channel_data *chan = dev->ml_priv;
    return &chan->stats;
    }

    @@ -1217,7 +1217,7 @@ static int cosa_sppp_ioctl(struct net_device *dev, struct ifreq *ifr,
    int cmd)
    {
    int rv;
    - struct channel_data *chan = dev->priv;
    + struct channel_data *chan = dev->ml_priv;
    rv = cosa_ioctl_common(chan->cosa, chan, cmd, (unsigned long)ifr->ifr_data);
    if (rv == -ENOIOCTLCMD) {
    return sppp_do_ioctl(dev, ifr, cmd);
    diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
    index 10396d9..0030833 100644
    --- a/drivers/net/wan/hdlc_ppp.c
    +++ b/drivers/net/wan/hdlc_ppp.c
    @@ -45,7 +45,7 @@ static int ppp_open(struct net_device *dev)
    int (*old_ioctl)(struct net_device *, struct ifreq *, int);
    int result;

    - dev->priv = &state(hdlc)->syncppp_ptr;
    + dev->ml_priv = &state(hdlc)->syncppp_ptr;
    state(hdlc)->syncppp_ptr = &state(hdlc)->pppdev;
    state(hdlc)->pppdev.dev = dev;

    diff --git a/drivers/net/wan/hostess_sv11.c b/drivers/net/wan/hostess_sv11.c
    index 83dbc92..f3065d3 100644
    --- a/drivers/net/wan/hostess_sv11.c
    +++ b/drivers/net/wan/hostess_sv11.c
    @@ -75,7 +75,7 @@ static void hostess_input(struct z8530_channel *c, struct sk_buff *skb)

    static int hostess_open(struct net_device *d)
    {
    - struct sv11_device *sv11=d->priv;
    + struct sv11_device *sv11=d->ml_priv;
    int err = -1;

    /*
    @@ -128,7 +128,7 @@ static int hostess_open(struct net_device *d)

    static int hostess_close(struct net_device *d)
    {
    - struct sv11_device *sv11=d->priv;
    + struct sv11_device *sv11=d->ml_priv;
    /*
    * Discard new frames
    */
    @@ -159,14 +159,14 @@ static int hostess_close(struct net_device *d)

    static int hostess_ioctl(struct net_device *d, struct ifreq *ifr, int cmd)
    {
    - /* struct sv11_device *sv11=d->priv;
    + /* struct sv11_device *sv11=d->ml_priv;
    z8530_ioctl(d,&sv11->sync.chanA,ifr,cmd) */
    return sppp_do_ioctl(d, ifr,cmd);
    }

    static struct net_device_stats *hostess_get_stats(struct net_device *d)
    {
    - struct sv11_device *sv11=d->priv;
    + struct sv11_device *sv11=d->ml_priv;
    if(sv11)
    return z8530_get_stats(&sv11->sync.chanA);
    else
    @@ -179,7 +179,7 @@ static struct net_device_stats *hostess_get_stats(struct net_device *d)

    static int hostess_queue_xmit(struct sk_buff *skb, struct net_device *d)
    {
    - struct sv11_device *sv11=d->priv;
    + struct sv11_device *sv11=d->ml_priv;
    return z8530_queue_xmit(&sv11->sync.chanA, skb);
    }

    @@ -325,6 +325,7 @@ static struct sv11_device *sv11_init(int iobase, int irq)
    /*
    * Initialise the PPP components
    */
    + d->ml_priv = sv;
    sppp_attach(&sv->netdev);

    /*
    @@ -333,7 +334,6 @@ static struct sv11_device *sv11_init(int iobase, int irq)

    d->base_addr = iobase;
    d->irq = irq;
    - d->priv = sv;

    if(register_netdev(d))
    {
    diff --git a/drivers/net/wan/lmc/lmc_main.c b/drivers/net/wan/lmc/lmc_main.c
    index 6635ece..62133ce 100644
    --- a/drivers/net/wan/lmc/lmc_main.c
    +++ b/drivers/net/wan/lmc/lmc_main.c
    @@ -891,6 +891,7 @@ static int __devinit lmc_init_one(struct pci_dev *pdev,

    /* Initialize the sppp layer */
    /* An ioctl can cause a subsequent detach for raw frame interface */
    + dev->ml_priv = sc;
    sc->if_type = LMC_PPP;
    sc->check = 0xBEAFCAFE;
    dev->base_addr = pci_resource_start(pdev, 0);
    diff --git a/drivers/net/wan/sealevel.c b/drivers/net/wan/sealevel.c
    index 11276bf..44a89df 100644
    --- a/drivers/net/wan/sealevel.c
    +++ b/drivers/net/wan/sealevel.c
    @@ -241,6 +241,7 @@ static inline struct slvl_device *slvl_alloc(int iobase, int irq)
    return NULL;

    sv = d->priv;
    + d->ml_priv = sv;
    sv->if_ptr = &sv->pppdev;
    sv->pppdev.dev = d;
    d->base_addr = iobase;
    diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
    index 7c1d446..7469017 100644
    --- a/include/linux/netdevice.h
    +++ b/include/linux/netdevice.h
    @@ -715,6 +715,9 @@ struct net_device
    struct net *nd_net;
    #endif

    + /* mid-layer private */
    + void *ml_priv;
    +
    /* bridge stuff */
    struct net_bridge_port *br_port;
    /* macvlan */
    diff --git a/include/net/syncppp.h b/include/net/syncppp.h
    index 877efa4..e43f407 100644
    --- a/include/net/syncppp.h
    +++ b/include/net/syncppp.h
    @@ -59,7 +59,7 @@ struct ppp_device

    static inline struct sppp *sppp_of(struct net_device *dev)
    {
    - struct ppp_device **ppp = dev->priv;
    + struct ppp_device **ppp = dev->ml_priv;
    BUG_ON((*ppp)->dev != dev);
    return &(*ppp)->sppp;
    }
    --
    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/

  11. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    David Miller writes:

    > No
    > locking at all over the HDLC protocol list, yikes!


    Right, I didn't think about ioctl() vs insmod/rmmod races. Will fix
    when I'm back home and can test it, in few days.
    --
    Krzysztof Halasa
    --
    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/

  12. Re: [PATCH v2] Re: WAN: new PPP code for generic HDLC

    BTW: The dev->ml_priv addition is obviously a good step but I guess
    you don't need the dev->priv hackery duplicated with dev->ml_priv.

    Can't test it at the moment but I'd leave dev->priv alone and only get
    rid of the syncppp_ptr here. Then I'd simply use dev->ml_priv as
    syncppp_ptr and dev->priv would be a private driver pointer, as
    usual. Generic HDLC uses dev->priv for itself but the relevant hw
    drivers use hdlc->priv instead so it's not a problem.

    When syncppp.c is removed we can use dev->ml_priv for HDLC layer,
    dev->priv for hw driver, and hdlc->priv would be no longer needed.

    I mean the following:

    David Miller writes:

    > --- a/include/net/syncppp.h
    > +++ b/include/net/syncppp.h
    > @@ -59,7 +59,7 @@ struct ppp_device
    >
    > static inline struct sppp *sppp_of(struct net_device *dev)
    > {
    > - struct ppp_device **ppp = dev->priv;
    > + struct ppp_device **ppp = dev->ml_priv;
    > BUG_ON((*ppp)->dev != dev);
    > return &(*ppp)->sppp;
    > }


    would become:

    static inline struct sppp *sppp_of(struct net_device *dev)
    {
    return dev->ml_priv;
    }

    The changes are not automatic, I'll look at it when able.
    --
    Krzysztof Halasa
    --
    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/

  13. [PATCH] WAN: protect HDLC proto list while insmod/rmmod

    WAN: protect protocol list in hdlc.c with RTNL.

    Signed-off-by: Krzysztof Hałasa

    --- a/drivers/net/wan/hdlc.c
    +++ b/drivers/net/wan/hdlc.c
    @@ -42,8 +42,7 @@ static const char* version = "HDLC support module revision 1.22";

    #undef DEBUG_LINK

    -static struct hdlc_proto *first_proto = NULL;
    -
    +static struct hdlc_proto *first_proto;

    static int hdlc_change_mtu(struct net_device *dev, int new_mtu)
    {
    @@ -313,21 +312,25 @@ void detach_hdlc_protocol(struct net_device *dev)

    void register_hdlc_protocol(struct hdlc_proto *proto)
    {
    + rtnl_lock();
    proto->next = first_proto;
    first_proto = proto;
    + rtnl_unlock();
    }


    void unregister_hdlc_protocol(struct hdlc_proto *proto)
    {
    - struct hdlc_proto **p = &first_proto;
    - while (*p) {
    - if (*p == proto) {
    - *p = proto->next;
    - return;
    - }
    + struct hdlc_proto **p;
    +
    + rtnl_lock();
    + p = &first_proto;
    + while (*p != proto) {
    + BUG_ON(!*p);
    p = &((*p)->next);
    }
    + *p = proto->next;
    + rtnl_unlock();
    }


    --
    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/

  14. Re: [PATCH] WAN: protect HDLC proto list while insmod/rmmod

    From: Krzysztof Halasa
    Date: Mon, 19 May 2008 19:00:51 +0200

    > WAN: protect protocol list in hdlc.c with RTNL.
    >
    > Signed-off-by: Krzysztof Hałasa


    Acked-by: David S. Miller
    --
    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/

  15. Re: [PATCH] WAN: protect HDLC proto list while insmod/rmmod

    Krzysztof Halasa wrote:
    > WAN: protect protocol list in hdlc.c with RTNL.
    >
    > Signed-off-by: Krzysztof Hałasa


    applied


    --
    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/

+ Reply to Thread
Page 3 of 3 FirstFirst 1 2 3