login
Search
Hi, I'll follow up with a patch for generic HDLC PPP code. Current status of generic HDLC is: - up to 2.6.22 - working. - 2.6.23.* and 2.6.24.* - Frame Relay and PPP protocols panic instantly, breakage caused by a change to netdev code that I overlooked. - 2.6.25-git - Frame Relay is already fixed, PPP still panics. The fix to FR was simple (983e23041b28abb113862b2935a85cfb9aab4f5a). PPP has been using syncppp.c implementation for years, meaning working as a mid-layer between hardware drivers and syncppp. This situation is increasingly hard to maintain, and I've decided to write a dedicated PPP implementation for generic HDLC. Rationale: - syncppp assumes no mid-layer between itself and hw drivers - dirty hacks must be used to work around this - syncppp doesn't even try to implement PPP correctly, it looks like it was written to work with another specific implementation and then left in this state for *teen years - every protocol (LCP and IPCP) uses different state machine code. - lack of IPv6 support and adding it is non-trivial. - I've considered using the generic PPP stack, however it's oriented towards async tty and requires the pppd daemon - I guess code to interface to it would be more complicated than the new PPP code. The PPP state machine is distributed over many functions and this is extremely hard to maintain or even understand. No wonder it's not very standard-compliant. Instead my new implementation has: - a single state machine code for all control protocols (LCP, IPCP, IPV6CP, and whatever is needed) - it's modelled after STD-51 - even more minimalistic than syncppp (no "magic number"), though adding optional support (if needed) is simple. - can actually be maintained. I have positively tested it on i686 and big-endian ARM, against itself and against syncppp.c. I can't test it against any other device due to -ENOHW, so I expect some bugs/problems in/with this code, though I hope it's better than the current brokenness. I ...
New synchronous PPP implementation for generic HDLC.
Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>
diff --git a/drivers/net/wan/Makefile b/drivers/net/wan/Makefile
index d61fef3..3081683 100644
--- a/drivers/net/wan/Makefile
+++ b/drivers/net/wan/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_HDLC_RAW) += hdlc_raw.o
obj-$(CONFIG_HDLC_RAW_ETH) += hdlc_raw_eth.o
obj-$(CONFIG_HDLC_CISCO) += hdlc_cisco.o
obj-$(CONFIG_HDLC_FR) += hdlc_fr.o
-obj-$(CONFIG_HDLC_PPP) += hdlc_ppp.o syncppp.o
+obj-$(CONFIG_HDLC_PPP) += hdlc_ppp.o
obj-$(CONFIG_HDLC_X25) += hdlc_x25.o
pc300-y := pc300_drv.o
diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 10396d9..b626aae 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -2,7 +2,7 @@
* Generic HDLC support routines for Linux
* Point-to-point protocol support
*
- * Copyright (C) 1999 - 2006 Krzysztof Halasa <khc@pm.waw.pl>
+ * Copyright (C) 1999 - 2008 Krzysztof Halasa <khc@pm.waw.pl>
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of version 2 of the GNU General Public License
@@ -19,87 +19,629 @@
#include <linux/skbuff.h>
#include <linux/pkt_sched.h>
#include <linux/inetdevice.h>
-#include <linux/lapb.h>
-#include <linux/rtnetlink.h>
#include <linux/hdlc.h>
-#include <net/syncppp.h>
+#include <linux/spinlock.h>
+
+#define DEBUG_CP 0 /* also bytes# to dump */
+#define DEBUG_STATE 0
+#define DEBUG_HARD_HEADER 0
+
+#define HDLC_ADDR_ALLSTATIONS 0xFF
+#define HDLC_CTRL_UI 0x03
+
+#define PID_LCP 0xC021
+#define PID_IP 0x0021
+#define PID_IPCP 0x8021
+#define PID_IPV6 0x0057
+#define PID_IPV6CP 0x8057
+
+enum {IDX_LCP = 0, IDX_IPCP, IDX_IPV6CP, IDX_COUNT};
+enum {CP_CONF_REQ = 1, CP_CONF_ACK, CP_CONF_NAK, CP_CONF_REJ, CP_TERM_REQ,
+ CP_TERM_ACK, CP_CODE_REJ, LCP_PROTO_REJ, LCP_ECHO_REQ, LCP_ECHO_REPLY,
+ LCP_DISC_REQ, CP_CODES};
+#if DEBUG_CP
+static const char *code_names[CP_CODES] = {"0", ...If I remember correctly, the packed is unnecessary for structures like this and causes GCC to generate worse code. --
Interesting. Gcc obviously will do the right thing without "packed", at least on "current" architectures, but I don't know what the C standard says. I will remove it, thanks. -- Krzysztof Halasa --
What if skb->data happens to be unaligned (can that even happen?)
BTW, here are some consts:
diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index b626aae..0e4cb1f 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -40,7 +40,7 @@ enum {CP_CONF_REQ = 1, CP_CONF_ACK, CP_CONF_NAK, CP_CONF_REJ, CP_TERM_REQ,
CP_TERM_ACK, CP_CODE_REJ, LCP_PROTO_REJ, LCP_ECHO_REQ, LCP_ECHO_REPLY,
LCP_DISC_REQ, CP_CODES};
#if DEBUG_CP
-static const char *code_names[CP_CODES] = {"0", "ConfReq", "ConfAck",
+static const char *const code_names[] = {"0", "ConfReq", "ConfAck",
"ConfNak", "ConfRej", "TermReq",
"TermAck", "CodeRej", "ProtoRej",
"EchoReq", "EchoReply", "Discard"};
@@ -90,10 +90,10 @@ enum {INV = 0x10, IRC = 0x20, ZRC = 0x40, SCR = 0x80, SCA = 0x100,
SCN = 0x200, STR = 0x400, STA = 0x800, SCJ = 0x1000};
#if DEBUG_STATE
-static const char *state_names[STATES] = {"Closed", "Stopped", "Stopping",
+static const char *const state_names[] = {"Closed", "Stopped", "Stopping",
"ReqSent", "AckRecv", "AckSent",
"Opened"};
-static const char *event_names[EVENTS] = {"Start", "Stop", "TO+", "TO-",
+static const char *const event_names[] = {"Start", "Stop", "TO+", "TO-",
"RCR+", "RCR-", "RCA", "RCN",
"RTR", "RTA", "RUC", "RXJ+", "RXJ-"};
#endif
@@ -374,7 +374,7 @@ static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
static void ppp_cp_parse_cr(struct net_device *dev, u16 pid, u8 id,
unsigned int len, u8 *data)
{
- static u8 valid_accm[6] = { LCP_OPTION_ACCM, 6, 0, 0, 0, 0 };
+ static const u8 valid_accm[] = { LCP_OPTION_ACCM, 6, 0, 0, 0, 0 };
u8 *opt, *out;
unsigned int nak_len = 0, rej_len = 0;
--
It can't if the hdlc header is 32-bit aligned... however, I can now see the above is buggy, skb->data doesn't point to the protocol The explicit sizes are there as a prevention. Thanks. -- Krzysztof Halasa --
Actually I was wrong, the code is correct and skb->data points to the proto ID. PATCH v2 is on the way. -- Krzysztof Halasa --
You can keep them if you need, the main issue was const anyway. --
New synchronous PPP implementation for generic HDLC.
Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>
drivers/net/wan/Makefile | 2 +-
drivers/net/wan/hdlc_ppp.c | 649 ++++++++++++++++++++++++++++++++++++++++----
2 files changed, 602 insertions(+), 49 deletions(-)
diff --git a/drivers/net/wan/Makefile b/drivers/net/wan/Makefile
index d61fef3..3081683 100644
--- a/drivers/net/wan/Makefile
+++ b/drivers/net/wan/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_HDLC_RAW) += hdlc_raw.o
obj-$(CONFIG_HDLC_RAW_ETH) += hdlc_raw_eth.o
obj-$(CONFIG_HDLC_CISCO) += hdlc_cisco.o
obj-$(CONFIG_HDLC_FR) += hdlc_fr.o
-obj-$(CONFIG_HDLC_PPP) += hdlc_ppp.o syncppp.o
+obj-$(CONFIG_HDLC_PPP) += hdlc_ppp.o
obj-$(CONFIG_HDLC_X25) += hdlc_x25.o
pc300-y := pc300_drv.o
diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 10396d9..9fc20fb 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -2,7 +2,7 @@
* Generic HDLC support routines for Linux
* Point-to-point protocol support
*
- * Copyright (C) 1999 - 2006 Krzysztof Halasa <khc@pm.waw.pl>
+ * Copyright (C) 1999 - 2008 Krzysztof Halasa <khc@pm.waw.pl>
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of version 2 of the GNU General Public License
@@ -19,87 +19,631 @@
#include <linux/skbuff.h>
#include <linux/pkt_sched.h>
#include <linux/inetdevice.h>
-#include <linux/lapb.h>
-#include <linux/rtnetlink.h>
#include <linux/hdlc.h>
-#include <net/syncppp.h>
+#include <linux/spinlock.h>
+
+#define DEBUG_CP 0 /* also bytes# to dump */
+#define DEBUG_STATE 0
+#define DEBUG_HARD_HEADER 0
+
+#define HDLC_ADDR_ALLSTATIONS 0xFF
+#define HDLC_CTRL_UI 0x03
+
+#define PID_LCP 0xC021
+#define PID_IP 0x0021
+#define PID_IPCP 0x8021
+#define PID_IPV6 0x0057
+#define PID_IPV6CP 0x8057
+
+enum {IDX_LCP = 0, IDX_IPCP, IDX_IPV6CP, IDX_COUNT};
+enum {CP_CONF_REQ = 1, CP_CONF_ACK, CP_CONF_NAK, CP_CONF_REJ, CP_TERM_REQ,
+ ...Hi, So are we leaving the generic-HDLC+PPP in broken state for 2.6.25, aren't we? If we do, perhaps a "|| BROKEN" in drivers/net/wan/Kconfig would make sense? Any attempt to use it will cause kernel panic immediately (PPP with generic HDLC only; drivers using syncppp.c directly are not affected). I can make that trivial Kconfig patch of course. -- Krzysztof Halasa --
From: Krzysztof Halasa <khc@pm.waw.pl> Kyzysztof, this is the way you behave every time your patches don't get looked at as quickly as you would like them to. And this behavior does not trigger us maintainers to stop everything we're doing and apply your patches. In fact it makes us want to review your patches even less. Sending a healthy ping asking if your patches need to be resent, etc., is one thing. But this isn't what you're doing here. --
I don't know what are you talking about. - someone broke my drivers, happens from time to time - I asked about related change and was told about some "out of tree" drivers instead - I had two modules to fix, one took 15 minutes and for the other one I had no time at the moment. - I finally rewrote the other module - got feedback, corrected it, resent. - asked if we are including it in 2.6.25 or if we are to make temporary Kconfig change instead What exactly don't you like in my "behaviour"? I didn't invent synchronous serial devices. Actually, I'm doing it all in my free time, and I'm sure I can find better things to do in that time. I accept I may use not the best wording from time to time, I'm not native English writter. What is your problem about me? I'd appreciate it if you point out the precise things I'm doing wrong. Thanks. -- Krzysztof Halasa --
In your original email you said I guess it should go into 2.6.25, not sure about "stable" series. I will appreciate any feedback, review and/or test results. At the time of the posting 2.6.25-rc6 had already been released, which seems like an inappropriate time for all that new code, which has been given so little exposure to real world testing. Certainly your original message said PPP panics, but without even minimal testing how do we know that your new code doesn't have equally problematic issues? So quite honestly a CONFIG_BROKEN patch might indeed be more appropriate since generic HDLC works with Frame Relay at least... Comments? IMO the current code is a known risk (PPP panic, FR works) whereas the new code is an unknown risk very late in 2.6.25-rc -- but with a good chance to make HDLC+PPP work again. Jeff --
Actually my question was intended for Andrew as he has already queued the PPP patch - I probably should have rearranged To/Cc: headers, sorry about that. The question still stands: I guess it's fair enough to have either patch in 2.6.25 (new PPP or "BROKEN" in Kconfig) but I think we shouldn't leave it as is. -- Krzysztof Halasa --
Well, there was something like "minimal testing", and it doesn't panic 100% like the old code does. But the probability that it won't work correctly is quite high. IOW: the new version is certainly better than the old one, though it's Actually Frame Relay and other protocols are not affected, the PPP patch doesn't change them a bit, it's a different module. The new code only affect PPP protocol. I'm fine with the Kconfig patch, actually I'm not sure what is better at this time - a known broken module marked as such or a new module with some small chances that it will crash the machine and with much bigger chances that it won't work with a certain PPP implementation on the other end. Something like that? Untested :-) Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl> --- a/drivers/net/wan/Kconfig +++ b/drivers/net/wan/Kconfig @@ -150,9 +150,13 @@ config HDLC_FR config HDLC_PPP tristate "Synchronous Point-to-Point Protocol (PPP) support" - depends on HDLC + depends on HDLC && BROKEN 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. If unsure, say N. --
Hi, Linus just made 2.6.25-rclast (I hope), I understand 2.6.25 will have broken drivers/net/wan/hdlc_ppp, any chance to apply the temporary Kconfig ("depends on BROKEN") patch? Would you like me to resend it? TIA. -- Krzysztof Halasa --
It never hurts to resend. It was 6000 patches ago and my mind is a blank. --
Sure, here is it. Some description, well: PPP support in generic HDLC in Linux 2.6.25 is broken and will cause a kernel panic when a device configured in PPP mode is activated. It will be replaced by new PPP implementation after Linux 2.6.25 is released. This affects only PPP support in generic HDLC (mostly Hitachi SCA and SCA-II based drivers, wanxl, and few others). Standalone syncppp and async PPP support are not affected. Untested :-) Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl> --- a/drivers/net/wan/Kconfig +++ b/drivers/net/wan/Kconfig @@ -150,9 +150,13 @@ config HDLC_FR config HDLC_PPP tristate "Synchronous Point-to-Point Protocol (PPP) support" - depends on HDLC + depends on HDLC && BROKEN 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. If unsure, say N. --
On Sat, 12 Apr 2008 10:10:40 +0200 Thats a pretty bad regression. Surely the correct fix is to revert the change series that caused the breakage then re-merge that and fixes for HDLC PPP for 2.6.26, not just leave it broken ? --
It's not that simple - it was broken between 2.6.22 and 2.6.23 (2.6.23
was already broken), I just haven't noticed (I don't use PPP myself,
except for tests).
author Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Fri, 6 Jul 2007 20:36:20 +0000 (13:36 -0700)
commit f25f4e44808f0f6c9875d94ef1c41ef86c288eb2
[CORE] Stack changes to add multiqueue hardware support API
Add the multiqueue hardware device support API to the core network
stack. Allow drivers to allocate multiple queues and manage them at
the netdev level if they choose to do so.
Added a new field to sk_buff, namely queue_mapping, for drivers to
know which tx_ring to select based on OS classification of the flow.
And it included:
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -565,9 +578,7 @@ struct net_device
static inline void *netdev_priv(const struct net_device *dev)
{
- return (char *)dev + ((sizeof(struct net_device)
- + NETDEV_ALIGN_CONST)
- & ~NETDEV_ALIGN_CONST);
+ return dev->priv;
}
#define SET_MODULE_OWNER(dev) do { } while (0)
HDLC PPP sits between hardware drivers and (in this case) syncppp.c,
and it (hdlc_ppp module) was using netdev_priv() as a pointer to the
memory allocated with (after) the netdev structure. dev->priv was used
by syncppp.c.
HDLC FR was also broken by this change but the fix was easy because
there is no external protocol module to interface to.
I didn't want to add to the HDLC PPP mess anymore and have rewritten
it instead (in process, it turned out syncppp alone, and syncppp +
HDLC have much more problems than I thought). Obviously I was way too
late for 2.6.25 (past 2.6.25-rc5 and the new code wasn't much tested).
Yes, I could add another dirty "interface fix" to the HDLC + syncppp
combo (hdlc_ppp), but I don't really see a sense, especially that I
have the new implementation ready and working (my time resources are
currently quite limited ...What I find a bit disturbing is I found my original patchsets I submitted to the list that were committed for this patchset. My original patches don't have this change in them at all. :-( -PJ Waskiewicz --
From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com> Ummmm, if you remember we had to put that in because it was the only way to get your multiqueue changes to even work. So don't even act as if that got slipped in behind your back, unknowingly. You were absolutely informed about this. --
David doesn't want the new PPP, insisting that I fix existing hdlc_ppp.c + syncppp.c combo instead, and that there are related bugs in drivers using syncppp.c alone (not hdlc_ppp) which I have to fix as well. Unfortunately I'm unable to do that. Even if I could add another workaround to hdlc_ppp to make it work with syncppp.c again, I can't fix the other drivers (if they are indeed broken) because of lack of hardware, and I can't really make significant changes to syncppp.c (to make them available to all drivers) because that could and would break those drivers. Comments? -- Krzysztof Halasa --
From: Krzysztof Halasa <khc@pm.waw.pl> I'll fix it properly because I know you won't do the work. You don't need to keep making excuses. --
Great to read that. Just few friendly after all notes so that you don't waste your time learning that yourself: - my patch didn't give us a third PPP implementation, we already have three: generic PPP, syncppp and PPP for ISDN :-) - of those, both generic PPP and (I think) ISDN PPP are in a good shape - generic PPP is specialized for dial-up async devices and has the required features (in kernel and in userspace pppd) - auth, multi-line, compression etc. It's a "lets give /dev/ttyS* a network device" implementation. - ISDN PPP does for ISDN cards basically the same as generic PPP does for terminals. They are completely different from syncppp as the needs are completely different. Fixed-line PPP must be small and fast, and self-contained. Now syncppp.c and sync serial (HDLC) cards. We basically have three categories of hardware: - mostly old ISA cards (cosa, cyclom-2x, z85230, s502/s508, personally I have c101 and n2 for tests, and PCI wanxl400). I think Alan maintains Zilog drivers, other "old" drivers are not maintained at all. They use syncppp.c directly (except c101, n2 and wanxl using generic HDLC). The list was longer but few drivers have been removed recently. - cards with in-kernel obsolete drivers and users encouraged to use manufacturers' driver - lmc. - a bit more modern PCI cards such as pc300, pci200syn, synclinks, dscc4, farsync, some off-tree ones. They use generic HDLC. - I have offered to port any remaining old driver to generic HDLC if I'm sent a hardware sample. Synclink, dscc4 and farsync have been ported by other people. I would be surprised to find that the "old" drivers are still functional. Nevertheless syncppp.c is functional (though marginally) and I think there are no bugs related to dev->priv and netdev_priv() for those drivers. The real problem with old drivers is the lack of hardware. Personally I suspect they may have 0 users worldwide. Syncppp.c is the problem for newer cards: - the ...
Generic ppp isn't specialized at all, and it isn't limited to async Can you elaborate on why the code that uses syncppp can't use the generic ppp code together with a userspace ppp control protocol implementation like pppd? -- James Chapman Katalix Systems Ltd http://www.katalix.com Catalysts for your Embedded Linux software development --
Perhaps I should write "dial-up" and other non-fixed connections. It's complex, I think kernel interface to generic HDLC would mean more code than PPP implementation required for fixed lines. Additional requirement - userspace daemon with additional plugin - may not be the best thing for fixed lines either. That would break backward compatibility, too. Personally I think it could be acceptable but it's not for me to decide. -- Krzysztof Halasa --
I maintain both pppd and generic HDLC PPP interfaces for the synclink drivers. I would like to have a single PPP implementation, but what Krzysztof writes about compatibility and complexity (both in coding and user configuration) is a real issue. Many customers who choose to use generic HDLC PPP are *dead* set against the added complexity and (user space) components of using pppd even though it has more features. I say that having tried to persuade such users to use pppd. The response is usually "support the simpler generic HDLC PPP way of doing things or we will go elsewhere". Others require the extra features of pppd. I understand customer desires are not always rational or a primary concern when making these architectural decisions, but I know forcing the extra complexity and components of pppd on generic HDLC users will cause a lot of anger and defections. -- Paul Fulghum Microgate Systems, Ltd. --
The fact that Krzysztof's solution was _small_ and _clean_ and easily maintainable was the reason I merged it [into my tree]. IMO sometimes "one size fits all" is not the best solution. Jeff --
From: Jeff Garzik <jeff@garzik.org> This is besides the point. We are discussing two things here: 1) How to "correctly" fix the syncppp private area bug. 2) How to, long term, support PPP properly in the kernel for various users. The fact that non-HDLC users of syncppp got left broken is why I objected to the change you merged in Jeff. It simply duplicated the majority of syncppp into the HDLC PPP code, which is just rediculious. That had nothing to do with whether we should, in the long term, use the generic PPP infrastructure we have now. I would have been more than happy if syncppp was retained and fixed properly, instead of being abandoned and duplicated in one fell swoop. We don't do things like that Jeff. --
The fact is it isn't broken WRT dev->priv at least (I don't know about random unrelated breakage). The drivers: a) alloc a netdev b) set dev->priv to some private kmalloced area It was like that for years, and it worked. The commit in question changed netdev_priv() and the drivers don't use this function. netdev_priv() was introduced at some point in time to allow alloc_netdev() to optionally allocate additional memory for internal use by the driver. It had nothing to do with dev->priv except "priv" name. The drivers don't use it passing size 0. This is probably suboptimal (two+ allocations instead of one), if the drivers had maintainers with access to hardware I guess it would be optimized, but it has nothing to do with the regression. The regression is present in HDLC PPP only, becase HDLC PPP actually used netdev_priv() (unlike non-HDLC PPP cases) as a means of addressing the area following net_device struct and not for retrieving dev->priv, I would be happy as well. The problem is that nobody shown any idea how to do it. I have offered: I will port any existing sync serial driver to generic HDLC if I'm sent a free hardware sample. That includes old ISA cards (I still have an old 2*PII-333 ISA test machine), and porting PC300 T1/E1 code to pc300too. If some driver/hw has no users I think there is no point in keeping it on life support here. The same goes for syncppp - if/when the number of drivers using it drops to zero, we can let it go. -- Krzysztof Halasa --
From: Krzysztof Halasa <khc@pm.waw.pl> 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. It was never meant to provide a facility to have two private areas associated with a device, but unfortunately some broken stuff decided to use it that way. --
That's essentially what I meant. And it has changed, call it whatever you wish. -- Krzysztof Halasa --
From: Krzysztof Halasa <khc@pm.waw.pl> 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 --
From: Krzysztof Halasa <khc@pm.waw.pl> 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 --
From: Krzysztof Halasa <khc@pm.waw.pl> 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. --
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 --
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 --
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 --
From: Alan Cox <alan@lxorguk.ukuu.org.uk>
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 <davem@davemloft.net>
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 <davem@davemloft.net>
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 = ...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 --
WAN: protect protocol list in hdlc.c with RTNL.
Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>
--- 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();
}
--
From: Krzysztof Halasa <khc@pm.waw.pl> Acked-by: David S. Miller <davem@davemloft.net> --
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:
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
--
I guess what caught my eye is a PPP control protocol implementation being in the kernel. I'd seen syncppp before but I assumed it was there for legacy reasons. A while ago there seemed to be strong desire to move control protocols such as bridge spanning tree into userspace. Is this no longer the case? -- James Chapman Katalix Systems Ltd http://www.katalix.com Catalysts for your Embedded Linux software development --
From: James Chapman <jchapman@katalix.com> It is still the case, believe me :-) --
From: Paul Fulghum <paulkf@microgate.com> Users say this to strong-hand developers, it's not something you should ever take very seriously. And even if Linux may simply not be for them, well that's fine too, and implementing something as obscure as HDLC PPP one way or the other is not going to change that. I mean, be realistic here. Are we going to have three copies of code implementing the same thing because some HDLC PPP users threatened to defect? That's simply rediculious. --
David Miller wrote: > Users say this to strong-hand developers, it's not something you > should ever take very seriously. And even if Linux may simply not be > for them, well that's fine too, and implementing something as obscure > as HDLC PPP one way or the other is not going to change that. Certainly not a big deal for Linux, but more significant for vendors of HDLC hardware :-) David Miller wrote: > I would have been more than happy if syncppp was retained and fixed > properly, instead of being abandoned and duplicated in one fell swoop. I'd be happy with that also. I was responding to the suggestion of merging generic HDLC PPP with the pppd implementation. It's been suggested before, but doing so looks messy. James Chapman wrote: > Paul Fulghum wrote: >> Many customers who choose to use generic HDLC PPP are *dead* >> set against the added complexity and (user space) >> components of using pppd even though it has more features. > > Are there technical reasons or is the complexity just a lack of > familiarity? From what I can tell it was an existing investment in scripts, training, tools, naming conventions, etc. Even when provided with new tools and scripts that do the same thing (as far as I could tell) the response was suprisingly vehement against change. -- Paul --
[two of them] Great, thanks. -- Krzysztof Halasa --
