Re: [PATCH net-next v5] net: x25: Queue received packets in the drivers instead of per-CPU queues
On 2021-04-05 21:34, Xie He wrote: Hi Martin, Could you ack? Thanks! Acked-by: Martin Schiller Just for the record: I'm certainly not always the fastest, but I don't work holidays or weekends. So you also need to have some patience.
Re: [PATCH net-next v4] net: x25: Queue received packets in the drivers instead of per-CPU queues
On 2021-03-31 20:41, Xie He wrote: Hi Martin, Could you ack this patch again? The only change from the RFC version (that you previously acked) is the addition of the "__GFP_NOMEMALLOC" flag in "dev_alloc_skb". This is because I want to prevent pfmemalloc skbs (which can't be handled by netif_receive_skb_core) from occurring. Thanks! Hi! Sorry for my late answer. Can you please also fix this line length warning? https://patchwork.hopto.org/static/nipa/442445/12117801/checkpatch/stdout - Martin
Re: [PATCH net] net: lapbether: Prevent racing when checking whether the netif is running
On 2021-03-14 02:59, Xie He wrote: Hi Martin, Could you ack? Thanks! Acked-by: Martin Schiller
Re: [PATCH net] net: lapbether: Remove netif_start_queue / netif_stop_queue
On 2021-03-07 12:33, Xie He wrote:
For the devices in this driver, the default qdisc is "noqueue",
because their "tx_queue_len" is 0.
In function "__dev_queue_xmit" in "net/core/dev.c", devices with the
"noqueue" qdisc are specially handled. Packets are transmitted without
being queued after a "dev->flags & IFF_UP" check. However, it's
possible
that even if this check succeeds, "ops->ndo_stop" may still have
already
been called. This is because in "__dev_close_many", "ops->ndo_stop" is
called before clearing the "IFF_UP" flag.
If we call "netif_stop_queue" in "ops->ndo_stop", then it's possible in
"__dev_queue_xmit", it sees the "IFF_UP" flag is present, and then it
checks "netif_xmit_stopped" and finds that the queue is already
stopped.
In this case, it will complain that:
"Virtual device ... asks to queue packet!"
To prevent "__dev_queue_xmit" from generating this complaint, we should
not call "netif_stop_queue" in "ops->ndo_stop".
We also don't need to call "netif_start_queue" in "ops->ndo_open",
because after a netdev is allocated and registered, the
"__QUEUE_STATE_DRV_XOFF" flag is initially not set, so there is no need
to call "netif_start_queue" to clear it.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Xie He
---
drivers/net/wan/lapbether.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
index 605fe555e157..c3372498f4f1 100644
--- a/drivers/net/wan/lapbether.c
+++ b/drivers/net/wan/lapbether.c
@@ -292,7 +292,6 @@ static int lapbeth_open(struct net_device *dev)
return -ENODEV;
}
- netif_start_queue(dev);
return 0;
}
@@ -300,8 +299,6 @@ static int lapbeth_close(struct net_device *dev)
{
int err;
- netif_stop_queue(dev);
-
if ((err = lapb_unregister(dev)) != LAPB_OK)
pr_err("lapb_unregister error: %d\n", err);
Seems reasonable to me.
Acked-by: Martin Schiller
Re: [PATCH net-next RFC] net: x25: Queue received packets in the drivers instead of per-CPU queues
On 2021-03-05 06:43, Xie He wrote: X.25 Layer 3 (the Packet Layer) expects layer 2 to provide a reliable datalink service such that no packets are reordered or dropped. And X.25 Layer 2 (the LAPB layer) is indeed designed to provide such service. However, this reliability is not preserved when a driver calls "netif_rx" to deliver the received packets to layer 3, because "netif_rx" will put the packets into per-CPU queues before they are delivered to layer 3. If there are multiple CPUs, the order of the packets may not be preserved. The per-CPU queues may also drop packets if there are too many. Therefore, we should not call "netif_rx" to let it queue the packets. Instead, we should use our own queue that won't reorder or drop packets. This patch changes all X.25 drivers to use their own queues instead of calling "netif_rx". The patch also documents this requirement in the "x25-iface" documentation. I've tested the hdlc_x25 driver. Looks good to me. Acked-by: Martin Schiller
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On 2021-03-03 00:30, Jakub Kicinski wrote: On Tue, 02 Mar 2021 08:04:20 +0100 Martin Schiller wrote: On 2021-03-01 09:56, Xie He wrote: > On Sun, Feb 28, 2021 at 10:56 PM Martin Schiller wrote: >> I mean the change from only one hdlc interface to both hdlc and >> hdlc_x25. >> >> I can't estimate how many users are out there and how their setup >> looks >> like. > > I'm also thinking about solving this issue by adding new APIs to the > HDLC subsystem (hdlc_stop_queue / hdlc_wake_queue) for hardware > drivers to call instead of netif_stop_queue / netif_wake_queue. This > way we can preserve backward compatibility. > > However I'm reluctant to change the code of all the hardware drivers > because I'm afraid of introducing bugs, etc. When I look at the code > of "wan/lmc/lmc_main.c", I feel I'm not able to make sure there are no > bugs (related to stop_queue / wake_queue) after my change (and even > before my change, actually). There are even serious style problems: > the majority of its lines are indented by spaces. > > So I don't want to mess with all the hardware drivers. Hardware driver > developers (if they wish to properly support hdlc_x25) should do the > change themselves. This is not a problem for me, because I use my own > out-of-tree hardware driver. However if I add APIs with no user code > in the kernel, other developers may think these APIs are not > necessary. I don't think a change that affects the entire HDLC subsystem is justified, since the actual problem only affects the hdlc_x25 area. The approach with the additional hdlc_x25 is clean and purposeful and I personally could live with it. I just don't see myself in the position to decide such a change at the moment. @Jakub: What is your opinion on this. Hard question to answer, existing users seem happy and Xie's driver isn't upstream, so the justification for potentially breaking backward compatibility isn't exactly "strong". Can we cop out and add a knob somewhere to control spawning the extra netdev? Let people who just want a newer kernel carry on without distractions and those who want the extra layer can flip the switch? Yes, that would be a good compromise. I think a compile time selection option is enough here. We could introduce a new config option CONFIG_HDLC_X25_LEGACY (or something like that) and then implement the new or the old behavior in the driver accordingly. A switch that can be toggled at runtime (e.g. via sethdlc) would also be conceivable, but I don't think this is necessary.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On 2021-03-01 09:56, Xie He wrote: On Sun, Feb 28, 2021 at 10:56 PM Martin Schiller wrote: >> Also, I have a hard time assessing if such a wrap is really >> enforceable. > > Sorry. I don't understand what you mean. What "wrap" are you referring > to? I mean the change from only one hdlc interface to both hdlc and hdlc_x25. I can't estimate how many users are out there and how their setup looks like. I'm also thinking about solving this issue by adding new APIs to the HDLC subsystem (hdlc_stop_queue / hdlc_wake_queue) for hardware drivers to call instead of netif_stop_queue / netif_wake_queue. This way we can preserve backward compatibility. However I'm reluctant to change the code of all the hardware drivers because I'm afraid of introducing bugs, etc. When I look at the code of "wan/lmc/lmc_main.c", I feel I'm not able to make sure there are no bugs (related to stop_queue / wake_queue) after my change (and even before my change, actually). There are even serious style problems: the majority of its lines are indented by spaces. So I don't want to mess with all the hardware drivers. Hardware driver developers (if they wish to properly support hdlc_x25) should do the change themselves. This is not a problem for me, because I use my own out-of-tree hardware driver. However if I add APIs with no user code in the kernel, other developers may think these APIs are not necessary. I don't think a change that affects the entire HDLC subsystem is justified, since the actual problem only affects the hdlc_x25 area. The approach with the additional hdlc_x25 is clean and purposeful and I personally could live with it. I just don't see myself in the position to decide such a change at the moment. @Jakub: What is your opinion on this.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On 2021-02-27 00:03, Xie He wrote: On Fri, Feb 26, 2021 at 6:21 AM Martin Schiller wrote: I have now had a look at it. It works as expected. I just wonder if it would not be more appropriate to call the lapb_register() already in x25_hdlc_open(), so that the layer2 (lapb) can already "work" before the hdlc_x25 interface is up. I think it's better not to keep LAPB running unless hdlc_x25 is up. If I am the user, I would expect that when I change the X.25 interface to the DOWN state, the LAPB protocol would be completely stopped and the LAPB layer would not generate any new frames anymore (even if the other side wants to connect), and when I change the X.25 interface back to the UP state, it would be a fresh new start for the LAPB protocol. Also, I have a hard time assessing if such a wrap is really enforceable. Sorry. I don't understand what you mean. What "wrap" are you referring to? I mean the change from only one hdlc interface to both hdlc and hdlc_x25. I can't estimate how many users are out there and how their setup looks like. Unfortunately I have no idea how many users there actually are.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On 2021-02-22 09:56, Xie He wrote: On Sun, Feb 21, 2021 at 11:14 PM Martin Schiller wrote: I'm not really happy with this change because it breaks compatibility. We then suddenly have 2 interfaces; the X.25 routings are to be set via the "new" hdlc_x25 interfaces instead of the hdlc interfaces. I currently just don't have a nicer solution to fix this queueing problem either. On the other hand, since the many years we have been using the current state, I have never noticed any problems with discarded frames. So it might be more a theoretical problem than a practical one. This problem becomes very serious when we use AF_PACKET sockets, because the majority of frames would be dropped by the hardware driver, which significantly impacts transmission speed. What I am really doing is to enable adequate support for AF_PACKET sockets, allowing users to use the bare (raw) LAPB protocol. If we take this into consideration, this problem is no longer just a theoretical problem, but a real practical issue. I have now had a look at it. It works as expected. I just wonder if it would not be more appropriate to call the lapb_register() already in x25_hdlc_open(), so that the layer2 (lapb) can already "work" before the hdlc_x25 interface is up. Also, I have a hard time assessing if such a wrap is really enforceable. Unfortunately I have no idea how many users there actually are. If we don't want to break backward compatibility, there is another option: We can create a new API for the HDLC subsystem for stopping/restarting the TX queue, and replace all HDLC hardware drivers' netif_stop_queue and netif_wake_queue calls with calls to this new API. This new API would then call hdlc_x25 to stop/restart its internal queue. But this option would require modifying all HDLC hardware drivers' code, and frankly, not all HDLC hardware drivers' developers care about running X.25 protocols on their hardware. So this would cause both hardware driver instabilities and confusion for hardware driver developers.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On 2021-02-19 21:28, Xie He wrote: On Fri, Feb 19, 2021 at 10:39 AM Jakub Kicinski wrote: Not entirely sure what the argument is about but adding constants would certainly help. Leon wants me to replace this: dev->needed_headroom = 3 - 1; with this: /* 2 is the result of 3 - 1 */ dev->needed_headroom = 2; But I don't feel his way is better than my way. More fundamentally IDK if we can make such a fundamental change here. When users upgrade from older kernel are all their scripts going to work the same? Won't they have to bring the new netdev up? Yes, this patch will break backward compatibility. Users with old scripts will find them no longer working. However, it's hard for me to find a better way to solve the problem described in the commit message. So I sent this as an RFC to see what people think about this. (Martin Schiller seems to be OK with this.) Well, I said I would like to test it. I'm not really happy with this change because it breaks compatibility. We then suddenly have 2 interfaces; the X.25 routings are to be set via the "new" hdlc_x25 interfaces instead of the hdlc interfaces. I currently just don't have a nicer solution to fix this queueing problem either. On the other hand, since the many years we have been using the current state, I have never noticed any problems with discarded frames. So it might be more a theoretical problem than a practical one. I think users who don't use scripts can adapt quickly and users who use scripts can also trivally fix their scripts. Actually many existing commits in the kernel also (more or less) cause some user-visible changes. But I admit this patch is a really big change.
Re: [PATCH net-next RFC v2] net: hdlc_x25: Queue outgoing LAPB frames
On 2021-02-10 18:35, Xie He wrote:
When sending packets, we will first hand over the (L3) packets to the
LAPB module, then the LAPB module will hand over the corresponding LAPB
(L2) frames back to us for us to transmit.
The LAPB module can also emit LAPB (L2) frames at any time without an
(L3) packet currently being sent, when it is trying to send (L3)
packets
queued up in its internal queue, or when it decides to send some (L2)
control frame.
This means we need have a queue for these outgoing LAPB (L2) frames to
avoid congestion. This queue needs to be controlled by the hardware
drivers' netif_stop_queue and netif_wake_queue calls. So we need to use
a qdisc TX queue for this purpose.
On the other hand, the outgoing (L3) packets don't need to be queued,
because the LAPB module already has an internal queue for them.
However, currently the outgoing LAPB (L2) frames are not queued. This
can cause frames to be dropped by hardware drivers when they are busy.
At the same time the (L3) packets are being queued and controlled by
hardware drivers' netif_stop_queue and netif_wake_queue calls. This is
unnecessary and meaningless.
To solve this problem, we can split the HDLC device into two devices:
a virtual X.25 device and an actual HDLC device, using the virtual
X.25 device to send (L3) packets and using the actual HDLC device to
queue LAPB (L2) frames. The outgoing LAPB queue will be controlled by
hardware drivers' netif_stop_queue and netif_wake_queue calls, while
outgoing (L3) packets will not be affected by these calls.
At first glance, the patch looks quite reasonable. The only thing I
noticed right away is that you also included the changes of your patch
"Return meaningful error code in x25_open".
I hope to get back to the office this week and test it.
Cc: Martin Schiller
Signed-off-by: Xie He
---
Change from RFC v1:
Properly initialize state(hdlc)->x25_dev and state(hdlc)->x25_dev_lock.
---
drivers/net/wan/hdlc_x25.c | 158 ++---
1 file changed, 129 insertions(+), 29 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index bb164805804e..2a7b3c3d0c05 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -23,6 +23,13 @@
struct x25_state {
x25_hdlc_proto settings;
+ struct net_device *x25_dev;
+ spinlock_t x25_dev_lock; /* Protects the x25_dev pointer */
+};
+
+/* Pointed to by netdev_priv(x25_dev) */
+struct x25_device {
+ struct net_device *hdlc_dev;
};
static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
@@ -32,6 +39,11 @@ static struct x25_state *state(hdlc_device *hdlc)
return hdlc->state;
}
+static struct x25_device *dev_to_x25(struct net_device *dev)
+{
+ return netdev_priv(dev);
+}
+
/* These functions are callbacks called by LAPB layer */
static void x25_connect_disconnect(struct net_device *dev, int
reason, int code)
@@ -89,15 +101,10 @@ static int x25_data_indication(struct net_device
*dev, struct sk_buff *skb)
static void x25_data_transmit(struct net_device *dev, struct sk_buff
*skb)
{
- hdlc_device *hdlc = dev_to_hdlc(dev);
-
+ skb->dev = dev_to_x25(dev)->hdlc_dev;
+ skb->protocol = htons(ETH_P_HDLC);
skb_reset_network_header(skb);
- skb->protocol = hdlc_type_trans(skb, dev);
-
- if (dev_nit_active(dev))
- dev_queue_xmit_nit(skb, dev);
-
- hdlc->xmit(skb, dev); /* Ignore return value :-( */
+ dev_queue_xmit(skb);
}
@@ -163,17 +170,18 @@ static int x25_open(struct net_device *dev)
.data_indication = x25_data_indication,
.data_transmit = x25_data_transmit,
};
- hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct net_device *hdlc_dev = dev_to_x25(dev)->hdlc_dev;
+ hdlc_device *hdlc = dev_to_hdlc(hdlc_dev);
struct lapb_parms_struct params;
int result;
result = lapb_register(dev, );
if (result != LAPB_OK)
- return result;
+ return -ENOMEM;
result = lapb_getparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
if (state(hdlc)->settings.dce)
params.mode = params.mode | LAPB_DCE;
@@ -188,16 +196,104 @@ static int x25_open(struct net_device *dev)
result = lapb_setparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
return 0;
}
-static void x25_close(struct net_device *dev)
+static int x25_close(struct net_device *dev)
{
lapb_unregister(dev);
+ return 0;
+}
+
+static const struct net_device_ops hdlc_x25_netdev_ops = {
+ .ndo_open = x25_open,
+ .ndo_stop = x25_close,
+ .ndo_start_xmit = x25_xmit,
+};
+
+static void x25_setup_virtual_dev(struct net_device *dev)
+{
+ dev->netdev_ops
Re: [PATCH net] net: hdlc_x25: Return meaningful error code in x25_open
On 2021-02-03 08:15, Xie He wrote:
It's not meaningful to pass on LAPB error codes to HDLC code or other
parts of the system, because they will not understand the error codes.
Instead, use system-wide recognizable error codes.
Fixes: f362e5fe0f1f ("wan/hdlc_x25: make lapb params configurable")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_x25.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index bb164805804e..4aaa6388b9ee 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -169,11 +169,11 @@ static int x25_open(struct net_device *dev)
result = lapb_register(dev, );
if (result != LAPB_OK)
- return result;
+ return -ENOMEM;
result = lapb_getparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
if (state(hdlc)->settings.dce)
params.mode = params.mode | LAPB_DCE;
@@ -188,7 +188,7 @@ static int x25_open(struct net_device *dev)
result = lapb_setparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
return 0;
}
Thanks for fixing this.
Acked-by: Martin Schiller
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On 2021-02-01 12:38, Xie He wrote: On Mon, Feb 1, 2021 at 1:18 AM Martin Schiller wrote: I have thought about this issue again. I also have to say that I have never noticed any problems in this area before. So again for (my) understanding: When a hardware driver calls netif_stop_queue, the frames sent from layer 3 (X.25) with dev_queue_xmit are queued and not passed "directly" to x25_xmit of the hdlc_x25 driver. So nothing is added to the write_queue anymore (except possibly un-acked-frames by lapb_requeue_frames). If the LAPB module only emits an L2 frame when an L3 packet comes from the upper layer, then yes, there would be no problem because the L3 packet is already controlled by the qdisc and there is no need to control the corresponding L2 frame again. However, the LAPB module can emits L2 frames when there's no L3 packet coming, when 1) there are some packets queued in the LAPB module's internal queue; and 2) the LAPB decides to send some control frame (e.g. by the timers). But control frames are currently sent past the lapb write_queue. So another queue would have to be created. And wouldn't it be better to have it in the hdlc_x25 driver, leaving LAPB unaffected? Shouldn't it actually be sufficient to check for netif_queue_stopped in lapb_kick and then do "nothing" if necessary? We can consider this situation: When the upper layer has nothing to send, but there are some packets in the LAPB module's internal queue waiting to be sent. The LAPB module will try to send the packets, but after it has sent out the first packet, it will meet the "queue stopped" situation. In this situation, it'd be preferable to immediately start sending the second packet after the queue is started again. "Doing nothing" in this situation would mean waiting until some other events occur, such as receiving responses from the other side, or receiving more outgoing packets from L3. As soon as the hardware driver calls netif_wake_queue, the whole thing should just continue running. This relies on the fact that the upper layer has something to send. If the upper layer has nothing to send, lapb_kick would not be automatically called again until some other events occur (such as receiving responses from the other side). I think it'd be better if we do not rely on the assumption that L3 is going to send more packets to us, as L3 itself would assume us to provide it a reliable link service and we should fulfill its expectation.
Re: [PATCH net] net: lapb: Copy the skb before sending a packet
On 2021-02-01 11:49, Xie He wrote: On Mon, Feb 1, 2021 at 2:05 AM Martin Schiller wrote: What kind of packages do you mean are corrupted? ETH_P_X25 or ETH_P_HDLC? I mean ETH_P_X25. I was using "lapbether.c" to test so there was no ETH_P_HDLC. I have also sent a patch here in the past that addressed corrupted ETH_P_X25 frames on an AF_PACKET socket: https://lkml.org/lkml/2020/1/13/388 Unfortunately I could not track and describe exactly where the problem was. Ah... Looks like we had the same problem. I just wonder when/where is the logically correct place to copy the skb. Shouldn't it be copied before removing the pseudo header (as I did in my patch)? I think it's not necessary to copy it before removing the pseudo header, because "skb_pull" will not change any data in the data buffer. "skb_pull" will only change the values of "skb->data" and "skb->len". These values are not shared between clones of skbs, so it's safe to change them without affecting other clones of the skb. I also choose to copy the skb in the LAPB module (rather than in the drivers) because I see all drivers have this problem (including the recently deleted x25_asy.c driver), so it'd be better to fix this issue in the LAPB module, for all drivers. OK. Acked-by: Martin Schiller
Re: [PATCH net] net: lapb: Copy the skb before sending a packet
On 2021-02-01 06:57, Xie He wrote:
When sending a packet, we will prepend it with an LAPB header.
This modifies the shared parts of a cloned skb, so we should copy the
skb rather than just clone it, before we prepend the header.
In "Documentation/networking/driver.rst" (the 2nd point), it states
that drivers shouldn't modify the shared parts of a cloned skb when
transmitting.
The "dev_queue_xmit_nit" function in "net/core/dev.c", which is called
when an skb is being sent, clones the skb and sents the clone to
AF_PACKET sockets. Because the LAPB drivers first remove a 1-byte
pseudo-header before handing over the skb to us, if we don't copy the
skb before prepending the LAPB header, the first byte of the packets
received on AF_PACKET sockets can be corrupted.
What kind of packages do you mean are corrupted?
ETH_P_X25 or ETH_P_HDLC?
I have also sent a patch here in the past that addressed corrupted
ETH_P_X25 frames on an AF_PACKET socket:
https://lkml.org/lkml/2020/1/13/388
Unfortunately I could not track and describe exactly where the problem
was.
I just wonder when/where is the logically correct place to copy the skb.
Shouldn't it be copied before removing the pseudo header (as I did in my
patch)?
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
net/lapb/lapb_out.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/lapb/lapb_out.c b/net/lapb/lapb_out.c
index 7a4d0715d1c3..a966d29c772d 100644
--- a/net/lapb/lapb_out.c
+++ b/net/lapb/lapb_out.c
@@ -82,7 +82,8 @@ void lapb_kick(struct lapb_cb *lapb)
skb = skb_dequeue(>write_queue);
do {
- if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) {
+ skbn = skb_copy(skb, GFP_ATOMIC);
+ if (!skbn) {
skb_queue_head(>write_queue, skb);
break;
}
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On 2021-01-31 04:16, Xie He wrote: On Sat, Jan 30, 2021 at 11:16 AM Jakub Kicinski wrote: Sounds like too much afford for a sub-optimal workaround. The qdisc semantics are borken in the proposed scheme (double counting packets) - both in term of statistics and if user decides to add a policer, filter etc. Hmm... Another solution might be creating another virtual device on top of the HDLC device (similar to what "hdlc_fr.c" does), so that we can first queue L3 packets in the virtual device's qdisc queue, and then queue the L2 frames in the actual HDLC device's qdisc queue. This way we can avoid the same outgoing data being queued to qdisc twice. But this would significantly change the way the user uses the hdlc_x25 driver. Another worry is that something may just inject a packet with skb->protocol == ETH_P_HDLC but unexpected structure (IDK if that's a real concern). This might not be a problem. Ethernet devices also allow the user to inject raw frames with user constructed headers. "hdlc_fr.c" also allows the user to bypass the virtual circuit interfaces and inject raw frames directly on the HDLC interface. I think the receiving side should be able to recognize and drop invalid frames. It may be better to teach LAPB to stop / start the internal queue. The lower level drivers just needs to call LAPB instead of making the start/wake calls directly to the stack, and LAPB can call the stack. Would that not work? I think this is a good solution. But this requires changing a lot of code. The HDLC subsystem needs to be changed to allow HDLC Hardware Drivers to ask HDLC Protocol Drivers (like hdlc_x25.c) to stop/wake the TX queue. The hdlc_x25.c driver can then ask the LAPB module to stop/wake the queue. So this means new APIs need to be added to both the HDLC subsystem and the LAPB module, and a number of HDLC Hardware Drivers need to be changed to call the new API of the HDLC subsystem. Martin, do you have any suggestions? I have thought about this issue again. I also have to say that I have never noticed any problems in this area before. So again for (my) understanding: When a hardware driver calls netif_stop_queue, the frames sent from layer 3 (X.25) with dev_queue_xmit are queued and not passed "directly" to x25_xmit of the hdlc_x25 driver. So nothing is added to the write_queue anymore (except possibly un-acked-frames by lapb_requeue_frames). Shouldn't it actually be sufficient to check for netif_queue_stopped in lapb_kick and then do "nothing" if necessary? As soon as the hardware driver calls netif_wake_queue, the whole thing should just continue running. Or am I missing something?
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On 2021-01-28 23:06, Xie He wrote: On Thu, Jan 28, 2021 at 11:47 AM Jakub Kicinski wrote: Noob question - could you point at or provide a quick guide to layering here? I take there is only one netdev, and something maintains an internal queue which is not stopped when HW driver stops the qdisc? Yes, there is only one netdev. The LAPB module (net/lapb/) (which is used as a library by the netdev driver - hdlc_x25.c) is maintaining an internal queue which is not stopped when the HW driver stops the qdisc. The queue is "write_queue" in "struct lapb_cb" in "include/net/lapb.h". The code that takes skbs out of the queue and feeds them to lower layers for transmission is at the "lapb_kick" function in "net/lapb/lapb_out.c". The layering is like this: Upper layer (Layer 3) (net/x25/ or net/packet/) ^ | L3 packets (with control info) v The netdev driver (hdlc_x25.c) ^ | L3 packets v The LAPB Module (net/lapb/) ^ | LAPB (L2) frames v The netdev driver (hdlc_x25.c) ^ | LAPB (L2) frames | (also called HDLC frames in the context of the HDLC subsystem) v HDLC core (hdlc.c) ^ | HDLC frames v HDLC Hardware Driver @Xie: Thank you for the detailed presentation. Sounds like we're optimizing to prevent drops, and this was not reported from production, rather thru code inspection. Ergo I think net-next will be more appropriate here, unless Martin disagrees. Yes, I have no problem in targeting net-next instead. Thanks! I agree.
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On 2021-01-27 21:29, Xie He wrote: On Wed, Jan 27, 2021 at 2:14 AM David Laight wrote: If I read this correctly it adds a (potentially big) queue between the LAPB code that adds the sequence numbers to the frames and the hardware that actually sends them. Yes. The actual number of outgoing LAPB frames being queued depends on how long the hardware driver stays in the TX busy state, and is limited by the LAPB sending window. IIRC [1] there is a general expectation that the NR in a transmitted frame will be the same as the last received NS unless acks are being delayed for flow control reasons. You definitely want to be able to ack a received frame while transmitting back-to-back I-frames. This really means that you only want 2 frames in the hardware driver. The one being transmitted and the next one - so it gets sent with a shared flag. There is no point sending an RR unless the hardware link is actually idle. If I understand correctly, what you mean is that the frames sent on the wire should reflect the most up-to-date status of what is received from the wire, so queueing outgoing LAPB frames is not appropriate. But this would require us to deal with the "TX busy" issue in the LAPB module. This is (as I said) not easy to do. I currently can't think of a good way of doing this. Instead, we can think of the TX queue as part of the "wire". We can think of the wire as long and having a little higher latency. I believe the LAPB protocol has no problem in handling long wires. What do you think? David: Can you please elaborate on your concerns a little bit more? I think Xie's approach is not bad at all. LAPB (L2) has no idea about L1 (apart from the link state) and sends as many packets as possible, which of course we should not discard. The remaining window determines how many packets are put into this queue. Since we can't send anything over the line due to the TX Busy state, the remote station (due to lack of ACKs) will also stop sending anything at some point. When the link goes down, all buffers/queues must be cleared.
Re: [PATCH net v5] net: lapb: Add locking to the lapb module
On 2021-01-24 05:45, Jakub Kicinski wrote:
On Fri, 22 Jan 2021 10:07:05 +0100 Martin Schiller wrote:
On 2021-01-21 01:21, Xie He wrote:
> In the lapb module, the timers may run concurrently with other code in
> this module, and there is currently no locking to prevent the code from
> racing on "struct lapb_cb". This patch adds locking to prevent racing.
>
> 1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
> "spin_unlock_bh" to APIs, timer functions and notifier functions.
>
> 2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
> able to ask running timers to abort; Modify "lapb_stop_t1timer" and
> "lapb_stop_t2timer" to make them able to abort running timers;
> Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
> abort after they are stopped by "lapb_stop_t1timer",
> "lapb_stop_t2timer",
> and "lapb_start_t1timer", "lapb_start_t2timer".
>
> 3. Let lapb_unregister wait for other API functions and running timers
> to stop.
>
> 4. The lapb_device_event function calls lapb_disconnect_request. In
> order to avoid trying to hold the lock twice, add a new function named
> "__lapb_disconnect_request" which assumes the lock is held, and make
> it called by lapb_disconnect_request and lapb_device_event.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: Martin Schiller
> Signed-off-by: Xie He
I don't have the opportunity to test this at the moment, but code
looks
reasonable so far. Have you tested this at runtime?
Are you okay with this being merged or would you like to review
further/test?
Nothing jumps out to me either (other than a few nit picks).
Adding a small delay in the while loop is a good idea.
Otherwise: Yes, I agree with merging this.
> Change from v4:
> Make lapb_unregister wait for other refs to "lapb" to drop, to ensure
> that other LAPB API calls have all finished.
>
> Change from v3:
> In lapb_unregister make sure the self-restarting t1timer has really
> been
> stopped.
>
> Change from v2:
> Create a new __lapb_disconnect_request function to reduce redundant
> code.
>
> Change from v1:
> Broke long lines to keep the line lengths within 80 characters.
> @@ -178,11 +182,23 @@ int lapb_unregister(struct net_device *dev)
>goto out;
>lapb_put(lapb);
>
> + /* Wait for other refs to "lapb" to drop */
> + while (refcount_read(>refcnt) > 2)
> + ;
Tight loop like this is a little scary, perhaps add a small
usleep_range() here?
> +
> + spin_lock_bh(>lock);
> +
>lapb_stop_t1timer(lapb);
>lapb_stop_t2timer(lapb);
>
>lapb_clear_queues(lapb);
>
> + spin_unlock_bh(>lock);
> +
> + /* Wait for running timers to stop */
> + del_timer_sync(>t1timer);
> + del_timer_sync(>t2timer);
> +
>__lapb_remove_cb(lapb);
>
>lapb_put(lapb);
> -int lapb_disconnect_request(struct net_device *dev)
> +static int __lapb_disconnect_request(struct lapb_cb *lapb)
> {
> - struct lapb_cb *lapb = lapb_devtostruct(dev);
> - int rc = LAPB_BADTOKEN;
> -
> - if (!lapb)
> - goto out;
> -
>switch (lapb->state) {
>case LAPB_STATE_0:
> - rc = LAPB_NOTCONNECTED;
> - goto out_put;
> + return LAPB_NOTCONNECTED;
>
>case LAPB_STATE_1:
>lapb_dbg(1, "(%p) S1 TX DISC(1)\n", lapb->dev);
> @@ -310,12 +328,10 @@ int lapb_disconnect_request(struct net_device
> *dev)
>lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON, LAPB_COMMAND);
>lapb->state = LAPB_STATE_0;
>lapb_start_t1timer(lapb);
> - rc = LAPB_NOTCONNECTED;
> - goto out_put;
> + return LAPB_NOTCONNECTED;
>
>case LAPB_STATE_2:
> - rc = LAPB_OK;
> - goto out_put;
> + return LAPB_OK;
>}
>
>lapb_clear_queues(lapb);
> @@ -328,8 +344,22 @@ int lapb_disconnect_request(struct net_device
> *dev)
>lapb_dbg(1, "(%p) S3 DISC(1)\n", lapb->dev);
>lapb_dbg(0, "(%p) S3 -> S2\n", lapb->dev);
>
> - rc = LAPB_OK;
> -out_put:
> + return LAPB_OK;
> +}
Since this is a fix for net, I'd advise against converting the goto
into direct returns (as much as I generally like such conversion).
Re: [PATCH net v5] net: lapb: Add locking to the lapb module
On 2021-01-21 01:21, Xie He wrote:
In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer",
"lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. Let lapb_unregister wait for other API functions and running timers
to stop.
4. The lapb_device_event function calls lapb_disconnect_request. In
order to avoid trying to hold the lock twice, add a new function named
"__lapb_disconnect_request" which assumes the lock is held, and make
it called by lapb_disconnect_request and lapb_device_event.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
I don't have the opportunity to test this at the moment, but code looks
reasonable so far. Have you tested this at runtime?
---
Change from v4:
Make lapb_unregister wait for other refs to "lapb" to drop, to ensure
that other LAPB API calls have all finished.
Change from v3:
In lapb_unregister make sure the self-restarting t1timer has really
been
stopped.
Change from v2:
Create a new __lapb_disconnect_request function to reduce redundant
code.
Change from v1:
Broke long lines to keep the line lengths within 80 characters.
---
include/net/lapb.h| 2 ++
net/lapb/lapb_iface.c | 70 +--
net/lapb/lapb_timer.c | 30 ---
3 files changed, 82 insertions(+), 20 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ boolt1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..b028dfc438ce 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode= LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(>lock);
refcount_set(>refcnt, 1);
out:
return lapb;
@@ -178,11 +182,23 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
+ /* Wait for other refs to "lapb" to drop */
+ while (refcount_read(>refcnt) > 2)
+ ;
+
+ spin_lock_bh(>lock);
+
lapb_stop_t1timer(lapb);
lapb_stop_t2timer(lapb);
lapb_clear_queues(lapb);
+ spin_unlock_bh(>lock);
+
+ /* Wait for running timers to stop */
+ del_timer_sync(>t1timer);
+ del_timer_sync(>t2timer);
+
__lapb_remove_cb(lapb);
lapb_put(lapb);
@@ -201,6 +217,8 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +237,7 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(>lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +253,8 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_INVALUE;
if (parms->t
Re: [PATCH net v4] net: lapb: Add locking to the lapb module
On 2021-01-20 11:28, Xie He wrote:
In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer",
"lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. In lapb_unregister, add "del_timer_sync" calls to make sure all
running timers have exited.
4. The lapb_device_event function calls lapb_disconnect_request. In
order to avoid trying to hold the lock twice, add a new function named
"__lapb_disconnect_request" which assumes the lock is held, and make
it called by lapb_disconnect_request and lapb_device_event.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
Can you please add a Changelog. What was changed in v4?
---
include/net/lapb.h| 2 ++
net/lapb/lapb_iface.c | 65 ---
net/lapb/lapb_timer.c | 30 +---
3 files changed, 77 insertions(+), 20 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ boolt1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..205f8736e68b 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode= LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(>lock);
refcount_set(>refcnt, 1);
out:
return lapb;
@@ -178,11 +182,18 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
+ spin_lock_bh(>lock);
+
lapb_stop_t1timer(lapb);
lapb_stop_t2timer(lapb);
lapb_clear_queues(lapb);
+ spin_unlock_bh(>lock);
+
+ del_timer_sync(>t1timer);
+ del_timer_sync(>t2timer);
+
__lapb_remove_cb(lapb);
lapb_put(lapb);
@@ -201,6 +212,8 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +232,7 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(>lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +248,8 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_INVALUE;
if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
goto out_put;
@@ -256,6 +272,7 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb);
out:
return rc;
@@ -270,6 +287,8 @@ int lapb_connect_request(struct net_device *dev)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_OK;
if (lapb->state == LAPB_STATE_1)
goto out_put;
@@ -285,24 +304,18 @@ int lapb_connect_request(struct net_device *dev)
rc = LAPB_OK;
out_put:
Re: [PATCH net v2] net: lapb: Add locking to the lapb module
On 2021-01-17 23:47, Xie He wrote:
In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer",
"lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. In lapb_unregister, change "lapb_stop_t1timer" and
"lapb_stop_t2timer"
to "del_timer_sync" to make sure all running timers have exited.
4. In lapb_device_event, replace the "lapb_disconnect_request" call
with
the content of "lapb_disconnect_request", to avoid trying to hold the
lock twice. When I do this, I removed "lapb_start_t1timer" because I
don't think it's necessary to start the timer when "NETDEV_GOING_DOWN".
I don't like the code redundancy this creates. Maybe you should move the
actual functionality from lapb_disconnect_request() to a
__lapb_disconnect_request(), and in lapb_disconnect_request() call this
function including locking around it and also in lapb_device_event
(without locking).
Calling lapb_start_t1timer() on a "NETDEV_GOING_DOWN" event does not
hurt and is correct from a protocol flow point of view after sending
the DISC.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
include/net/lapb.h| 2 ++
net/lapb/lapb_iface.c | 56 +++
net/lapb/lapb_timer.c | 30 +++
3 files changed, 80 insertions(+), 8 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ boolt1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..45f332607685 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode= LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(>lock);
refcount_set(>refcnt, 1);
out:
return lapb;
@@ -178,8 +182,8 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
- lapb_stop_t1timer(lapb);
- lapb_stop_t2timer(lapb);
+ del_timer_sync(>t1timer);
+ del_timer_sync(>t2timer);
lapb_clear_queues(lapb);
@@ -201,6 +205,8 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +225,7 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(>lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +241,8 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_INVALUE;
if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
goto out_put;
@@ -256,6 +265,7 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb)
Re: [PATCH net] net: lapb: Decrease the refcount of "struct lapb_cb" in lapb_device_event
On 2020-12-31 18:43, Xie He wrote:
In lapb_device_event, lapb_devtostruct is called to get a reference to
an object of "struct lapb_cb". lapb_devtostruct increases the refcount
of the object and returns a pointer to it. However, we didn't decrease
the refcount after we finished using the pointer. This patch fixes this
problem.
Fixes: a4989fa91110 ("net/lapb: support netdev events")
Cc: Martin Schiller
Signed-off-by: Xie He
---
net/lapb/lapb_iface.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 213ea7abc9ab..40961889e9c0 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -489,6 +489,7 @@ static int lapb_device_event(struct notifier_block
*this, unsigned long event,
break;
}
+ lapb_put(lapb);
return NOTIFY_DONE;
}
Well, I guess I missed that one. Thank you!
Acked-by: Martin Schiller
Re: [PATCH AUTOSEL 5.4 075/130] net/lapb: fix t1 timer handling for LAPB_STATE_0
On 2020-12-24 10:49, Xie He wrote: On Wed, Dec 23, 2020 at 9:01 AM Xie He wrote: I don't think this patch is suitable for stable branches. This patch is part of a patch series that changes the lapb module from "establishing the L2 connection only when needed by L3", to "establishing the L2 connection automatically whenever we are able to". This is a behavioral change. It should be seen as a new feature. It is not a bug fix. Applying this patch without other patches in the same series will also introduce problems, because this patch relies on part of the changes in the subsequent patch in the same series to be correct. Hi Martin, It's better that we avoid using words like "fix" in non-bug-fix patches, and make every patch work on its own without subsequent patches. Otherwise we'll make people confused. Yes, you are right.
Re: [PATCH net-next] net: lapbether: Consider it successful if (dis)connecting when already (dis)connected
On 2020-12-08 23:50, Xie He wrote:
When the upper layer instruct us to connect (or disconnect), but we
have
already connected (or disconnected), consider this operation successful
rather than failed.
This can help the upper layer to correct its record about whether we
are
connected or not here in layer 2.
The upper layer may not have the correct information about whether we
are
connected or not. This can happen if this driver has already been
running
for some time when the "x25" module gets loaded.
Another X.25 driver (hdlc_x25) is already doing this, so we make this
driver do this, too.
Looks good to me.
Acked-by: Martin Schiller
Cc: Martin Schiller
Signed-off-by: Xie He
---
drivers/net/wan/lapbether.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
index b6be2454b8bd..605fe555e157 100644
--- a/drivers/net/wan/lapbether.c
+++ b/drivers/net/wan/lapbether.c
@@ -55,6 +55,9 @@ struct lapbethdev {
static LIST_HEAD(lapbeth_devices);
+static void lapbeth_connected(struct net_device *dev, int reason);
+static void lapbeth_disconnected(struct net_device *dev, int reason);
+
/*
*/
/*
@@ -167,11 +170,17 @@ static netdev_tx_t lapbeth_xmit(struct sk_buff
*skb,
case X25_IFACE_DATA:
break;
case X25_IFACE_CONNECT:
- if ((err = lapb_connect_request(dev)) != LAPB_OK)
+ err = lapb_connect_request(dev);
+ if (err == LAPB_CONNECTED)
+ lapbeth_connected(dev, LAPB_OK);
+ else if (err != LAPB_OK)
pr_err("lapb_connect_request error: %d\n", err);
goto drop;
case X25_IFACE_DISCONNECT:
- if ((err = lapb_disconnect_request(dev)) != LAPB_OK)
+ err = lapb_disconnect_request(dev);
+ if (err == LAPB_NOTCONNECTED)
+ lapbeth_disconnected(dev, LAPB_OK);
+ else if (err != LAPB_OK)
pr_err("lapb_disconnect_request err: %d\n", err);
fallthrough;
default:
Re: [PATCH net-next] net: x25: Fix handling of Restart Request and Restart Confirmation
On 2020-12-09 21:16, Xie He wrote: On Wed, Dec 9, 2020 at 2:31 AM Martin Schiller wrote: >> 1. When the x25 module gets loaded, layer 2 may already be running and >> connected. In this case, although we are in X25_LINK_STATE_0, we still >> need to handle the Restart Request received, rather than ignore it. > > Hmm... I've never loaded the X.25 module after the interface is UP, but > in this case we really have to fix it. > This seems to be a regression caused by moving the Layer2 link handling into the lapb driver, which wasn't intended in my original patchset. I also have another patch on my todo list which aims orphan packet handling in the x25_receive_data() function. Maybe it is better to catch the whole thing there. OK.. Currently it's not clear to me what your future patches would be. Maybe we can first have this patch applied? Because based on the current code I think this patch is necessary. When you are ready to submit your patches, you can replace my code and we can discuss further. Yes, that's also the reason why I already acked this patch. We can solve this later a little bit cleaner if necessary. My patch that takes care of the orphaned packets in x25_receive_data() has again a dependency on other patches, especially the patch to configure the neighbor parameters (DCE/DTE, number of channels etc.), which I already sent before but still have to revise. Unfortunately I have only limited time for this topic, so I am not as fast as some people would wish. Sorry for that. Martin
Re: [PATCH net-next v7 4/5] net/x25: fix restart request/confirm handling
On 2020-12-09 23:11, Xie He wrote: On Wed, Dec 9, 2020 at 1:47 AM Xie He wrote: On Wed, Dec 9, 2020 at 1:41 AM Martin Schiller wrote: > > Right. > By the way: A "Restart Collision" is in practice a very common event to > establish the Layer 3. Oh, I see. Thanks! Hi Martin, When you submit future patch series, can you try ensuring the code to be in a completely working state after each patch in the series? This makes reviewing the patches easier. After the patches get applied, this also makes tracing bugs (for example, with "git bisect") through the commit history easier. Well I thought that's what patch series are for: Send patches that belong together and should be applied together. Of course I will try to make each patch work on its own, but this is not always possible with major changes or ends up in monster patches. And nobody wants that. Martin
Re: [PATCH net-next] net: x25: Fix handling of Restart Request and Restart Confirmation
On 2020-12-09 10:52, Martin Schiller wrote: On 2020-12-09 09:16, Xie He wrote: 1. When the x25 module gets loaded, layer 2 may already be running and connected. In this case, although we are in X25_LINK_STATE_0, we still need to handle the Restart Request received, rather than ignore it. Hmm... I've never loaded the X.25 module after the interface is UP, but in this case we really have to fix it. This seems to be a regression caused by moving the Layer2 link handling into the lapb driver, which wasn't intended in my original patchset. I also have another patch on my todo list which aims orphan packet handling in the x25_receive_data() function. Maybe it is better to catch the whole thing there. 2. When we are in X25_LINK_STATE_2, we have already sent a Restart Request and is waiting for the Restart Confirmation with t20timer. t20timer will restart itself repeatedly forever so it will always be there, as long as we are in State 2. So we don't need to check x25_t20timer_pending again. Yeah, you're right, we can actually leave that out. Acked-by: Martin Schiller
Re: [PATCH net-next] net: x25: Fix handling of Restart Request and Restart Confirmation
On 2020-12-09 09:16, Xie He wrote: 1. When the x25 module gets loaded, layer 2 may already be running and connected. In this case, although we are in X25_LINK_STATE_0, we still need to handle the Restart Request received, rather than ignore it. Hmm... I've never loaded the X.25 module after the interface is UP, but in this case we really have to fix it. 2. When we are in X25_LINK_STATE_2, we have already sent a Restart Request and is waiting for the Restart Confirmation with t20timer. t20timer will restart itself repeatedly forever so it will always be there, as long as we are in State 2. So we don't need to check x25_t20timer_pending again. Yeah, you're right, we can actually leave that out. Acked-by: Martin Schiller
Re: [PATCH net-next v7 4/5] net/x25: fix restart request/confirm handling
On 2020-12-09 10:17, Xie He wrote:
On Wed, Dec 9, 2020 at 1:01 AM Xie He wrote:
On Wed, Nov 25, 2020 at 10:36 PM Martin Schiller
wrote:
>
> switch (nb->state) {
> case X25_LINK_STATE_0:
> - nb->state = X25_LINK_STATE_2;
> - break;
> case X25_LINK_STATE_1:
> x25_transmit_restart_request(nb);
> nb->state = X25_LINK_STATE_2;
What is the reason for this change? Originally only the connecting
side will transmit a Restart Request; the connected side will not and
will only wait for the Restart Request to come. Now both sides will
transmit Restart Requests at the same time. I think we should better
avoid collision situations like this.
Oh. I see. Because in other patches we are giving L2 the ability to
connect by itself, both sides can now appear here to be the
"connected" side. So we can't make the "connected" side wait as we did
before.
Right.
By the way: A "Restart Collision" is in practice a very common event to
establish the Layer 3.
[PATCH net-next v7 5/5] net/x25: remove x25_kill_by_device()
Remove obsolete function x25_kill_by_device(). It's not used any more.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c | 16
1 file changed, 16 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 313a6222ded9..1432a05805ab 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -199,22 +199,6 @@ static void x25_remove_socket(struct sock *sk)
write_unlock_bh(_list_lock);
}
-/*
- * Kill all bound sockets on a dropped device.
- */
-static void x25_kill_by_device(struct net_device *dev)
-{
- struct sock *s;
-
- write_lock_bh(_list_lock);
-
- sk_for_each(s, _list)
- if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
- x25_disconnect(s, ENETUNREACH, 0, 0);
-
- write_unlock_bh(_list_lock);
-}
-
/*
* Handle device status changes.
*/
--
2.20.1
[PATCH net-next v7 3/5] net/lapb: fix t1 timer handling for LAPB_STATE_0
1. DTE interface changes immediately to LAPB_STATE_1 and start sending
SABM(E).
2. DCE interface sends N2-times DM and changes to LAPB_STATE_1
afterwards if there is no response in the meantime.
Signed-off-by: Martin Schiller
---
net/lapb/lapb_timer.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 8f5b17001a07..baa247fe4ed0 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -85,11 +85,18 @@ static void lapb_t1timer_expiry(struct timer_list *t)
switch (lapb->state) {
/*
-* If we are a DCE, keep going DM .. DM .. DM
+* If we are a DCE, send DM up to N2 times, then switch to
+* STATE_1 and send SABM(E).
*/
case LAPB_STATE_0:
- if (lapb->mode & LAPB_DCE)
+ if (lapb->mode & LAPB_DCE &&
+ lapb->n2count != lapb->n2) {
+ lapb->n2count++;
lapb_send_control(lapb, LAPB_DM, LAPB_POLLOFF,
LAPB_RESPONSE);
+ } else {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
break;
/*
--
2.20.1
[PATCH net-next v7 4/5] net/x25: fix restart request/confirm handling
We have to take the actual link state into account to handle
restart requests/confirms well.
Signed-off-by: Martin Schiller
---
net/x25/x25_link.c | 41 +
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index 11e868aa625d..f92073f3cb11 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -74,16 +74,43 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh
*nb,
switch (frametype) {
case X25_RESTART_REQUEST:
- confirm = !x25_t20timer_pending(nb);
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
- if (confirm)
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ confirm = !x25_t20timer_pending(nb);
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ if (confirm)
+ x25_transmit_restart_confirmation(nb);
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
x25_transmit_restart_confirmation(nb);
+ break;
+ }
break;
case X25_RESTART_CONFIRMATION:
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ if (x25_t20timer_pending(nb)) {
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ } else {
+ x25_transmit_restart_request(nb);
+ x25_start_t20timer(nb);
+ }
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
+ x25_transmit_restart_request(nb);
+ nb->state = X25_LINK_STATE_2;
+ x25_start_t20timer(nb);
+ break;
+ }
break;
case X25_DIAGNOSTIC:
@@ -214,8 +241,6 @@ void x25_link_established(struct x25_neigh *nb)
{
switch (nb->state) {
case X25_LINK_STATE_0:
- nb->state = X25_LINK_STATE_2;
- break;
case X25_LINK_STATE_1:
x25_transmit_restart_request(nb);
nb->state = X25_LINK_STATE_2;
--
2.20.1
[PATCH net-next v7 1/5] net/x25: handle additional netdev events
1. Add / remove x25_link_device by NETDEV_REGISTER/UNREGISTER and also
by NETDEV_POST_TYPE_CHANGE/NETDEV_PRE_TYPE_CHANGE.
This change is needed so that the x25_neigh struct for an interface
is already created when it shows up and is kept independently if the
interface goes UP or DOWN.
This is used in an upcomming commit, where x25 params of an neighbour
will get configurable through ioctls.
2. NETDEV_CHANGE event makes it possible to handle carrier loss and
detection. If carrier is lost, clean up everything related to this
neighbour by calling x25_link_terminated().
3. Also call x25_link_terminated() for NETDEV_DOWN events and remove the
call to x25_clear_forward_by_dev() in x25_route_device_down(), as
this is already called by x25_kill_by_neigh() which gets called by
x25_link_terminated().
4. Do nothing for NETDEV_UP and NETDEV_GOING_DOWN events, as these will
be handled in layer 2 (LAPB) and layer3 (X.25) will be informed by
layer2 when layer2 link is established and layer3 link should be
initiated.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c| 22 --
net/x25/x25_link.c | 6 +++---
net/x25/x25_route.c | 3 ---
3 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 046d3fee66a9..313a6222ded9 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -233,21 +233,31 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
#endif
) {
switch (event) {
- case NETDEV_UP:
+ case NETDEV_REGISTER:
+ case NETDEV_POST_TYPE_CHANGE:
x25_link_device_up(dev);
break;
- case NETDEV_GOING_DOWN:
+ case NETDEV_DOWN:
nb = x25_get_neigh(dev);
if (nb) {
- x25_terminate_link(nb);
+ x25_link_terminated(nb);
x25_neigh_put(nb);
}
- break;
- case NETDEV_DOWN:
- x25_kill_by_device(dev);
x25_route_device_down(dev);
+ break;
+ case NETDEV_PRE_TYPE_CHANGE:
+ case NETDEV_UNREGISTER:
x25_link_device_down(dev);
break;
+ case NETDEV_CHANGE:
+ if (!netif_carrier_ok(dev)) {
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_link_terminated(nb);
+ x25_neigh_put(nb);
+ }
+ }
+ break;
}
}
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index fdae054b7dc1..11e868aa625d 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -232,6 +232,9 @@ void x25_link_established(struct x25_neigh *nb)
void x25_link_terminated(struct x25_neigh *nb)
{
nb->state = X25_LINK_STATE_0;
+ skb_queue_purge(>queue);
+ x25_stop_t20timer(nb);
+
/* Out of order: clear existing virtual calls (X.25 03/93 4.6.3) */
x25_kill_by_neigh(nb);
}
@@ -277,9 +280,6 @@ void x25_link_device_up(struct net_device *dev)
*/
static void __x25_remove_neigh(struct x25_neigh *nb)
{
- skb_queue_purge(>queue);
- x25_stop_t20timer(nb);
-
if (nb->node.next) {
list_del(>node);
x25_neigh_put(nb);
diff --git a/net/x25/x25_route.c b/net/x25/x25_route.c
index 00e46c9a5280..ec2a39e9b3e6 100644
--- a/net/x25/x25_route.c
+++ b/net/x25/x25_route.c
@@ -115,9 +115,6 @@ void x25_route_device_down(struct net_device *dev)
__x25_remove_route(rt);
}
write_unlock_bh(_route_list_lock);
-
- /* Remove any related forwarding */
- x25_clear_forward_by_dev(dev);
}
/*
--
2.20.1
[PATCH net-next v7 2/5] net/lapb: support netdev events
This patch allows layer2 (LAPB) to react to netdev events itself and
avoids the detour via layer3 (X.25).
1. Establish layer2 on NETDEV_UP events, if the carrier is already up.
2. Call lapb_disconnect_request() on NETDEV_GOING_DOWN events to signal
the peer that the connection will go down.
(Only when the carrier is up.)
3. When a NETDEV_DOWN event occur, clear all queues, enter state
LAPB_STATE_0 and stop all timers.
4. The NETDEV_CHANGE event makes it possible to handle carrier loss and
detection.
In case of Carrier Loss, clear all queues, enter state LAPB_STATE_0
and stop all timers.
In case of Carrier Detection, we start timer t1 on a DCE interface,
and on a DTE interface we change to state LAPB_STATE_1 and start
sending SABM(E).
Signed-off-by: Martin Schiller
---
net/lapb/lapb_iface.c | 82 ++-
1 file changed, 81 insertions(+), 1 deletion(-)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 3c03f6512c5f..213ea7abc9ab 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -418,14 +418,94 @@ int lapb_data_transmit(struct lapb_cb *lapb, struct
sk_buff *skb)
return used;
}
+/* Handle device status changes. */
+static int lapb_device_event(struct notifier_block *this, unsigned long event,
+void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+ struct lapb_cb *lapb;
+
+ if (!net_eq(dev_net(dev), _net))
+ return NOTIFY_DONE;
+
+ if (dev->type != ARPHRD_X25)
+ return NOTIFY_DONE;
+
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ return NOTIFY_DONE;
+
+ switch (event) {
+ case NETDEV_UP:
+ lapb_dbg(0, "(%p) Interface up: %s\n", dev, dev->name);
+
+ if (netif_carrier_ok(dev)) {
+ lapb_dbg(0, "(%p): Carrier is already up: %s\n", dev,
+dev->name);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ if (lapb->state == LAPB_STATE_0) {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
+ }
+ }
+ break;
+ case NETDEV_GOING_DOWN:
+ if (netif_carrier_ok(dev))
+ lapb_disconnect_request(dev);
+ break;
+ case NETDEV_DOWN:
+ lapb_dbg(0, "(%p) Interface down: %s\n", dev, dev->name);
+ lapb_dbg(0, "(%p) S%d -> S0\n", dev, lapb->state);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2timer(lapb);
+ break;
+ case NETDEV_CHANGE:
+ if (netif_carrier_ok(dev)) {
+ lapb_dbg(0, "(%p): Carrier detected: %s\n", dev,
+dev->name);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ if (lapb->state == LAPB_STATE_0) {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
+ }
+ } else {
+ lapb_dbg(0, "(%p) Carrier lost: %s\n", dev, dev->name);
+ lapb_dbg(0, "(%p) S%d -> S0\n", dev, lapb->state);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2timer(lapb);
+ }
+ break;
+ }
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block lapb_dev_notifier = {
+ .notifier_call = lapb_device_event,
+};
+
static int __init lapb_init(void)
{
- return 0;
+ return register_netdevice_notifier(_dev_notifier);
}
static void __exit lapb_exit(void)
{
WARN_ON(!list_empty(_list));
+
+ unregister_netdevice_notifier(_dev_notifier);
}
MODULE_AUTHOR("Jonathan Naylor ");
--
2.20.1
[PATCH net-next v7 0/5] net/x25: netdev event handling
--- Changes to v6: o integrated some code styling suggestions by Jakub. Changes to v5: o fix numbering in commit message of patch 2/5. Changes to v4: o also establish layer2 (LAPB) on NETDEV_UP events, if the carrier is already UP. Changes to v3: o another complete rework of the patch-set to split event handling for layer2 (LAPB) and layer3 (X.25) Changes to v2: o restructure complete patch-set o keep netdev event handling in layer3 (X.25) o add patch to fix lapb_connect_request() for DCE o add patch to handle carrier loss correctly in lapb o drop patch for x25_neighbour param handling this may need fixes/cleanup and will be resubmitted later. Changes to v1: o fix 'subject_prefix' and 'checkpatch' warnings --- Martin Schiller (5): net/x25: handle additional netdev events net/lapb: support netdev events net/lapb: fix t1 timer handling for LAPB_STATE_0 net/x25: fix restart request/confirm handling net/x25: remove x25_kill_by_device() net/lapb/lapb_iface.c | 82 ++- net/lapb/lapb_timer.c | 11 -- net/x25/af_x25.c | 38 +--- net/x25/x25_link.c| 47 +++-- net/x25/x25_route.c | 3 -- 5 files changed, 142 insertions(+), 39 deletions(-) -- 2.20.1
Re: [PATCH net-next v6 2/5] net/lapb: support netdev events
On 2020-11-26 01:08, Xie He wrote: Hi Martin, Since we are going to assume lapb->state would remain in LAPB_STATE_0 when the carrier is down (as understood by me. Right?), could we add a check in lapb_connect_request to reject the upper layer's "connect" instruction when the carrier is down? Like this: No, because this will break the considered "on demand" calling feature. diff --git a/include/linux/lapb.h b/include/linux/lapb.h index eb56472f23b2..7923b1c6fc6a 100644 --- a/include/linux/lapb.h +++ b/include/linux/lapb.h @@ -14,6 +14,7 @@ #defineLAPB_REFUSED5 #defineLAPB_TIMEDOUT 6 #defineLAPB_NOMEM 7 +#defineLAPB_NOCARRIER 8 #defineLAPB_STANDARD 0x00 #defineLAPB_EXTENDED 0x01 diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c index 3c03f6512c5f..c909d8db1bef 100644 --- a/net/lapb/lapb_iface.c +++ b/net/lapb/lapb_iface.c @@ -270,6 +270,10 @@ int lapb_connect_request(struct net_device *dev) if (!lapb) goto out; + rc = LAPB_NOCARRIER; + if (!netif_carrier_ok(dev)) + goto out_put; + rc = LAPB_OK; if (lapb->state == LAPB_STATE_1) goto out_put; Also, since we are going to assume the lapb->state would remain in LAPB_STATE_0 when the carrier is down, are the "lapb->state == LAPB_STATE_0" checks in carrier-up/device-up event handling necessary? If they are not necessary, it might be better to remove them because it may confuse people reading the code. They are still necessary, because if the link setup is initiated by upper layers, we've already entered the respective state by lapb_connect_request(). Every suggestion for improvement is really welcome, but please let this patch set pass now, if you don't find any more gross errors. Martin
Re: [PATCH net-next v6 2/5] net/lapb: support netdev events
On 2020-11-25 22:49, Jakub Kicinski wrote:
On Tue, 24 Nov 2020 10:39:35 +0100 Martin Schiller wrote:
This patch allows layer2 (LAPB) to react to netdev events itself and
avoids the detour via layer3 (X.25).
1. Establish layer2 on NETDEV_UP events, if the carrier is already up.
2. Call lapb_disconnect_request() on NETDEV_GOING_DOWN events to
signal
the peer that the connection will go down.
(Only when the carrier is up.)
3. When a NETDEV_DOWN event occur, clear all queues, enter state
LAPB_STATE_0 and stop all timers.
4. The NETDEV_CHANGE event makes it possible to handle carrier loss
and
detection.
In case of Carrier Loss, clear all queues, enter state LAPB_STATE_0
and stop all timers.
In case of Carrier Detection, we start timer t1 on a DCE interface,
and on a DTE interface we change to state LAPB_STATE_1 and start
sending SABM(E).
Signed-off-by: Martin Schiller
+/* Handle device status changes. */
+static int lapb_device_event(struct notifier_block *this, unsigned
long event,
+void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+ struct lapb_cb *lapb;
+
+ if (!net_eq(dev_net(dev), _net))
+ return NOTIFY_DONE;
+
+ if (dev->type == ARPHRD_X25) {
Flip condition, save indentation.
if (dev->type != ARPHRD_X25)
return NOTIFY_DONE;
You can also pull out of all the cases:
lapb = lapb_devtostruct(dev);
if (!lapb)
return NOTIFY_DONE;
right?
+ switch (event) {
+ case NETDEV_UP:
+ lapb_dbg(0, "(%p) Interface up: %s\n", dev,
+dev->name);
+
+ if (netif_carrier_ok(dev)) {
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
static int __init lapb_init(void)
{
+ register_netdevice_notifier(_dev_notifier);
This can fail, so:
return register_netdevice_notifier(_dev_notifier);
return 0;
}
static void __exit lapb_exit(void)
{
WARN_ON(!list_empty(_list));
+
+ unregister_netdevice_notifier(_dev_notifier);
}
MODULE_AUTHOR("Jonathan Naylor ");
Thanks for your hints! I will send an update.
Re: [PATCH net-next v5 3/5] net/lapb: fix t1 timer handling for LAPB_STATE_0
On 2020-11-24 12:43, David Laight wrote:
From: Martin Schiller
Sent: 24 November 2020 09:36
1. DTE interface changes immediately to LAPB_STATE_1 and start sending
SABM(E).
2. DCE interface sends N2-times DM and changes to LAPB_STATE_1
afterwards if there is no response in the meantime.
Seems reasonable.
It is 35 years since I wrote LAPB and I can't exactly remember
what we did.
If I stole a copy of the code it's on a QIC-150 tape cartridge!
I really don't remember having a DTE/DCE option.
It is likely that LAPB came up sending DM (response without F)
until level3 requested the link come up when it would send
N2 SABM+P hoping to get a UA+F.
It would then send DM-F until a retry request was made.
We certainly had several different types of crossover connectors
for DTE-DTE working.
David
The support for DTE/DCE was already in the LAPB code and I made it
configurable from userspace (at least for hdlc interfaces) with this
commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f362e5fe0f1f
For Layer3 (X.25) I will add it with an addional patch (you already
commented on that) on a next step.
The described behaviour above is my interpretation of point 2.4.4.1 of
the "ITU-T Recommendation X.25 (10/96) aka "Blue Book" [1].
[1] https://www.itu.int/rec/T-REC-X.25-199610-I/
Signed-off-by: Martin Schiller
---
net/lapb/lapb_timer.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 8f5b17001a07..baa247fe4ed0 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -85,11 +85,18 @@ static void lapb_t1timer_expiry(struct timer_list
*t)
switch (lapb->state) {
/*
-* If we are a DCE, keep going DM .. DM .. DM
+* If we are a DCE, send DM up to N2 times, then switch to
+* STATE_1 and send SABM(E).
*/
case LAPB_STATE_0:
- if (lapb->mode & LAPB_DCE)
+ if (lapb->mode & LAPB_DCE &&
+ lapb->n2count != lapb->n2) {
+ lapb->n2count++;
lapb_send_control(lapb, LAPB_DM, LAPB_POLLOFF,
LAPB_RESPONSE);
+ } else {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
break;
/*
--
2.20.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
MK1 1PT, UK
Registration No: 1397386 (Wales)
[PATCH net-next v6 4/5] net/x25: fix restart request/confirm handling
We have to take the actual link state into account to handle
restart requests/confirms well.
Signed-off-by: Martin Schiller
---
net/x25/x25_link.c | 41 +
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index 11e868aa625d..f92073f3cb11 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -74,16 +74,43 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh
*nb,
switch (frametype) {
case X25_RESTART_REQUEST:
- confirm = !x25_t20timer_pending(nb);
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
- if (confirm)
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ confirm = !x25_t20timer_pending(nb);
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ if (confirm)
+ x25_transmit_restart_confirmation(nb);
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
x25_transmit_restart_confirmation(nb);
+ break;
+ }
break;
case X25_RESTART_CONFIRMATION:
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ if (x25_t20timer_pending(nb)) {
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ } else {
+ x25_transmit_restart_request(nb);
+ x25_start_t20timer(nb);
+ }
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
+ x25_transmit_restart_request(nb);
+ nb->state = X25_LINK_STATE_2;
+ x25_start_t20timer(nb);
+ break;
+ }
break;
case X25_DIAGNOSTIC:
@@ -214,8 +241,6 @@ void x25_link_established(struct x25_neigh *nb)
{
switch (nb->state) {
case X25_LINK_STATE_0:
- nb->state = X25_LINK_STATE_2;
- break;
case X25_LINK_STATE_1:
x25_transmit_restart_request(nb);
nb->state = X25_LINK_STATE_2;
--
2.20.1
[PATCH net-next v6 5/5] net/x25: remove x25_kill_by_device()
Remove obsolete function x25_kill_by_device(). It's not used any more.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c | 16
1 file changed, 16 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 313a6222ded9..1432a05805ab 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -199,22 +199,6 @@ static void x25_remove_socket(struct sock *sk)
write_unlock_bh(_list_lock);
}
-/*
- * Kill all bound sockets on a dropped device.
- */
-static void x25_kill_by_device(struct net_device *dev)
-{
- struct sock *s;
-
- write_lock_bh(_list_lock);
-
- sk_for_each(s, _list)
- if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
- x25_disconnect(s, ENETUNREACH, 0, 0);
-
- write_unlock_bh(_list_lock);
-}
-
/*
* Handle device status changes.
*/
--
2.20.1
[PATCH net-next v6 2/5] net/lapb: support netdev events
This patch allows layer2 (LAPB) to react to netdev events itself and
avoids the detour via layer3 (X.25).
1. Establish layer2 on NETDEV_UP events, if the carrier is already up.
2. Call lapb_disconnect_request() on NETDEV_GOING_DOWN events to signal
the peer that the connection will go down.
(Only when the carrier is up.)
3. When a NETDEV_DOWN event occur, clear all queues, enter state
LAPB_STATE_0 and stop all timers.
4. The NETDEV_CHANGE event makes it possible to handle carrier loss and
detection.
In case of Carrier Loss, clear all queues, enter state LAPB_STATE_0
and stop all timers.
In case of Carrier Detection, we start timer t1 on a DCE interface,
and on a DTE interface we change to state LAPB_STATE_1 and start
sending SABM(E).
Signed-off-by: Martin Schiller
---
net/lapb/lapb_iface.c | 94 +++
1 file changed, 94 insertions(+)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 3c03f6512c5f..f226d354aaf0 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -418,14 +418,108 @@ int lapb_data_transmit(struct lapb_cb *lapb, struct
sk_buff *skb)
return used;
}
+/* Handle device status changes. */
+static int lapb_device_event(struct notifier_block *this, unsigned long event,
+void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+ struct lapb_cb *lapb;
+
+ if (!net_eq(dev_net(dev), _net))
+ return NOTIFY_DONE;
+
+ if (dev->type == ARPHRD_X25) {
+ switch (event) {
+ case NETDEV_UP:
+ lapb_dbg(0, "(%p) Interface up: %s\n", dev,
+dev->name);
+
+ if (netif_carrier_ok(dev)) {
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
+
+ lapb_dbg(0, "(%p): Carrier is already up: %s\n",
+dev, dev->name);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ if (lapb->state == LAPB_STATE_0) {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
+ }
+ }
+ break;
+ case NETDEV_GOING_DOWN:
+ if (netif_carrier_ok(dev))
+ lapb_disconnect_request(dev);
+ break;
+ case NETDEV_DOWN:
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
+
+ lapb_dbg(0, "(%p) Interface down: %s\n", dev,
+dev->name);
+ lapb_dbg(0, "(%p) S%d -> S0\n", dev,
+lapb->state);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2timer(lapb);
+ break;
+ case NETDEV_CHANGE:
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
+
+ if (netif_carrier_ok(dev)) {
+ lapb_dbg(0, "(%p): Carrier detected: %s\n",
+dev, dev->name);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ if (lapb->state == LAPB_STATE_0) {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
+ }
+ } else {
+ lapb_dbg(0, "(%p) Carrier lost: %s\n", dev,
+dev->name);
+ lapb_dbg(0, "(%p) S%d -> S0\n", dev,
+lapb->state);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2ti
[PATCH net-next v6 3/5] net/lapb: fix t1 timer handling for LAPB_STATE_0
1. DTE interface changes immediately to LAPB_STATE_1 and start sending
SABM(E).
2. DCE interface sends N2-times DM and changes to LAPB_STATE_1
afterwards if there is no response in the meantime.
Signed-off-by: Martin Schiller
---
net/lapb/lapb_timer.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 8f5b17001a07..baa247fe4ed0 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -85,11 +85,18 @@ static void lapb_t1timer_expiry(struct timer_list *t)
switch (lapb->state) {
/*
-* If we are a DCE, keep going DM .. DM .. DM
+* If we are a DCE, send DM up to N2 times, then switch to
+* STATE_1 and send SABM(E).
*/
case LAPB_STATE_0:
- if (lapb->mode & LAPB_DCE)
+ if (lapb->mode & LAPB_DCE &&
+ lapb->n2count != lapb->n2) {
+ lapb->n2count++;
lapb_send_control(lapb, LAPB_DM, LAPB_POLLOFF,
LAPB_RESPONSE);
+ } else {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
break;
/*
--
2.20.1
[PATCH net-next v6 1/5] net/x25: handle additional netdev events
1. Add / remove x25_link_device by NETDEV_REGISTER/UNREGISTER and also
by NETDEV_POST_TYPE_CHANGE/NETDEV_PRE_TYPE_CHANGE.
This change is needed so that the x25_neigh struct for an interface
is already created when it shows up and is kept independently if the
interface goes UP or DOWN.
This is used in an upcomming commit, where x25 params of an neighbour
will get configurable through ioctls.
2. NETDEV_CHANGE event makes it possible to handle carrier loss and
detection. If carrier is lost, clean up everything related to this
neighbour by calling x25_link_terminated().
3. Also call x25_link_terminated() for NETDEV_DOWN events and remove the
call to x25_clear_forward_by_dev() in x25_route_device_down(), as
this is already called by x25_kill_by_neigh() which gets called by
x25_link_terminated().
4. Do nothing for NETDEV_UP and NETDEV_GOING_DOWN events, as these will
be handled in layer 2 (LAPB) and layer3 (X.25) will be informed by
layer2 when layer2 link is established and layer3 link should be
initiated.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c| 22 --
net/x25/x25_link.c | 6 +++---
net/x25/x25_route.c | 3 ---
3 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 046d3fee66a9..313a6222ded9 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -233,21 +233,31 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
#endif
) {
switch (event) {
- case NETDEV_UP:
+ case NETDEV_REGISTER:
+ case NETDEV_POST_TYPE_CHANGE:
x25_link_device_up(dev);
break;
- case NETDEV_GOING_DOWN:
+ case NETDEV_DOWN:
nb = x25_get_neigh(dev);
if (nb) {
- x25_terminate_link(nb);
+ x25_link_terminated(nb);
x25_neigh_put(nb);
}
- break;
- case NETDEV_DOWN:
- x25_kill_by_device(dev);
x25_route_device_down(dev);
+ break;
+ case NETDEV_PRE_TYPE_CHANGE:
+ case NETDEV_UNREGISTER:
x25_link_device_down(dev);
break;
+ case NETDEV_CHANGE:
+ if (!netif_carrier_ok(dev)) {
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_link_terminated(nb);
+ x25_neigh_put(nb);
+ }
+ }
+ break;
}
}
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index fdae054b7dc1..11e868aa625d 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -232,6 +232,9 @@ void x25_link_established(struct x25_neigh *nb)
void x25_link_terminated(struct x25_neigh *nb)
{
nb->state = X25_LINK_STATE_0;
+ skb_queue_purge(>queue);
+ x25_stop_t20timer(nb);
+
/* Out of order: clear existing virtual calls (X.25 03/93 4.6.3) */
x25_kill_by_neigh(nb);
}
@@ -277,9 +280,6 @@ void x25_link_device_up(struct net_device *dev)
*/
static void __x25_remove_neigh(struct x25_neigh *nb)
{
- skb_queue_purge(>queue);
- x25_stop_t20timer(nb);
-
if (nb->node.next) {
list_del(>node);
x25_neigh_put(nb);
diff --git a/net/x25/x25_route.c b/net/x25/x25_route.c
index 00e46c9a5280..ec2a39e9b3e6 100644
--- a/net/x25/x25_route.c
+++ b/net/x25/x25_route.c
@@ -115,9 +115,6 @@ void x25_route_device_down(struct net_device *dev)
__x25_remove_route(rt);
}
write_unlock_bh(_route_list_lock);
-
- /* Remove any related forwarding */
- x25_clear_forward_by_dev(dev);
}
/*
--
2.20.1
[PATCH net-next v6 0/5] net/x25: netdev event handling
--- Changes to v5: o fix numbering in commit message of patch 2/5. Changes to v4: o also establish layer2 (LAPB) on NETDEV_UP events, if the carrier is already UP. Changes to v3: o another complete rework of the patch-set to split event handling for layer2 (LAPB) and layer3 (X.25) Changes to v2: o restructure complete patch-set o keep netdev event handling in layer3 (X.25) o add patch to fix lapb_connect_request() for DCE o add patch to handle carrier loss correctly in lapb o drop patch for x25_neighbour param handling this may need fixes/cleanup and will be resubmitted later. Changes to v1: o fix 'subject_prefix' and 'checkpatch' warnings --- Martin Schiller (5): net/x25: handle additional netdev events net/lapb: support netdev events net/lapb: fix t1 timer handling for LAPB_STATE_0 net/x25: fix restart request/confirm handling net/x25: remove x25_kill_by_device() net/lapb/lapb_iface.c | 94 +++ net/lapb/lapb_timer.c | 11 - net/x25/af_x25.c | 38 - net/x25/x25_link.c| 47 +- net/x25/x25_route.c | 3 -- 5 files changed, 155 insertions(+), 38 deletions(-) -- 2.20.1
[PATCH net-next v5 5/5] net/x25: remove x25_kill_by_device()
Remove obsolete function x25_kill_by_device(). It's not used any more.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c | 16
1 file changed, 16 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 313a6222ded9..1432a05805ab 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -199,22 +199,6 @@ static void x25_remove_socket(struct sock *sk)
write_unlock_bh(_list_lock);
}
-/*
- * Kill all bound sockets on a dropped device.
- */
-static void x25_kill_by_device(struct net_device *dev)
-{
- struct sock *s;
-
- write_lock_bh(_list_lock);
-
- sk_for_each(s, _list)
- if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
- x25_disconnect(s, ENETUNREACH, 0, 0);
-
- write_unlock_bh(_list_lock);
-}
-
/*
* Handle device status changes.
*/
--
2.20.1
[PATCH net-next v5 3/5] net/lapb: fix t1 timer handling for LAPB_STATE_0
1. DTE interface changes immediately to LAPB_STATE_1 and start sending
SABM(E).
2. DCE interface sends N2-times DM and changes to LAPB_STATE_1
afterwards if there is no response in the meantime.
Signed-off-by: Martin Schiller
---
net/lapb/lapb_timer.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 8f5b17001a07..baa247fe4ed0 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -85,11 +85,18 @@ static void lapb_t1timer_expiry(struct timer_list *t)
switch (lapb->state) {
/*
-* If we are a DCE, keep going DM .. DM .. DM
+* If we are a DCE, send DM up to N2 times, then switch to
+* STATE_1 and send SABM(E).
*/
case LAPB_STATE_0:
- if (lapb->mode & LAPB_DCE)
+ if (lapb->mode & LAPB_DCE &&
+ lapb->n2count != lapb->n2) {
+ lapb->n2count++;
lapb_send_control(lapb, LAPB_DM, LAPB_POLLOFF,
LAPB_RESPONSE);
+ } else {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
break;
/*
--
2.20.1
[PATCH net-next v5 4/5] net/x25: fix restart request/confirm handling
We have to take the actual link state into account to handle
restart requests/confirms well.
Signed-off-by: Martin Schiller
---
net/x25/x25_link.c | 41 +
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index 11e868aa625d..f92073f3cb11 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -74,16 +74,43 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh
*nb,
switch (frametype) {
case X25_RESTART_REQUEST:
- confirm = !x25_t20timer_pending(nb);
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
- if (confirm)
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ confirm = !x25_t20timer_pending(nb);
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ if (confirm)
+ x25_transmit_restart_confirmation(nb);
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
x25_transmit_restart_confirmation(nb);
+ break;
+ }
break;
case X25_RESTART_CONFIRMATION:
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ if (x25_t20timer_pending(nb)) {
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ } else {
+ x25_transmit_restart_request(nb);
+ x25_start_t20timer(nb);
+ }
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
+ x25_transmit_restart_request(nb);
+ nb->state = X25_LINK_STATE_2;
+ x25_start_t20timer(nb);
+ break;
+ }
break;
case X25_DIAGNOSTIC:
@@ -214,8 +241,6 @@ void x25_link_established(struct x25_neigh *nb)
{
switch (nb->state) {
case X25_LINK_STATE_0:
- nb->state = X25_LINK_STATE_2;
- break;
case X25_LINK_STATE_1:
x25_transmit_restart_request(nb);
nb->state = X25_LINK_STATE_2;
--
2.20.1
[PATCH net-next v5 2/5] net/lapb: support netdev events
This patch allows layer2 (LAPB) to react to netdev events itself and
avoids the detour via layer3 (X.25).
1. Establish layer2 on NETDEV_UP events, if the carrier is already up.
2. Call lapb_disconnect_request() on NETDEV_GOING_DOWN events to signal
the peer that the connection will go down.
(Only when the carrier is up.)
2. When a NETDEV_DOWN event occur, clear all queues, enter state
LAPB_STATE_0 and stop all timers.
3. The NETDEV_CHANGE event makes it possible to handle carrier loss and
detection.
In case of Carrier Loss, clear all queues, enter state LAPB_STATE_0
and stop all timers.
In case of Carrier Detection, we start timer t1 on a DCE interface,
and on a DTE interface we change to state LAPB_STATE_1 and start
sending SABM(E).
Signed-off-by: Martin Schiller
---
net/lapb/lapb_iface.c | 94 +++
1 file changed, 94 insertions(+)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 3c03f6512c5f..f226d354aaf0 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -418,14 +418,108 @@ int lapb_data_transmit(struct lapb_cb *lapb, struct
sk_buff *skb)
return used;
}
+/* Handle device status changes. */
+static int lapb_device_event(struct notifier_block *this, unsigned long event,
+void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+ struct lapb_cb *lapb;
+
+ if (!net_eq(dev_net(dev), _net))
+ return NOTIFY_DONE;
+
+ if (dev->type == ARPHRD_X25) {
+ switch (event) {
+ case NETDEV_UP:
+ lapb_dbg(0, "(%p) Interface up: %s\n", dev,
+dev->name);
+
+ if (netif_carrier_ok(dev)) {
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
+
+ lapb_dbg(0, "(%p): Carrier is already up: %s\n",
+dev, dev->name);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ if (lapb->state == LAPB_STATE_0) {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
+ }
+ }
+ break;
+ case NETDEV_GOING_DOWN:
+ if (netif_carrier_ok(dev))
+ lapb_disconnect_request(dev);
+ break;
+ case NETDEV_DOWN:
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
+
+ lapb_dbg(0, "(%p) Interface down: %s\n", dev,
+dev->name);
+ lapb_dbg(0, "(%p) S%d -> S0\n", dev,
+lapb->state);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2timer(lapb);
+ break;
+ case NETDEV_CHANGE:
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
+
+ if (netif_carrier_ok(dev)) {
+ lapb_dbg(0, "(%p): Carrier detected: %s\n",
+dev, dev->name);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ if (lapb->state == LAPB_STATE_0) {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
+ }
+ } else {
+ lapb_dbg(0, "(%p) Carrier lost: %s\n", dev,
+dev->name);
+ lapb_dbg(0, "(%p) S%d -> S0\n", dev,
+lapb->state);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2ti
[PATCH net-next v5 1/5] net/x25: handle additional netdev events
1. Add / remove x25_link_device by NETDEV_REGISTER/UNREGISTER and also
by NETDEV_POST_TYPE_CHANGE/NETDEV_PRE_TYPE_CHANGE.
This change is needed so that the x25_neigh struct for an interface
is already created when it shows up and is kept independently if the
interface goes UP or DOWN.
This is used in an upcomming commit, where x25 params of an neighbour
will get configurable through ioctls.
2. NETDEV_CHANGE event makes it possible to handle carrier loss and
detection. If carrier is lost, clean up everything related to this
neighbour by calling x25_link_terminated().
3. Also call x25_link_terminated() for NETDEV_DOWN events and remove the
call to x25_clear_forward_by_dev() in x25_route_device_down(), as
this is already called by x25_kill_by_neigh() which gets called by
x25_link_terminated().
4. Do nothing for NETDEV_UP and NETDEV_GOING_DOWN events, as these will
be handled in layer 2 (LAPB) and layer3 (X.25) will be informed by
layer2 when layer2 link is established and layer3 link should be
initiated.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c| 22 --
net/x25/x25_link.c | 6 +++---
net/x25/x25_route.c | 3 ---
3 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 046d3fee66a9..313a6222ded9 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -233,21 +233,31 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
#endif
) {
switch (event) {
- case NETDEV_UP:
+ case NETDEV_REGISTER:
+ case NETDEV_POST_TYPE_CHANGE:
x25_link_device_up(dev);
break;
- case NETDEV_GOING_DOWN:
+ case NETDEV_DOWN:
nb = x25_get_neigh(dev);
if (nb) {
- x25_terminate_link(nb);
+ x25_link_terminated(nb);
x25_neigh_put(nb);
}
- break;
- case NETDEV_DOWN:
- x25_kill_by_device(dev);
x25_route_device_down(dev);
+ break;
+ case NETDEV_PRE_TYPE_CHANGE:
+ case NETDEV_UNREGISTER:
x25_link_device_down(dev);
break;
+ case NETDEV_CHANGE:
+ if (!netif_carrier_ok(dev)) {
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_link_terminated(nb);
+ x25_neigh_put(nb);
+ }
+ }
+ break;
}
}
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index fdae054b7dc1..11e868aa625d 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -232,6 +232,9 @@ void x25_link_established(struct x25_neigh *nb)
void x25_link_terminated(struct x25_neigh *nb)
{
nb->state = X25_LINK_STATE_0;
+ skb_queue_purge(>queue);
+ x25_stop_t20timer(nb);
+
/* Out of order: clear existing virtual calls (X.25 03/93 4.6.3) */
x25_kill_by_neigh(nb);
}
@@ -277,9 +280,6 @@ void x25_link_device_up(struct net_device *dev)
*/
static void __x25_remove_neigh(struct x25_neigh *nb)
{
- skb_queue_purge(>queue);
- x25_stop_t20timer(nb);
-
if (nb->node.next) {
list_del(>node);
x25_neigh_put(nb);
diff --git a/net/x25/x25_route.c b/net/x25/x25_route.c
index 00e46c9a5280..ec2a39e9b3e6 100644
--- a/net/x25/x25_route.c
+++ b/net/x25/x25_route.c
@@ -115,9 +115,6 @@ void x25_route_device_down(struct net_device *dev)
__x25_remove_route(rt);
}
write_unlock_bh(_route_list_lock);
-
- /* Remove any related forwarding */
- x25_clear_forward_by_dev(dev);
}
/*
--
2.20.1
[PATCH net-next v5 0/5] net/x25: netdev event handling
--- Changes to v4: o also establish layer2 (LAPB) on NETDEV_UP events, if the carrier is already UP. Changes to v3: o another complete rework of the patch-set to split event handling for layer2 (LAPB) and layer3 (X.25) Changes to v2: o restructure complete patch-set o keep netdev event handling in layer3 (X.25) o add patch to fix lapb_connect_request() for DCE o add patch to handle carrier loss correctly in lapb o drop patch for x25_neighbour param handling this may need fixes/cleanup and will be resubmitted later. Changes to v1: o fix 'subject_prefix' and 'checkpatch' warnings --- Martin Schiller (5): net/x25: handle additional netdev events net/lapb: support netdev events net/lapb: fix t1 timer handling for LAPB_STATE_0 net/x25: fix restart request/confirm handling net/x25: remove x25_kill_by_device() net/lapb/lapb_iface.c | 94 +++ net/lapb/lapb_timer.c | 11 - net/x25/af_x25.c | 38 - net/x25/x25_link.c| 47 +- net/x25/x25_route.c | 3 -- 5 files changed, 155 insertions(+), 38 deletions(-) -- 2.20.1
Re: [PATCH net-next v4 2/5] net/lapb: support netdev events
On 2020-11-23 23:09, Xie He wrote: On Mon, Nov 23, 2020 at 11:36 AM Jakub Kicinski wrote: > > From this point of view it will be the best to handle the NETDEV_UP in > > the lapb event handler and establish the link analog to the > > NETDEV_CHANGE event if the carrier is UP. > > Thanks! This way we can make sure LAPB would automatically connect in > all situations. > > Since we'll have a netif_carrier_ok check in NETDEV_UP handing, it > might make the code look prettier to also have a netif_carrier_ok > check in NETDEV_GOING_DOWN handing (for symmetry). Just a suggestion. > You can do whatever looks good to you :) Xie other than this the patches look good to you? Martin should I expect a respin to follow Xie's suggestion or should I apply v4? There should be a respin because we need to handle the NETDEV_UP event. The lapbether driver (and possibly some HDLC WAN drivers) doesn't generate carrier events so we need to do auto-connect in the NETDEV_UP event. I'll send a v5 with the appropriate change.
Re: [PATCH net-next v4 2/5] net/lapb: support netdev events
On 2020-11-23 11:08, Xie He wrote: On Mon, Nov 23, 2020 at 1:36 AM Xie He wrote: Some drivers don't support carrier status and will never change it. Their carrier status will always be UP. There will not be a NETDEV_CHANGE event. Well, one could argue that we would have to repair these drivers, but I don't think that will get us anywhere. From this point of view it will be the best to handle the NETDEV_UP in the lapb event handler and establish the link analog to the NETDEV_CHANGE event if the carrier is UP. lapbether doesn't change carrier status. I also have my own virtual HDLC WAN driver (for testing) which also doesn't change carrier status. I just tested with lapbether. When I bring up the interface, there will only be NETDEV_PRE_UP and then NETDEV_UP. There will not be NETDEV_CHANGE. The carrier status is alway UP. I haven't tested whether a device can receive NETDEV_CHANGE when it is down. It's possible for a device driver to call netif_carrier_on when the interface is down. Do you know what will happen if a device driver calls netif_carrier_on when the interface is down? I just did a test on lapbether and saw there would be no NETDEV_CHANGE event when the netif is down, even if netif_carrier_on/off is called. So we can rest assured of this part.
Re: [PATCH net-next v4 2/5] net/lapb: support netdev events
On 2020-11-23 09:31, Xie He wrote: On Sun, Nov 22, 2020 at 10:55 PM Martin Schiller wrote: No, they aren't independent. The carrier can only be up if the device / interface is UP. And as far as I can see a NETDEV_CHANGE event will also only be generated on interfaces that are UP. So you can be sure, that if there is a NETDEV_CHANGE event then the device is UP. OK. Thanks for your explanation! I removed the NETDEV_UP handling because I don't think it makes sense to implicitly try to establish layer2 (LAPB) if there is no carrier. As I understand, when the device goes up, the carrier can be either down or up. Right? If this is true, when a device goes up and the carrier then goes up after that, L2 will automatically connect, but if a device goes up and the carrier is already up, L2 will not automatically connect. I think it might be better to eliminate this difference in handling. It might be better to make it automatically connect in both situations, or in neither situations. AFAIK the carrier can't be up before the device is up. Therefore, there will be a NETDEV_CHANGE event after the NETDEV_UP event. This is what I can see in my tests (with the HDLC interface). Is the behaviour different for e.g. lapbether? If you want to go with the second way (auto connect in neither situations), the next (3rd) patch of this series might be also not needed. I just want to make the behavior of LAPB more consistent. I think we should either make LAPB auto-connect in all situations, or make LAPB wait for L3's instruction to connect in all situations. And with the first X.25 connection request on that interface, it will be established anyway by x25_transmit_link(). I've tested it here with an HDLC WAN Adapter and it works as expected. These are also the ideal conditions for the already mentioned "on demand" scenario. The only necessary change would be to call x25_terminate_link() on an interface after clearing the last X.25 session. > On NETDEV_GOING_DOWN, we can also check the carrier status first and > if it is down, we don't need to call lapb_disconnect_request. This is not necessary because lapb_disconnect_request() checks the current state. And if the carrier is DOWN then the state should also be LAPB_STATE_0 and so lapb_disconnect_request() does nothing. Yes, I understand. I just thought adding this check might make the code cleaner. But you are right.
Re: [PATCH net-next v4 2/5] net/lapb: support netdev events
On 2020-11-21 00:50, Xie He wrote: On Fri, Nov 20, 2020 at 3:11 PM Xie He wrote: Should we also handle the NETDEV_UP event here? In previous versions of this patch series you seemed to want to establish the L2 connection on device-up. But in this patch, you didn't handle NETDEV_UP. Maybe on device-up, we need to check if the carrier is up, and if it is, we do the same thing as we do on carrier-up. Are the device up/down status and the carrier up/down status independent of each other? If they are, on device-up or carrier-up, we only need to try establishing the L2 connection if we see both are up. No, they aren't independent. The carrier can only be up if the device / interface is UP. And as far as I can see a NETDEV_CHANGE event will also only be generated on interfaces that are UP. So you can be sure, that if there is a NETDEV_CHANGE event then the device is UP. I removed the NETDEV_UP handling because I don't think it makes sense to implicitly try to establish layer2 (LAPB) if there is no carrier. And with the first X.25 connection request on that interface, it will be established anyway by x25_transmit_link(). I've tested it here with an HDLC WAN Adapter and it works as expected. These are also the ideal conditions for the already mentioned "on demand" scenario. The only necessary change would be to call x25_terminate_link() on an interface after clearing the last X.25 session. On NETDEV_GOING_DOWN, we can also check the carrier status first and if it is down, we don't need to call lapb_disconnect_request. This is not necessary because lapb_disconnect_request() checks the current state. And if the carrier is DOWN then the state should also be LAPB_STATE_0 and so lapb_disconnect_request() does nothing.
Re: [PATCH net-next v5] net/tun: Call netdev notifiers
On 2020-11-20 19:28, Jakub Kicinski wrote: On Wed, 18 Nov 2020 07:39:19 +0100 Martin Schiller wrote: Call netdev notifiers before and after changing the device type. Signed-off-by: Martin Schiller This is a fix, right? Can you give an example of something that goes wrong without this patch? This change is related to my latest patches to the X.25 Subsystem: https://patchwork.kernel.org/project/netdevbpf/list/?series=388087 I use a tun interface in a XoT (X.25 over TCP) application and use the TUNSETLINK ioctl to change the device type to ARPHRD_X25. As the default device type is ARPHRD_NONE the initial NETDEV_REGISTER event won't be catched by the X.25 Stack. Therefore I have to use the NETDEV_POST_TYPE_CHANGE to make sure that the corresponding neighbour structure is created. I could imagine that other protocols have similar requirements. Whether this is a fix or a functional extension is hard to say. Some time ago there was also a corresponding patch for the WAN/HDLC subsystem: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=2f8364a291e8
[PATCH net-next v4 5/5] net/x25: remove x25_kill_by_device()
Remove obsolete function x25_kill_by_device(). It's not used any more.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c | 16
1 file changed, 16 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 313a6222ded9..1432a05805ab 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -199,22 +199,6 @@ static void x25_remove_socket(struct sock *sk)
write_unlock_bh(_list_lock);
}
-/*
- * Kill all bound sockets on a dropped device.
- */
-static void x25_kill_by_device(struct net_device *dev)
-{
- struct sock *s;
-
- write_lock_bh(_list_lock);
-
- sk_for_each(s, _list)
- if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
- x25_disconnect(s, ENETUNREACH, 0, 0);
-
- write_unlock_bh(_list_lock);
-}
-
/*
* Handle device status changes.
*/
--
2.20.1
[PATCH net-next v4 4/5] net/x25: fix restart request/confirm handling
We have to take the actual link state into account to handle
restart requests/confirms well.
Signed-off-by: Martin Schiller
---
net/x25/x25_link.c | 41 +
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index 11e868aa625d..f92073f3cb11 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -74,16 +74,43 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh
*nb,
switch (frametype) {
case X25_RESTART_REQUEST:
- confirm = !x25_t20timer_pending(nb);
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
- if (confirm)
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ confirm = !x25_t20timer_pending(nb);
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ if (confirm)
+ x25_transmit_restart_confirmation(nb);
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
x25_transmit_restart_confirmation(nb);
+ break;
+ }
break;
case X25_RESTART_CONFIRMATION:
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ if (x25_t20timer_pending(nb)) {
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ } else {
+ x25_transmit_restart_request(nb);
+ x25_start_t20timer(nb);
+ }
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
+ x25_transmit_restart_request(nb);
+ nb->state = X25_LINK_STATE_2;
+ x25_start_t20timer(nb);
+ break;
+ }
break;
case X25_DIAGNOSTIC:
@@ -214,8 +241,6 @@ void x25_link_established(struct x25_neigh *nb)
{
switch (nb->state) {
case X25_LINK_STATE_0:
- nb->state = X25_LINK_STATE_2;
- break;
case X25_LINK_STATE_1:
x25_transmit_restart_request(nb);
nb->state = X25_LINK_STATE_2;
--
2.20.1
[PATCH net-next v4 2/5] net/lapb: support netdev events
This patch allows layer2 (LAPB) to react to netdev events itself and
avoids the detour via layer3 (X.25).
1. Call lapb_disconnect_request() on NETDEV_GOING_DOWN events.
2. When a NETDEV_DOWN event occur, clear all queues, enter state
LAPB_STATE_0 and stop all timers.
3. The NETDEV_CHANGE event makes it possible to handle carrier loss and
detection.
In case of Carrier Loss, clear all queues, enter state LAPB_STATE_0
and stop all timers.
In case of Carrier Detection, we start timer t1 on a DCE interface,
and on a DTE interface we change to state LAPB_STATE_1 and start
sending SABM(E).
Signed-off-by: Martin Schiller
---
net/lapb/lapb_iface.c | 72 +++
1 file changed, 72 insertions(+)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 3c03f6512c5f..52d59984fbe6 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -418,14 +418,86 @@ int lapb_data_transmit(struct lapb_cb *lapb, struct
sk_buff *skb)
return used;
}
+/* Handle device status changes. */
+static int lapb_device_event(struct notifier_block *this, unsigned long event,
+void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+ struct lapb_cb *lapb;
+
+ if (!net_eq(dev_net(dev), _net))
+ return NOTIFY_DONE;
+
+ if (dev->type == ARPHRD_X25) {
+ switch (event) {
+ case NETDEV_GOING_DOWN:
+ lapb_disconnect_request(dev);
+ break;
+ case NETDEV_DOWN:
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
+
+ lapb_dbg(0, "(%p) Interface down: %s\n", dev,
+dev->name);
+ lapb_dbg(0, "(%p) S%d -> S0\n", dev,
+lapb->state);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2timer(lapb);
+ break;
+ case NETDEV_CHANGE:
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
+
+ if (!netif_carrier_ok(dev)) {
+ lapb_dbg(0, "(%p) Carrier lost: %s\n", dev,
+dev->name);
+ lapb_dbg(0, "(%p) S%d -> S0\n", dev,
+lapb->state);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2timer(lapb);
+ } else {
+ lapb_dbg(0, "(%p): Carrier detected: %s\n",
+dev, dev->name);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ if (lapb->state == LAPB_STATE_0) {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
+ }
+ }
+ break;
+ }
+ }
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block lapb_dev_notifier = {
+ .notifier_call = lapb_device_event,
+};
+
static int __init lapb_init(void)
{
+ register_netdevice_notifier(_dev_notifier);
+
return 0;
}
static void __exit lapb_exit(void)
{
WARN_ON(!list_empty(_list));
+
+ unregister_netdevice_notifier(_dev_notifier);
}
MODULE_AUTHOR("Jonathan Naylor ");
--
2.20.1
[PATCH net-next v4 3/5] net/lapb: fix t1 timer handling for LAPB_STATE_0
1. DTE interface changes immediately to LAPB_STATE_1 and start sending
SABM(E).
2. DCE interface sends N2-times DM and changes to LAPB_STATE_1
afterwards if there is no response in the meantime.
Signed-off-by: Martin Schiller
---
net/lapb/lapb_timer.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 8f5b17001a07..baa247fe4ed0 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -85,11 +85,18 @@ static void lapb_t1timer_expiry(struct timer_list *t)
switch (lapb->state) {
/*
-* If we are a DCE, keep going DM .. DM .. DM
+* If we are a DCE, send DM up to N2 times, then switch to
+* STATE_1 and send SABM(E).
*/
case LAPB_STATE_0:
- if (lapb->mode & LAPB_DCE)
+ if (lapb->mode & LAPB_DCE &&
+ lapb->n2count != lapb->n2) {
+ lapb->n2count++;
lapb_send_control(lapb, LAPB_DM, LAPB_POLLOFF,
LAPB_RESPONSE);
+ } else {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
break;
/*
--
2.20.1
[PATCH net-next v4 1/5] net/x25: handle additional netdev events
1. Add / remove x25_link_device by NETDEV_REGISTER/UNREGISTER and also
by NETDEV_POST_TYPE_CHANGE/NETDEV_PRE_TYPE_CHANGE.
This change is needed so that the x25_neigh struct for an interface
is already created when it shows up and is kept independently if the
interface goes UP or DOWN.
This is used in an upcomming commit, where x25 params of an neighbour
will get configurable through ioctls.
2. NETDEV_CHANGE event makes it possible to handle carrier loss and
detection. If carrier is lost, clean up everything related to this
neighbour by calling x25_link_terminated().
3. Also call x25_link_terminated() for NETDEV_DOWN events and remove the
call to x25_clear_forward_by_dev() in x25_route_device_down(), as
this is already called by x25_kill_by_neigh() which gets called by
x25_link_terminated().
4. Do nothing for NETDEV_UP and NETDEV_GOING_DOWN events, as these will
be handled in layer 2 (LAPB) and layer3 (X.25) will be informed by
layer2 when layer2 link is established and layer3 link should be
initiated.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c| 22 --
net/x25/x25_link.c | 6 +++---
net/x25/x25_route.c | 3 ---
3 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 046d3fee66a9..313a6222ded9 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -233,21 +233,31 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
#endif
) {
switch (event) {
- case NETDEV_UP:
+ case NETDEV_REGISTER:
+ case NETDEV_POST_TYPE_CHANGE:
x25_link_device_up(dev);
break;
- case NETDEV_GOING_DOWN:
+ case NETDEV_DOWN:
nb = x25_get_neigh(dev);
if (nb) {
- x25_terminate_link(nb);
+ x25_link_terminated(nb);
x25_neigh_put(nb);
}
- break;
- case NETDEV_DOWN:
- x25_kill_by_device(dev);
x25_route_device_down(dev);
+ break;
+ case NETDEV_PRE_TYPE_CHANGE:
+ case NETDEV_UNREGISTER:
x25_link_device_down(dev);
break;
+ case NETDEV_CHANGE:
+ if (!netif_carrier_ok(dev)) {
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_link_terminated(nb);
+ x25_neigh_put(nb);
+ }
+ }
+ break;
}
}
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index fdae054b7dc1..11e868aa625d 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -232,6 +232,9 @@ void x25_link_established(struct x25_neigh *nb)
void x25_link_terminated(struct x25_neigh *nb)
{
nb->state = X25_LINK_STATE_0;
+ skb_queue_purge(>queue);
+ x25_stop_t20timer(nb);
+
/* Out of order: clear existing virtual calls (X.25 03/93 4.6.3) */
x25_kill_by_neigh(nb);
}
@@ -277,9 +280,6 @@ void x25_link_device_up(struct net_device *dev)
*/
static void __x25_remove_neigh(struct x25_neigh *nb)
{
- skb_queue_purge(>queue);
- x25_stop_t20timer(nb);
-
if (nb->node.next) {
list_del(>node);
x25_neigh_put(nb);
diff --git a/net/x25/x25_route.c b/net/x25/x25_route.c
index 00e46c9a5280..ec2a39e9b3e6 100644
--- a/net/x25/x25_route.c
+++ b/net/x25/x25_route.c
@@ -115,9 +115,6 @@ void x25_route_device_down(struct net_device *dev)
__x25_remove_route(rt);
}
write_unlock_bh(_route_list_lock);
-
- /* Remove any related forwarding */
- x25_clear_forward_by_dev(dev);
}
/*
--
2.20.1
[PATCH net-next v4 0/5] net/x25: netdev event handling
--- Changes to v3: o another complete rework of the patch-set to split event handling for layer2 (LAPB) and layer3 (X.25) Changes to v2: o restructure complete patch-set o keep netdev event handling in layer3 (X.25) o add patch to fix lapb_connect_request() for DCE o add patch to handle carrier loss correctly in lapb o drop patch for x25_neighbour param handling this may need fixes/cleanup and will be resubmitted later. Changes to v1: o fix 'subject_prefix' and 'checkpatch' warnings --- Martin Schiller (5): net/x25: handle additional netdev events net/lapb: support netdev events net/lapb: fix t1 timer handling for LAPB_STATE_0 net/x25: fix restart request/confirm handling net/x25: remove x25_kill_by_device() net/lapb/lapb_iface.c | 72 +++ net/lapb/lapb_timer.c | 11 +-- net/x25/af_x25.c | 38 ++- net/x25/x25_link.c| 47 +--- net/x25/x25_route.c | 3 -- 5 files changed, 133 insertions(+), 38 deletions(-) -- 2.20.1
Re: [PATCH net-next v3 0/6] net/x25: netdev event handling
On 2020-11-18 15:47, Xie He wrote: On Wed, Nov 18, 2020 at 5:59 AM Martin Schiller wrote: --- Changes to v2: o restructure complete patch-set o keep netdev event handling in layer3 (X.25) But... Won't it be better to handle L2 connections in L2 code? For example, if we are running X.25 over XOT, we can decide in the XOT layer whether and when we reconnect in case the TCP connection is dropped. We can decide how long we wait for responses before we consider the TCP connection to be dropped. If we still want "on-demand" connections in certain L2's, we can also implement it in that L2 without the need to change L3. Every L2 has its own characteristics. It might be better to let different L2's handle their connections in their own way. This gives L2 the flexibility to handle their connections according to their actual link characteristics. Letting L3 handle L2 connections also makes L2 code too related to / coupled with L3 code, which makes the logic complex. OK, I will give it a try. But we need to keep the possibility to initiate and terminate the L2 connection from L3. In the on demand scenario i mentioned, the L2 should be connected when the first L3 logical channel goes up and needs to be disconnected, when the last L3 logical channel on an interface is cleared. o add patch to fix lapb_connect_request() for DCE o add patch to handle carrier loss correctly in lapb o drop patch for x25_neighbour param handling this may need fixes/cleanup and will be resubmitted later. Changes to v1: o fix 'subject_prefix' and 'checkpatch' warnings --- Martin Schiller (6): net/x25: handle additional netdev events net/lapb: fix lapb_connect_request() for DCE net/lapb: handle carrier loss correctly net/lapb: fix t1 timer handling for DCE net/x25: fix restart request/confirm handling net/x25: remove x25_kill_by_device()
[PATCH net-next v3 5/6] net/x25: fix restart request/confirm handling
We have to take the actual link state into account to handle
restart requests/confirms well.
Also, the T20 timer needs to be stopped, if the link is terminated.
Signed-off-by: Martin Schiller
---
net/x25/x25_link.c | 45 +
1 file changed, 37 insertions(+), 8 deletions(-)
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index 92828a8a4ada..40ffc10f7a96 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -74,16 +74,43 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh
*nb,
switch (frametype) {
case X25_RESTART_REQUEST:
- confirm = !x25_t20timer_pending(nb);
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
- if (confirm)
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ confirm = !x25_t20timer_pending(nb);
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ if (confirm)
+ x25_transmit_restart_confirmation(nb);
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
x25_transmit_restart_confirmation(nb);
+ break;
+ }
break;
case X25_RESTART_CONFIRMATION:
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
+ switch (nb->state) {
+ case X25_LINK_STATE_2:
+ if (x25_t20timer_pending(nb)) {
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
+ } else {
+ x25_transmit_restart_request(nb);
+ x25_start_t20timer(nb);
+ }
+ break;
+ case X25_LINK_STATE_3:
+ /* clear existing virtual calls */
+ x25_kill_by_neigh(nb);
+
+ x25_transmit_restart_request(nb);
+ nb->state = X25_LINK_STATE_2;
+ x25_start_t20timer(nb);
+ break;
+ }
break;
case X25_DIAGNOSTIC:
@@ -214,8 +241,6 @@ void x25_link_established(struct x25_neigh *nb)
{
switch (nb->state) {
case X25_LINK_STATE_0:
- nb->state = X25_LINK_STATE_2;
- break;
case X25_LINK_STATE_1:
x25_transmit_restart_request(nb);
nb->state = X25_LINK_STATE_2;
@@ -232,6 +257,10 @@ void x25_link_established(struct x25_neigh *nb)
void x25_link_terminated(struct x25_neigh *nb)
{
nb->state = X25_LINK_STATE_0;
+
+ if (x25_t20timer_pending(nb))
+ x25_stop_t20timer(nb);
+
/* Out of order: clear existing virtual calls (X.25 03/93 4.6.3) */
x25_kill_by_neigh(nb);
}
--
2.20.1
[PATCH net-next v3 6/6] net/x25: remove x25_kill_by_device()
Remove unnecessary function x25_kill_by_device().
Replace the call to x25_kill_by_device() by x25_kill_by_neigh().
Therefore, also remove the call to x25_clear_forward_by_dev() in
x25_route_device_down(), as this is already called by
x25_kill_by_neigh().
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c| 22 +-
net/x25/x25_route.c | 3 ---
2 files changed, 5 insertions(+), 20 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 02f56386e05b..ec90956f38d4 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -199,22 +199,6 @@ static void x25_remove_socket(struct sock *sk)
write_unlock_bh(_list_lock);
}
-/*
- * Kill all bound sockets on a dropped device.
- */
-static void x25_kill_by_device(struct net_device *dev)
-{
- struct sock *s;
-
- write_lock_bh(_list_lock);
-
- sk_for_each(s, _list)
- if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
- x25_disconnect(s, ENETUNREACH, 0, 0);
-
- write_unlock_bh(_list_lock);
-}
-
/*
* Handle device status changes.
*/
@@ -260,7 +244,11 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
case NETDEV_DOWN:
pr_debug("X.25: got event NETDEV_DOWN for device: %s\n",
dev->name);
- x25_kill_by_device(dev);
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_kill_by_neigh(nb);
+ x25_neigh_put(nb);
+ }
x25_route_device_down(dev);
x25_link_device_down(dev);
break;
diff --git a/net/x25/x25_route.c b/net/x25/x25_route.c
index 00e46c9a5280..ec2a39e9b3e6 100644
--- a/net/x25/x25_route.c
+++ b/net/x25/x25_route.c
@@ -115,9 +115,6 @@ void x25_route_device_down(struct net_device *dev)
__x25_remove_route(rt);
}
write_unlock_bh(_route_list_lock);
-
- /* Remove any related forwarding */
- x25_clear_forward_by_dev(dev);
}
/*
--
2.20.1
[PATCH net-next v3 4/6] net/lapb: fix t1 timer handling for DCE
fix t1 timer handling for DCE in LAPB_STATE_0:
o DTE interface changes immediately to LAPB_STATE_1 and start sending
SABM(E).
o DCE interface sends N2-times DM and changes to LAPB_STATE_1
afterwards if there is no response in the meantime.
Signed-off-by: Martin Schiller
---
net/lapb/lapb_timer.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 8f5b17001a07..baa247fe4ed0 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -85,11 +85,18 @@ static void lapb_t1timer_expiry(struct timer_list *t)
switch (lapb->state) {
/*
-* If we are a DCE, keep going DM .. DM .. DM
+* If we are a DCE, send DM up to N2 times, then switch to
+* STATE_1 and send SABM(E).
*/
case LAPB_STATE_0:
- if (lapb->mode & LAPB_DCE)
+ if (lapb->mode & LAPB_DCE &&
+ lapb->n2count != lapb->n2) {
+ lapb->n2count++;
lapb_send_control(lapb, LAPB_DM, LAPB_POLLOFF,
LAPB_RESPONSE);
+ } else {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
break;
/*
--
2.20.1
[PATCH net-next v3 3/6] net/lapb: handle carrier loss correctly
In case of carrier loss, clear all queues, enter state LABB_STATE_0 and
stop all timers.
By setting rc = LAPB_NOTCONNECTED, the upper layer is informed about the
disconnect.
Signed-off-by: Martin Schiller
---
net/lapb/lapb_iface.c | 12
1 file changed, 12 insertions(+)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 8dd7c420ae93..017bc169c334 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -303,6 +303,18 @@ int lapb_disconnect_request(struct net_device *dev)
if (!lapb)
goto out;
+ if (!netif_carrier_ok(dev)) {
+ lapb_dbg(0, "(%p) Carrier lost!\n", lapb->dev);
+ lapb_dbg(0, "(%p) S%d -> S0\n", lapb->dev, lapb->state);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2timer(lapb);
+ rc = LAPB_NOTCONNECTED;
+ goto out_put;
+ }
+
switch (lapb->state) {
case LAPB_STATE_0:
rc = LAPB_NOTCONNECTED;
--
2.20.1
[PATCH net-next v3 2/6] net/lapb: fix lapb_connect_request() for DCE
For a DTE interface we should change to state LAPB_STATE_1 and start
sending SABM(E). But for DCE interfaces, we simply should start the
timer t1.
Signed-off-by: Martin Schiller
---
net/lapb/lapb_iface.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 3c03f6512c5f..8dd7c420ae93 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -278,10 +278,14 @@ int lapb_connect_request(struct net_device *dev)
if (lapb->state == LAPB_STATE_3 || lapb->state == LAPB_STATE_4)
goto out_put;
- lapb_establish_data_link(lapb);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ lapb_establish_data_link(lapb);
- lapb_dbg(0, "(%p) S0 -> S1\n", lapb->dev);
- lapb->state = LAPB_STATE_1;
+ lapb_dbg(0, "(%p) S0 -> S1\n", lapb->dev);
+ lapb->state = LAPB_STATE_1;
+ }
rc = LAPB_OK;
out_put:
--
2.20.1
[PATCH net-next v3 1/6] net/x25: handle additional netdev events
Add / remove x25_link_device by NETDEV_REGISTER/UNREGISTER and also by
NETDEV_POST_TYPE_CHANGE/NETDEV_PRE_TYPE_CHANGE.
This change is needed so that the x25_neigh struct for an interface is
already created when it shows up and is kept independently if the
interface goes UP or DOWN.
This is used in an upcomming commit, where x25 params of an neighbour
will get configurable through ioctls.
Additionally the NETDEV_CHANGE event makes it possible to handle carrier
loss and detection.
Signed-off-by: Martin Schiller
---
include/net/x25.h | 2 ++
net/x25/af_x25.c | 44
net/x25/x25_link.c | 45 +++--
3 files changed, 85 insertions(+), 6 deletions(-)
diff --git a/include/net/x25.h b/include/net/x25.h
index d7d6c2b4ffa7..4c1502e8b2b2 100644
--- a/include/net/x25.h
+++ b/include/net/x25.h
@@ -231,6 +231,8 @@ int x25_backlog_rcv(struct sock *, struct sk_buff *);
/* x25_link.c */
void x25_link_control(struct sk_buff *, struct x25_neigh *, unsigned short);
+void x25_link_device_add(struct net_device *dev);
+void x25_link_device_remove(struct net_device *dev);
void x25_link_device_up(struct net_device *);
void x25_link_device_down(struct net_device *);
void x25_link_established(struct x25_neigh *);
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 046d3fee66a9..02f56386e05b 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -233,10 +233,24 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
#endif
) {
switch (event) {
+ case NETDEV_REGISTER:
+ pr_debug("X.25: got event NETDEV_REGISTER for device:
%s\n",
+dev->name);
+ x25_link_device_add(dev);
+ break;
+ case NETDEV_POST_TYPE_CHANGE:
+ pr_debug("X.25: got event NETDEV_POST_TYPE_CHANGE for
device: %s\n",
+dev->name);
+ x25_link_device_add(dev);
+ break;
case NETDEV_UP:
+ pr_debug("X.25: got event NETDEV_UP for device: %s\n",
+dev->name);
x25_link_device_up(dev);
break;
case NETDEV_GOING_DOWN:
+ pr_debug("X.25: got event NETDEV_GOING_DOWN for device:
%s\n",
+dev->name);
nb = x25_get_neigh(dev);
if (nb) {
x25_terminate_link(nb);
@@ -244,10 +258,40 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
}
break;
case NETDEV_DOWN:
+ pr_debug("X.25: got event NETDEV_DOWN for device: %s\n",
+dev->name);
x25_kill_by_device(dev);
x25_route_device_down(dev);
x25_link_device_down(dev);
break;
+ case NETDEV_PRE_TYPE_CHANGE:
+ pr_debug("X.25: got event NETDEV_PRE_TYPE_CHANGE for
device: %s\n",
+dev->name);
+ x25_link_device_remove(dev);
+ break;
+ case NETDEV_UNREGISTER:
+ pr_debug("X.25: got event NETDEV_UNREGISTER for device:
%s\n",
+dev->name);
+ x25_link_device_remove(dev);
+ break;
+ case NETDEV_CHANGE:
+ pr_debug("X.25: got event NETDEV_CHANGE for device:
%s\n",
+dev->name);
+ nb = x25_get_neigh(dev);
+ if (!nb)
+ break;
+
+ if (!netif_carrier_ok(dev)) {
+ pr_debug("X.25: Carrier lost -> set link state
down: %s\n",
+dev->name);
+ x25_terminate_link(nb);
+ } else {
+ pr_debug("X.25: Carrier detected: %s\n",
+dev->name);
+ x25_establish_link(nb);
+ }
+ x25_neigh_put(nb);
+ break;
}
}
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index fdae054b7dc1..92828a8a4ada 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -239,9 +239,17 @@ void x25_link_terminated(struct x25_neigh *nb)
/*
* Add a new device.
*/
-void x25_link_device_up(struct
[PATCH net-next v3 0/6] net/x25: netdev event handling
--- Changes to v2: o restructure complete patch-set o keep netdev event handling in layer3 (X.25) o add patch to fix lapb_connect_request() for DCE o add patch to handle carrier loss correctly in lapb o drop patch for x25_neighbour param handling this may need fixes/cleanup and will be resubmitted later. Changes to v1: o fix 'subject_prefix' and 'checkpatch' warnings --- Martin Schiller (6): net/x25: handle additional netdev events net/lapb: fix lapb_connect_request() for DCE net/lapb: handle carrier loss correctly net/lapb: fix t1 timer handling for DCE net/x25: fix restart request/confirm handling net/x25: remove x25_kill_by_device() include/net/x25.h | 2 + net/lapb/lapb_iface.c | 22 +-- net/lapb/lapb_timer.c | 11 +- net/x25/af_x25.c | 66 +++ net/x25/x25_link.c| 90 --- net/x25/x25_route.c | 3 -- 6 files changed, 155 insertions(+), 39 deletions(-) -- 2.20.1
Re: [PATCH net-next v2 5/6] net/lapb: support netdev events
On 2020-11-18 14:46, Xie He wrote: On Wed, Nov 18, 2020 at 5:03 AM Xie He wrote: On Wed, Nov 18, 2020 at 12:49 AM Martin Schiller wrote: > > I also have a patch here that implements an "on demand" link feature, > which we used for ISDN dialing connections. > As ISDN is de facto dead, this is not relevant anymore. But if we want > such kind of feature, I think we need to stay with the method to control > L2 link state from L3. I see. Hmm... I guess for ISDN, the current code (before this patch series) is the best. We only establish the connection when L3 has packets to send. Can we do this? We let L2 handle all device-up / device-down / carrier-up / carrier-down events. And when L3 has some packets to send but it still finds the L2 link is not up, it will then instruct L2 to connect. This way we may be able to both keep the logic simple and still keep L3 compatible with ISDN. Another solution might be letting ISDN automatically connect when it receives the first packet from L3. This way we can still free L3 from all handlings of L2 connections. ISDN is not important now. Also the I4L subsystem has been removed. I have now completely reworked the patch-set and it is now much tidier. For now I left the event handling completely in X.25 (L3). I will now send the whole thing as v3 and we can discuss it further.
Re: [PATCH net-next v2 5/6] net/lapb: support netdev events
On 2020-11-17 19:28, Xie He wrote: On Tue, Nov 17, 2020 at 5:26 AM Martin Schiller wrote: On 2020-11-17 12:32, Xie He wrote: > > I think for a DCE, it doesn't need to initiate the L2 > connection on device-up. It just needs to wait for a connection to > come. But L3 seems to be still instructing it to initiate the L2 > connection. This seems to be a problem. The "ITU-T Recommendation X.25 (10/96) aka "Blue Book" [1] says under point 2.4.4.1: "Either the DTE or the DCE may initiate data link set-up." Experience shows that there are also DTEs that do not establish a connection themselves. That is also the reason why I've added this patch: https://patchwork.kernel.org/project/netdevbpf/patch/20201116135522.21791-7...@dev.tdt.de/ Yes, I understand that either the DTE or the DCE *may* initiate the L2 connection. This is also the way the current code (before this patch set) works. But I see both the DTE and the DCE will try to initiate the L2 connection after device-up, because according to your 1st patch, L3 will always instruct L2 to do this on device-up. However, looking at your 6th patch (in the link you gave), you seem to want the DCE to wait for a while before initiating the connection by itself. So I'm unclear which way you want to go. Making DCE initiate the L2 connection on device-up, or making DCE wait for a while before initiating the L2 connection? I think the second way is more reasonable. Ah, ok. Now I see what you mean. Yes, we should check the lapb->mode in lapb_connect_request(). > It feels unclean to me that the L2 connection will sometimes be > initiated by L3 and sometimes by L2 itself. Can we make L2 connections > always be initiated by L2 itself? If L3 needs to do something after L2 > links up, L2 will notify it anyway. My original goal was to change as little as possible of the original code. And in the original code the NETDEV_UP/NETDEV_DOWN events were/are handled in L3. But it is of course conceivable to shift this to L2. I suggested moving L2 connection handling to L2 because I think having both L2 and L3 to handle this makes the logic of the code too complex. For example, after a device-up event, L3 will instruct L2 to initiate the L2 connection. But L2 code has its own way of initiating connections. For a DCE, L2 wants to wait a while before initiating the connection. So currently L2 and L3 want to do things differently and they are doing things at the same time. But you have to keep in mind that the X.25 L3 stack can also be used with tap interfaces (e.g. for XOT), where you do not have a L2 at all. Can we treat XOT the same as LAPB? I think XOT should be considered a L2 in this case. So maybe XOT can establish the TCP connection by itself without being instructed by L3. I'm not sure if this is feasible in practice but it'd be good if it is. This also simplifies the L3 code. I also have a patch here that implements an "on demand" link feature, which we used for ISDN dialing connections. As ISDN is de facto dead, this is not relevant anymore. But if we want such kind of feature, I think we need to stay with the method to control L2 link state from L3.
Re: [PATCH net-next v2 3/6] net/x25: replace x25_kill_by_device with x25_kill_by_neigh
On 2020-11-17 20:50, Xie He wrote:
On Mon, Nov 16, 2020 at 6:00 AM Martin Schiller wrote:
Remove unnecessary function x25_kill_by_device.
-/*
- * Kill all bound sockets on a dropped device.
- */
-static void x25_kill_by_device(struct net_device *dev)
-{
- struct sock *s;
-
- write_lock_bh(_list_lock);
-
- sk_for_each(s, _list)
- if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev
== dev)
- x25_disconnect(s, ENETUNREACH, 0, 0);
-
- write_unlock_bh(_list_lock);
-}
-
/*
* Handle device status changes.
*/
@@ -273,7 +257,11 @@ static int x25_device_event(struct notifier_block
*this, unsigned long event,
case NETDEV_DOWN:
pr_debug("X.25: got event NETDEV_DOWN for
device: %s\n",
dev->name);
- x25_kill_by_device(dev);
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_kill_by_neigh(nb);
+ x25_neigh_put(nb);
+ }
x25_route_device_down(dev);
x25_link_device_down(dev);
break;
This patch might not be entirely necessary. x25_kill_by_neigh and
x25_kill_by_device are just two helper functions. One function takes
nb as the argument and the other one takes dev as the argument. But
they do almost the same things. It doesn't harm to keep both. In C++
we often have different functions with the same name doing almost the
same things.
Well I don't like to have 2 functions doing the same thing.
But after another look at this code, I've found that I also need to
remove the call to x25_clear_forward_by_dev() in the function
x25_route_device_down(). Otherwise, it will be called twice.
The original code also seems to be a little more efficient than the new
code.
The only difference would be the x25_get_neigh() and x25_neigh_put()
calls. That shouldn't cost to much.
[PATCH net-next v5] net/tun: Call netdev notifiers
Call netdev notifiers before and after changing the device type.
Signed-off-by: Martin Schiller
---
Changes to v4:
* Fix copy'n'paste error
Changes to v3:
* Handle return value of call_netdevice_notifiers()
Changes to v2:
* Use subject_prefix 'net-next' to fix 'fixes_present' issue
Changes to v1:
* Fix 'subject_prefix' and 'checkpatch' warnings
---
drivers/net/tun.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 3d45d56172cb..7c62d82c57db 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3071,10 +3071,19 @@ static long __tun_chr_ioctl(struct file *file, unsigned
int cmd,
"Linktype set failed because interface is
up\n");
ret = -EBUSY;
} else {
+ ret = call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE,
+ tun->dev);
+ ret = notifier_to_errno(ret);
+ if (ret) {
+ netif_info(tun, drv, tun->dev,
+ "Refused to change device type\n");
+ break;
+ }
tun->dev->type = (int) arg;
netif_info(tun, drv, tun->dev, "linktype set to %d\n",
tun->dev->type);
- ret = 0;
+ call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
+tun->dev);
}
break;
--
2.20.1
[PATCH net-next v4] net/tun: Call netdev notifiers
Call netdev notifiers before and after changing the device type.
Signed-off-by: Martin Schiller
---
Changes to v3:
* Handle return value of call_netdevice_notifiers()
Changes to v2:
* Use subject_prefix 'net-next' to fix 'fixes_present' issue
Changes to v1:
* Fix 'subject_prefix' and 'checkpatch' warnings
---
drivers/net/tun.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 3d45d56172cb..704306695b88 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3071,10 +3071,19 @@ static long __tun_chr_ioctl(struct file *file, unsigned
int cmd,
"Linktype set failed because interface is
up\n");
ret = -EBUSY;
} else {
+ ret = call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE,
+ tun->dev);
+ ret = notifier_to_errno(err);
+ if (ret) {
+ netif_info(tun, drv, tun->dev,
+ "Refused to change device type\n");
+ break;
+ }
tun->dev->type = (int) arg;
netif_info(tun, drv, tun->dev, "linktype set to %d\n",
tun->dev->type);
- ret = 0;
+ call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
+tun->dev);
}
break;
--
2.20.1
Re: [PATCH net-next v3] net/tun: Call netdev notifiers
On 2020-11-18 01:32, Jakub Kicinski wrote:
On Mon, 16 Nov 2020 11:41:21 +0100 Martin Schiller wrote:
Call netdev notifiers before and after changing the device type.
Signed-off-by: Martin Schiller
---
Change from v2:
use subject_prefix 'net-next' to fix 'fixes_present' issue
Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings
---
drivers/net/tun.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 3d45d56172cb..2d9a00f41023 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3071,9 +3071,13 @@ static long __tun_chr_ioctl(struct file *file,
unsigned int cmd,
"Linktype set failed because interface is
up\n");
ret = -EBUSY;
} else {
+ call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE,
+tun->dev);
This call may return an error (which has to be unpacked with
notifier_to_errno()).
OK, I'll fix that and send a v3 patch.
tun->dev->type = (int) arg;
netif_info(tun, drv, tun->dev, "linktype set to %d\n",
tun->dev->type);
+ call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
+tun->dev);
ret = 0;
}
break;
Re: [PATCH net-next v2 5/6] net/lapb: support netdev events
On 2020-11-17 12:32, Xie He wrote: On Tue, Nov 17, 2020 at 1:53 AM Martin Schiller wrote: On 2020-11-16 21:16, Xie He wrote: > Do you mean we will now automatically establish LAPB connections > without upper layers instructing us to do so? Yes, as soon as the physical link is established, the L2 and also the L3 layer (restart handshake) is established. I see. Looking at your code in Patch 1 and this patch, I see after the device goes up, L3 code will instruct L2 to establish the connection, and before the device goes down, L3 will instruct L2 to terminate the connection. But if there is a carrier up/down event, L2 will automatically handle this without being instructed by L3, and it will establish the connection automatically when the carrier goes up. L2 will notify L3 on any L2 link status change. Is this right? Yes, this is right. I think for a DCE, it doesn't need to initiate the L2 connection on device-up. It just needs to wait for a connection to come. But L3 seems to be still instructing it to initiate the L2 connection. This seems to be a problem. The "ITU-T Recommendation X.25 (10/96) aka "Blue Book" [1] says under point 2.4.4.1: "Either the DTE or the DCE may initiate data link set-up." Experience shows that there are also DTEs that do not establish a connection themselves. That is also the reason why I've added this patch: https://patchwork.kernel.org/project/netdevbpf/patch/20201116135522.21791-7...@dev.tdt.de/ It feels unclean to me that the L2 connection will sometimes be initiated by L3 and sometimes by L2 itself. Can we make L2 connections always be initiated by L2 itself? If L3 needs to do something after L2 links up, L2 will notify it anyway. My original goal was to change as little as possible of the original code. And in the original code the NETDEV_UP/NETDEV_DOWN events were/are handled in L3. But it is of course conceivable to shift this to L2. But you have to keep in mind that the X.25 L3 stack can also be used with tap interfaces (e.g. for XOT), where you do not have a L2 at all. In this context I also noticed that I should add another patch to this patch-set to correct the restart handling. Do you mean you will add code to let L3 restart any L3 connections previously abnormally terminated after L2 link up? No, I mean the handling of Restart Request and Restart Confirm is buggy and needs to be fixed also. As already mentioned I have a stack of fixes and extensions lying around that I would like to get upstream. Please do so! Thanks! I previously found a locking problem in X.25 code and tried to address it in: https://patchwork.kernel.org/project/netdevbpf/patch/20201114103625.323919-1-xie.he.0...@gmail.com/ But later I found I needed to fix more code than I previously thought. Do you already have a fix for this problem? No, sorry. [1] https://www.itu.int/rec/T-REC-X.25-199610-I/
Re: [PATCH net-next v2 4/6] net/x25: support NETDEV_CHANGE notifier
On 2020-11-17 12:41, Xie He wrote:
On Mon, Nov 16, 2020 at 6:00 AM Martin Schiller wrote:
This makes it possible to handle carrier lost and detection.
In case of carrier lost, we shutdown layer 3 and flush all sessions.
@@ -275,6 +275,19 @@ static int x25_device_event(struct notifier_block
*this, unsigned long event,
dev->name);
x25_link_device_remove(dev);
break;
+ case NETDEV_CHANGE:
+ pr_debug("X.25: got event NETDEV_CHANGE for
device: %s\n",
+dev->name);
+ if (!netif_carrier_ok(dev)) {
+ pr_debug("X.25: Carrier lost -> set
link state down: %s\n",
+dev->name);
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_link_terminated(nb);
+ x25_neigh_put(nb);
+ }
+ }
+ break;
}
}
I think L2 will notify L3 if the L2 connection is terminated. Is this
patch necessary?
Hmm... well I guess you're right. Admittedly, these patches were made
about 7 - 8 years ago and I have to keep thinking about them.
But I can't think of any situation where this patch should be necessary
at the moment.
I will drop this patch from the patch-set.
Re: [PATCH net-next v2 5/6] net/lapb: support netdev events
On 2020-11-16 21:16, Xie He wrote: Do you mean we will now automatically establish LAPB connections without upper layers instructing us to do so? Yes, as soon as the physical link is established, the L2 and also the L3 layer (restart handshake) is established. In this context I also noticed that I should add another patch to this patch-set to correct the restart handling. As already mentioned I have a stack of fixes and extensions lying around that I would like to get upstream. If that is the case, is the one-byte header for instructing the LAPB layer to connect / disconnect no longer needed? The one-byte header is still needed to signal the status of the LAPB connection to the upper layer.
[PATCH net-next v2 5/6] net/lapb: support netdev events
This makes it possible to handle carrier loss and detection.
In case of Carrier Loss, layer 2 is terminated
In case of Carrier Detection, we start timer t1 on a DCE interface,
and on a DTE interface we change to state LAPB_STATE_1 and start
sending SABM(E).
Signed-off-by: Martin Schiller
---
Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings
---
net/lapb/lapb_iface.c | 83 +++
1 file changed, 83 insertions(+)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 3c03f6512c5f..63124cdf1926 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -418,14 +418,97 @@ int lapb_data_transmit(struct lapb_cb *lapb, struct
sk_buff *skb)
return used;
}
+/* Handle device status changes. */
+static int lapb_device_event(struct notifier_block *this, unsigned long event,
+void *ptr)
+{
+ struct net_device *dev = ptr;
+ struct lapb_cb *lapb;
+
+ if (!net_eq(dev_net(dev), _net))
+ return NOTIFY_DONE;
+
+ if (dev->type == ARPHRD_X25) {
+ switch (event) {
+ case NETDEV_REGISTER:
+ lapb_dbg(0, "(%p): got event NETDEV_REGISTER for
device: %s\n",
+dev, dev->name);
+ break;
+ case NETDEV_POST_TYPE_CHANGE:
+ lapb_dbg(0, "(%p): got event NETDEV_POST_TYPE_CHANGE
for device: %s\n",
+dev, dev->name);
+ break;
+ case NETDEV_UP:
+ lapb_dbg(0, "(%p): got event NETDEV_UP for device:
%s\n",
+dev, dev->name);
+ break;
+ case NETDEV_GOING_DOWN:
+ lapb_dbg(0, "(%p): got event NETDEV_GOING_DOWN for
device: %s\n",
+dev, dev->name);
+ break;
+ case NETDEV_DOWN:
+ lapb_dbg(0, "(%p): got event NETDEV_DOWN for device:
%s\n",
+dev, dev->name);
+ break;
+ case NETDEV_PRE_TYPE_CHANGE:
+ lapb_dbg(0, "(%p): got event NETDEV_PRE_TYPE_CHANGE for
device: %s\n",
+dev, dev->name);
+ break;
+ case NETDEV_UNREGISTER:
+ lapb_dbg(0, "(%p): got event NETDEV_UNREGISTER for
device: %s\n",
+dev, dev->name);
+ break;
+ case NETDEV_CHANGE:
+ lapb_dbg(0, "(%p): got event NETDEV_CHANGE for device:
%s\n",
+dev, dev->name);
+ lapb = lapb_devtostruct(dev);
+ if (!lapb)
+ break;
+
+ if (!netif_carrier_ok(dev)) {
+ lapb_dbg(0, "(%p): Carrier lost -> Entering
LAPB_STATE_0: %s\n",
+dev, dev->name);
+ lapb_disconnect_indication(lapb, LAPB_OK);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2timer(lapb);
+ } else {
+ lapb_dbg(0, "(%p): Carrier detected: %s\n",
+dev, dev->name);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ if (lapb->state == LAPB_STATE_0) {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
+ }
+ }
+ break;
+ }
+ }
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block lapb_dev_notifier = {
+ .notifier_call = lapb_device_event,
+};
+
static int __init lapb_init(void)
{
+ register_netdevice_notifier(_dev_notifier);
+
return 0;
}
static void __exit lapb_exit(void)
{
WARN_ON(!list_empty(_list));
+
+ unregister_netdevice_notifier(_dev_notifier);
}
MODULE_AUTHOR("Jonathan Naylor ");
--
2.20.1
[PATCH net-next v2 6/6] net/lapb: fix t1 timer handling
fix t1 timer handling in LAPB_STATE_0:
o DTE interface changes immediately to LAPB_STATE_1 and start sending
SABM(E).
o DCE interface sends N2-times DM and changes to LAPB_STATE_1
afterwards if there is no response in the meantime.
Signed-off-by: Martin Schiller
---
Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings
---
net/lapb/lapb_timer.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 8f5b17001a07..baa247fe4ed0 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -85,11 +85,18 @@ static void lapb_t1timer_expiry(struct timer_list *t)
switch (lapb->state) {
/*
-* If we are a DCE, keep going DM .. DM .. DM
+* If we are a DCE, send DM up to N2 times, then switch to
+* STATE_1 and send SABM(E).
*/
case LAPB_STATE_0:
- if (lapb->mode & LAPB_DCE)
+ if (lapb->mode & LAPB_DCE &&
+ lapb->n2count != lapb->n2) {
+ lapb->n2count++;
lapb_send_control(lapb, LAPB_DM, LAPB_POLLOFF,
LAPB_RESPONSE);
+ } else {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
break;
/*
--
2.20.1
[PATCH net-next v2 2/6] net/x25: make neighbour params configurable
Extended struct x25_neigh and x25_subscrip_struct to configure following
params through SIOCX25SSUBSCRIP:
o mode (DTE/DCE)
o number of channels
o facilities (packet size, window size)
o timer T20
Based on this configuration options the following changes/extensions
where made:
o DTE/DCE handling to select the next lc (DCE=from bottom / DTE=from
top)
o DTE/DCE handling to set correct clear/reset/restart cause
o take default facilities from neighbour settings
Signed-off-by: Martin Schiller
---
Change from v1:
o fix 'subject_prefix' and 'checkpatch' warnings
o fix incompatible assignment of 'struct compat_x25_facilities'
---
include/net/x25.h| 8 ++-
include/uapi/linux/x25.h | 56 ---
net/x25/af_x25.c | 145 ---
net/x25/x25_facilities.c | 6 +-
net/x25/x25_link.c | 97 ++
net/x25/x25_subr.c | 22 +-
6 files changed, 268 insertions(+), 66 deletions(-)
diff --git a/include/net/x25.h b/include/net/x25.h
index 4c1502e8b2b2..ec00f595fcc6 100644
--- a/include/net/x25.h
+++ b/include/net/x25.h
@@ -140,6 +140,9 @@ struct x25_neigh {
struct net_device *dev;
unsigned intstate;
unsigned intextended;
+ unsigned intdce;
+ unsigned intlc;
+ struct x25_facilities facilities;
struct sk_buff_head queue;
unsigned long t20;
struct timer_list t20timer;
@@ -164,6 +167,8 @@ struct x25_sock {
struct timer_list timer;
struct x25_causediagcausediag;
struct x25_facilities facilities;
+ /* set, if facilities changed by SIOCX25SFACILITIES */
+ unsigned intsocket_defined_facilities;
struct x25_dte_facilities dte_facilities;
struct x25_calluserdata calluserdata;
unsigned long vc_facil_mask; /* inc_call facilities mask */
@@ -215,7 +220,8 @@ int x25_create_facilities(unsigned char *, struct
x25_facilities *,
struct x25_dte_facilities *, unsigned long);
int x25_negotiate_facilities(struct sk_buff *, struct sock *,
struct x25_facilities *,
-struct x25_dte_facilities *);
+ struct x25_dte_facilities *,
+ struct x25_neigh *);
void x25_limit_facilities(struct x25_facilities *, struct x25_neigh *);
/* x25_forward.c */
diff --git a/include/uapi/linux/x25.h b/include/uapi/linux/x25.h
index 034b7dc5593a..094dc2cff37b 100644
--- a/include/uapi/linux/x25.h
+++ b/include/uapi/linux/x25.h
@@ -63,31 +63,6 @@ struct sockaddr_x25 {
struct x25_address sx25_addr; /* X.121 Address */
};
-/*
- * DTE/DCE subscription options.
- *
- * As this is missing lots of options, user should expect major
- * changes of this structure in 2.5.x which might break compatibilty.
- * The somewhat ugly dimension 200-sizeof() is needed to maintain
- * backward compatibility.
- */
-struct x25_subscrip_struct {
- char device[200-sizeof(unsigned long)];
- unsigned long global_facil_mask; /* 0 to disable negotiation */
- unsigned intextended;
-};
-
-/* values for above global_facil_mask */
-
-#defineX25_MASK_REVERSE0x01
-#defineX25_MASK_THROUGHPUT 0x02
-#defineX25_MASK_PACKET_SIZE0x04
-#defineX25_MASK_WINDOW_SIZE0x08
-
-#define X25_MASK_CALLING_AE 0x10
-#define X25_MASK_CALLED_AE 0x20
-
-
/*
* Routing table control structure.
*/
@@ -127,6 +102,37 @@ struct x25_dte_facilities {
__u8 called_ae[20];
};
+/*
+ * DTE/DCE subscription options.
+ *
+ * As this is missing lots of options, user should expect major
+ * changes of this structure in 2.5.x which might break compatibility.
+ * The somewhat ugly dimension 200-sizeof() is needed to maintain
+ * backward compatibility.
+ */
+struct x25_subscrip_struct {
+ char device[200 - ((2 * sizeof(unsigned long)) +
+ sizeof(struct x25_facilities) +
+ (2 * sizeof(unsigned int)))];
+ unsigned intdce;
+ unsigned intlc;
+ struct x25_facilities facilities;
+ unsigned long t20;
+ unsigned long global_facil_mask; /* 0 to disable
negotiation */
+ unsigned intextended;
+};
+
+/* values for above global_facil_mask */
+
+#defineX25_MASK_REVERSE0x01
+#defineX25_MASK_THROUGHPUT 0x02
+#defineX25_MASK_PACKET_SIZE0x04
+#defineX25_MASK_WINDOW_SIZE0x08
+
+#define X25_MASK_CALLING_AE 0x10
+#define X25_MASK_CALLED_AE 0x20
+
+
/*
* Call User Data structure.
*/
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index d2a52c254cca..4c2a395fdbdb 100644
--- a/net/x25/af_x25.c
+++ b/net/x25
[PATCH net-next v2 3/6] net/x25: replace x25_kill_by_device with x25_kill_by_neigh
Remove unnecessary function x25_kill_by_device.
Signed-off-by: Martin Schiller
---
Change from v1:
fix 'subject_prefix' warning
---
net/x25/af_x25.c | 22 +-
1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 4c2a395fdbdb..25726120fcc7 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -212,22 +212,6 @@ static void x25_remove_socket(struct sock *sk)
write_unlock_bh(_list_lock);
}
-/*
- * Kill all bound sockets on a dropped device.
- */
-static void x25_kill_by_device(struct net_device *dev)
-{
- struct sock *s;
-
- write_lock_bh(_list_lock);
-
- sk_for_each(s, _list)
- if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
- x25_disconnect(s, ENETUNREACH, 0, 0);
-
- write_unlock_bh(_list_lock);
-}
-
/*
* Handle device status changes.
*/
@@ -273,7 +257,11 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
case NETDEV_DOWN:
pr_debug("X.25: got event NETDEV_DOWN for device: %s\n",
dev->name);
- x25_kill_by_device(dev);
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_kill_by_neigh(nb);
+ x25_neigh_put(nb);
+ }
x25_route_device_down(dev);
x25_link_device_down(dev);
break;
--
2.20.1
[PATCH net-next v2 4/6] net/x25: support NETDEV_CHANGE notifier
This makes it possible to handle carrier lost and detection.
In case of carrier lost, we shutdown layer 3 and flush all sessions.
Signed-off-by: Martin Schiller
---
Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings
---
net/x25/af_x25.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 25726120fcc7..6a95ca11694e 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -275,6 +275,19 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
dev->name);
x25_link_device_remove(dev);
break;
+ case NETDEV_CHANGE:
+ pr_debug("X.25: got event NETDEV_CHANGE for device:
%s\n",
+dev->name);
+ if (!netif_carrier_ok(dev)) {
+ pr_debug("X.25: Carrier lost -> set link state
down: %s\n",
+dev->name);
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_link_terminated(nb);
+ x25_neigh_put(nb);
+ }
+ }
+ break;
}
}
--
2.20.1
[PATCH net-next v2 0/6] netdev event handling + neighbour config
Martin Schiller (6): net/x25: handle additional netdev events net/x25: make neighbour params configurable net/x25: replace x25_kill_by_device with x25_kill_by_neigh net/x25: support NETDEV_CHANGE notifier net/lapb: support netdev events net/lapb: fix t1 timer handling include/net/x25.h| 10 +- include/uapi/linux/x25.h | 56 ++- net/lapb/lapb_iface.c| 83 net/lapb/lapb_timer.c| 11 ++- net/x25/af_x25.c | 206 +++ net/x25/x25_facilities.c | 6 +- net/x25/x25_link.c | 142 +++ net/x25/x25_subr.c | 22 - 8 files changed, 445 insertions(+), 91 deletions(-) -- 2.20.1
[PATCH net-next v2 1/6] net/x25: handle additional netdev events
Add / remove x25_link_device by NETDEV_REGISTER/UNREGISTER and also by
NETDEV_POST_TYPE_CHANGE/NETDEV_PRE_TYPE_CHANGE.
This change is needed so that the x25_neigh struct for an interface is
already created when it shows up and is kept independently if the
interface goes UP or DOWN.
This is used in the next commit, where x25 params of an neighbour will
get configurable through ioctls.
Signed-off-by: Martin Schiller
---
Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings
---
include/net/x25.h | 2 ++
net/x25/af_x25.c | 26 ++
net/x25/x25_link.c | 45 +++--
3 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/include/net/x25.h b/include/net/x25.h
index d7d6c2b4ffa7..4c1502e8b2b2 100644
--- a/include/net/x25.h
+++ b/include/net/x25.h
@@ -231,6 +231,8 @@ int x25_backlog_rcv(struct sock *, struct sk_buff *);
/* x25_link.c */
void x25_link_control(struct sk_buff *, struct x25_neigh *, unsigned short);
+void x25_link_device_add(struct net_device *dev);
+void x25_link_device_remove(struct net_device *dev);
void x25_link_device_up(struct net_device *);
void x25_link_device_down(struct net_device *);
void x25_link_established(struct x25_neigh *);
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 046d3fee66a9..d2a52c254cca 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -233,10 +233,24 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
#endif
) {
switch (event) {
+ case NETDEV_REGISTER:
+ pr_debug("X.25: got event NETDEV_REGISTER for device:
%s\n",
+dev->name);
+ x25_link_device_add(dev);
+ break;
+ case NETDEV_POST_TYPE_CHANGE:
+ pr_debug("X.25: got event NETDEV_POST_TYPE_CHANGE for
device: %s\n",
+dev->name);
+ x25_link_device_add(dev);
+ break;
case NETDEV_UP:
+ pr_debug("X.25: got event NETDEV_UP for device: %s\n",
+dev->name);
x25_link_device_up(dev);
break;
case NETDEV_GOING_DOWN:
+ pr_debug("X.25: got event NETDEV_GOING_DOWN for device:
%s\n",
+dev->name);
nb = x25_get_neigh(dev);
if (nb) {
x25_terminate_link(nb);
@@ -244,10 +258,22 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
}
break;
case NETDEV_DOWN:
+ pr_debug("X.25: got event NETDEV_DOWN for device: %s\n",
+dev->name);
x25_kill_by_device(dev);
x25_route_device_down(dev);
x25_link_device_down(dev);
break;
+ case NETDEV_PRE_TYPE_CHANGE:
+ pr_debug("X.25: got event NETDEV_PRE_TYPE_CHANGE for
device: %s\n",
+dev->name);
+ x25_link_device_remove(dev);
+ break;
+ case NETDEV_UNREGISTER:
+ pr_debug("X.25: got event NETDEV_UNREGISTER for device:
%s\n",
+dev->name);
+ x25_link_device_remove(dev);
+ break;
}
}
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index fdae054b7dc1..92828a8a4ada 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -239,9 +239,17 @@ void x25_link_terminated(struct x25_neigh *nb)
/*
* Add a new device.
*/
-void x25_link_device_up(struct net_device *dev)
+void x25_link_device_add(struct net_device *dev)
{
- struct x25_neigh *nb = kmalloc(sizeof(*nb), GFP_ATOMIC);
+ struct x25_neigh *nb = x25_get_neigh(dev);
+
+ /* Check, if we already have a neighbour for this device */
+ if (nb) {
+ x25_neigh_put(nb);
+ return;
+ }
+
+ nb = kmalloc(sizeof(*nb), GFP_ATOMIC);
if (!nb)
return;
@@ -268,6 +276,20 @@ void x25_link_device_up(struct net_device *dev)
write_unlock_bh(_neigh_list_lock);
}
+/* A device is coming up */
+void x25_link_device_up(struct net_device *dev)
+{
+ struct x25_neigh *nb = x25_get_neigh(dev);
+
+ if (!nb)
+ return;
+
+ nb->state = X25_LINK_STATE_1;
+ x25_establish_link(nb);
+
+ x25_neigh_put(nb);
+}
+
/**
* __x25_remove_neigh - remove neighbour from x25_neigh_list
* @nb: - neigh to remove
@@ -277,9 +299,6
[PATCH net-next v3] net/tun: Call netdev notifiers
Call netdev notifiers before and after changing the device type.
Signed-off-by: Martin Schiller
---
Change from v2:
use subject_prefix 'net-next' to fix 'fixes_present' issue
Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings
---
drivers/net/tun.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 3d45d56172cb..2d9a00f41023 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3071,9 +3071,13 @@ static long __tun_chr_ioctl(struct file *file, unsigned
int cmd,
"Linktype set failed because interface is
up\n");
ret = -EBUSY;
} else {
+ call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE,
+tun->dev);
tun->dev->type = (int) arg;
netif_info(tun, drv, tun->dev, "linktype set to %d\n",
tun->dev->type);
+ call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
+tun->dev);
ret = 0;
}
break;
--
2.20.1
[PATCH net v2] net/tun: Call netdev notifiers
Call netdev notifiers before and after changing the device type.
Signed-off-by: Martin Schiller
---
Change from v1:
fix 'subject_prefix' and 'checkpatch' warnings
---
drivers/net/tun.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index be69d272052f..f3b337d45ad0 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3124,9 +3124,13 @@ static long __tun_chr_ioctl(struct file *file, unsigned
int cmd,
"Linktype set failed because interface is
up\n");
ret = -EBUSY;
} else {
+ call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE,
+tun->dev);
tun->dev->type = (int) arg;
netif_info(tun, drv, tun->dev, "linktype set to %d\n",
tun->dev->type);
+ call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
+tun->dev);
ret = 0;
}
break;
--
2.20.1
Re: [PATCH 1/6] net/x25: add/remove x25_link_device by NETDEV_REGISTER/UNREGISTER
On 2020-11-16 09:45, Xie He wrote: Hi Martin, Thanks for the series. To get the series applied faster, could you address the warnings and failures shown on this page? https://patchwork.kernel.org/project/netdevbpf/list/?submitter=174539 Thanks! To let the netdev robot know which tree this series should be applied, we can use [PATCH net-next 1/6] as the subject prefix. Hi Xie! Thanks. I will have a look at this.
Re: [PATCH net-next] MAINTAINERS: Add Martin Schiller as a maintainer for the X.25 stack
On 2020-11-14 12:10, Xie He wrote: Martin Schiller is an active developer and reviewer for the X.25 code. His company is providing products based on the Linux X.25 stack. So he is a good candidate for maintainers of the X.25 code. The original maintainer of the X.25 network layer (Andrew Hendry) has not sent any email to the netdev mail list since 2013. So he is probably inactive now. Cc: Martin Schiller Cc: Andrew Hendry Signed-off-by: Xie He --- MAINTAINERS | 19 +-- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index af9f6a3ab100..ab8b2c9ad00e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9842,13 +9842,6 @@ S: Maintained F: arch/mips/lantiq F: drivers/soc/lantiq -LAPB module -L: linux-...@vger.kernel.org -S: Orphan -F: Documentation/networking/lapb-module.rst -F: include/*/lapb.h -F: net/lapb/ - LASI 53c700 driver for PARISC M: "James E.J. Bottomley" L: linux-s...@vger.kernel.org @@ -18986,12 +18979,18 @@ L:linux-kernel@vger.kernel.org S: Maintained N: axp[128] -X.25 NETWORK LAYER -M: Andrew Hendry +X.25 STACK +M: Martin Schiller L: linux-...@vger.kernel.org -S: Odd Fixes +S: Maintained +F: Documentation/networking/lapb-module.rst F: Documentation/networking/x25* +F: drivers/net/wan/hdlc_x25.c +F: drivers/net/wan/lapbether.c +F: include/*/lapb.h F: include/net/x25* +F: include/uapi/linux/x25.h +F: net/lapb/ F: net/x25/ X86 ARCHITECTURE (32-BIT AND 64-BIT) Acked-by: Martin Schiller
[PATCH] net/tun: Call notifiers before and after changing device type
Signed-off-by: Martin Schiller
---
drivers/net/tun.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index be69d272052f..8253c5b03105 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3124,9 +3124,11 @@ static long __tun_chr_ioctl(struct file *file, unsigned
int cmd,
"Linktype set failed because interface is
up\n");
ret = -EBUSY;
} else {
+ call_netdevice_notifiers(NETDEV_PRE_TYPE_CHANGE,
tun->dev);
tun->dev->type = (int) arg;
netif_info(tun, drv, tun->dev, "linktype set to %d\n",
tun->dev->type);
+ call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
tun->dev);
ret = 0;
}
break;
--
2.20.1
[PATCH 6/6] net/lapb: fix t1 timer handling
fix t1 timer handling in LAPB_STATE_0:
o DTE interface changes immediately to LAPB_STATE_1 and start sending
SABM(E).
o DCE interface sends N2-times DM and changes to LAPB_STATE_1
afterwards if there is no response in the meantime.
Signed-off-by: Martin Schiller
---
net/lapb/lapb_timer.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 8f5b17001a07..d1f80d67892f 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -85,11 +85,16 @@ static void lapb_t1timer_expiry(struct timer_list *t)
switch (lapb->state) {
/*
-* If we are a DCE, keep going DM .. DM .. DM
+* If we are a DCE, send DM up to N2 times, then switch to
STATE_1 and send SABM(E)
*/
case LAPB_STATE_0:
- if (lapb->mode & LAPB_DCE)
+ if (lapb->mode & LAPB_DCE && lapb->n2count != lapb->n2)
{
+ lapb->n2count++;
lapb_send_control(lapb, LAPB_DM, LAPB_POLLOFF,
LAPB_RESPONSE);
+ } else {
+ lapb->state = LAPB_STATE_1;
+ lapb_establish_data_link(lapb);
+ }
break;
/*
--
2.20.1
[PATCH 1/6] net/x25: add/remove x25_link_device by NETDEV_REGISTER/UNREGISTER
and also by NETDEV_POST_TYPE_CHANGE/NETDEV_PRE_TYPE_CHANGE.
This change is needed so that the x25_neigh struct for an interface is
already created when it shows up and is kept independantly if the
interface goes UP or DOWN.
This is used in the next commit, where x25 params of an neighbour will
get configurable through ioctls.
Signed-off-by: Martin Schiller
---
include/net/x25.h | 2 ++
net/x25/af_x25.c | 19 +
net/x25/x25_link.c | 51 --
3 files changed, 66 insertions(+), 6 deletions(-)
diff --git a/include/net/x25.h b/include/net/x25.h
index d7d6c2b4ffa7..af841c5ede28 100644
--- a/include/net/x25.h
+++ b/include/net/x25.h
@@ -231,6 +231,8 @@ int x25_backlog_rcv(struct sock *, struct sk_buff *);
/* x25_link.c */
void x25_link_control(struct sk_buff *, struct x25_neigh *, unsigned short);
+void x25_link_device_add(struct net_device *);
+void x25_link_device_remove(struct net_device *);
void x25_link_device_up(struct net_device *);
void x25_link_device_down(struct net_device *);
void x25_link_established(struct x25_neigh *);
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index a10487e7574c..d8e5ca251801 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -233,10 +233,20 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
#endif
) {
switch (event) {
+ case NETDEV_REGISTER:
+ pr_debug("X.25: got event NETDEV_REGISTER for device:
%s\n", dev->name);
+ x25_link_device_add(dev);
+ break;
+ case NETDEV_POST_TYPE_CHANGE:
+ pr_debug("X.25: got event NETDEV_POST_TYPE_CHANGE for
device: %s\n", dev->name);
+ x25_link_device_add(dev);
+ break;
case NETDEV_UP:
+ pr_debug("X.25: got event NETDEV_UP for device: %s\n",
dev->name);
x25_link_device_up(dev);
break;
case NETDEV_GOING_DOWN:
+ pr_debug("X.25: got event NETDEV_GOING_DOWN for device:
%s\n", dev->name);
nb = x25_get_neigh(dev);
if (nb) {
x25_terminate_link(nb);
@@ -244,10 +254,19 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
}
break;
case NETDEV_DOWN:
+ pr_debug("X.25: got event NETDEV_DOWN for device:
%s\n", dev->name);
x25_kill_by_device(dev);
x25_route_device_down(dev);
x25_link_device_down(dev);
break;
+ case NETDEV_PRE_TYPE_CHANGE:
+ pr_debug("X.25: got event NETDEV_PRE_TYPE_CHANGE for
device: %s\n", dev->name);
+ x25_link_device_remove(dev);
+ break;
+ case NETDEV_UNREGISTER:
+ pr_debug("X.25: got event NETDEV_UNREGISTER for device:
%s\n", dev->name);
+ x25_link_device_remove(dev);
+ break;
}
}
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index fdae054b7dc1..22055ee40056 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -239,9 +239,19 @@ void x25_link_terminated(struct x25_neigh *nb)
/*
* Add a new device.
*/
-void x25_link_device_up(struct net_device *dev)
+void x25_link_device_add(struct net_device *dev)
{
- struct x25_neigh *nb = kmalloc(sizeof(*nb), GFP_ATOMIC);
+ struct x25_neigh *nb = x25_get_neigh(dev);
+
+ /*
+* Check, if we already have a neighbour for this device
+*/
+ if (nb) {
+ x25_neigh_put(nb);
+ return;
+ }
+
+ nb = kmalloc(sizeof(*nb), GFP_ATOMIC);
if (!nb)
return;
@@ -268,6 +278,22 @@ void x25_link_device_up(struct net_device *dev)
write_unlock_bh(_neigh_list_lock);
}
+/*
+ * A device is coming up
+ */
+void x25_link_device_up(struct net_device *dev)
+{
+ struct x25_neigh *nb = x25_get_neigh(dev);
+
+ if (!nb)
+ return;
+
+ nb->state = X25_LINK_STATE_1;
+ x25_establish_link(nb);
+
+ x25_neigh_put(nb);
+}
+
/**
* __x25_remove_neigh - remove neighbour from x25_neigh_list
* @nb: - neigh to remove
@@ -277,9 +303,6 @@ void x25_link_device_up(struct net_device *dev)
*/
static void __x25_remove_neigh(struct x25_neigh *nb)
{
- skb_queue_purge(>queue);
- x25_stop_t20timer(nb);
-
if (nb->node.next) {
list_del(>node);
x25_neigh_put(nb);
@@ -289,7 +312,7 @@ static void __x25_remove_neigh(struct
[PATCH 5/6] net/lapb: support netdev events
This makes it possible to handle carrier loss and detection.
In case of Carrier Loss, layer 2 is terminated
In case of Carrier Detection, we start timer t1 on a DCE interface,
and on a DTE interface we change to state LAPB_STATE_1 and start
sending SABM(E).
Signed-off-by: Martin Schiller
---
net/lapb/lapb_iface.c | 74 +++
1 file changed, 74 insertions(+)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 3c03f6512c5f..6a109c8c286f 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -418,14 +418,88 @@ int lapb_data_transmit(struct lapb_cb *lapb, struct
sk_buff *skb)
return used;
}
+/*
+ * Handle device status changes.
+ */
+static int lapb_device_event(struct notifier_block *this, unsigned long event,
+ void *ptr)
+{
+ struct net_device *dev = ptr;
+ struct lapb_cb *lapb;
+
+ if (!net_eq(dev_net(dev), _net))
+ return NOTIFY_DONE;
+
+ if (dev->type == ARPHRD_X25) {
+ switch (event) {
+ case NETDEV_REGISTER:
+ lapb_dbg(0, "(%p): got event NETDEV_REGISTER for
device: %s\n", dev, dev->name);
+ break;
+ case NETDEV_POST_TYPE_CHANGE:
+ lapb_dbg(0, "(%p): got event NETDEV_POST_TYPE_CHANGE
for device: %s\n", dev, dev->name);
+ break;
+ case NETDEV_UP:
+ lapb_dbg(0, "(%p): got event NETDEV_UP for device:
%s\n", dev, dev->name);
+ break;
+ case NETDEV_GOING_DOWN:
+ lapb_dbg(0, "(%p): got event NETDEV_GOING_DOWN for
device: %s\n", dev, dev->name);
+ break;
+ case NETDEV_DOWN:
+ lapb_dbg(0, "(%p): got event NETDEV_DOWN for device:
%s\n", dev, dev->name);
+ break;
+ case NETDEV_PRE_TYPE_CHANGE:
+ lapb_dbg(0, "(%p): got event NETDEV_PRE_TYPE_CHANGE for
device: %s\n", dev, dev->name);
+ break;
+ case NETDEV_UNREGISTER:
+ lapb_dbg(0, "(%p): got event NETDEV_UNREGISTER for
device: %s\n", dev, dev->name);
+ break;
+ case NETDEV_CHANGE:
+ lapb_dbg(0, "(%p): got event NETDEV_CHANGE for device:
%s\n", dev, dev->name);
+ lapb = lapb_devtostruct(dev);
+ if (lapb) {
+ if (!netif_carrier_ok(dev)) {
+ lapb_dbg(0, "(%p): Carrier lost ->
Entering LAPB_STATE_0: %s\n", dev, dev->name);
+ lapb_disconnect_indication(lapb,
LAPB_OK);
+ lapb_clear_queues(lapb);
+ lapb->state = LAPB_STATE_0;
+ lapb->n2count = 0;
+ lapb_stop_t1timer(lapb);
+ lapb_stop_t2timer(lapb);
+ } else {
+ lapb_dbg(0, "(%p): Carrier detected:
%s\n", dev, dev->name);
+ if (lapb->mode & LAPB_DCE) {
+ lapb_start_t1timer(lapb);
+ } else {
+ if (lapb->state ==
LAPB_STATE_0) {
+ lapb->state =
LAPB_STATE_1;
+
lapb_establish_data_link(lapb);
+ }
+ }
+ }
+ }
+ break;
+ }
+ }
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block lapb_dev_notifier = {
+ .notifier_call = lapb_device_event,
+};
+
static int __init lapb_init(void)
{
+ register_netdevice_notifier(_dev_notifier);
+
return 0;
}
static void __exit lapb_exit(void)
{
WARN_ON(!list_empty(_list));
+
+ unregister_netdevice_notifier(_dev_notifier);
}
MODULE_AUTHOR("Jonathan Naylor ");
--
2.20.1
[PATCH 3/6] net/x25: replace x25_kill_by_device with x25_kill_by_neigh
Remove unnecessary function x25_kill_by_device.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c | 22 +-
1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 439ae65ab7a8..d98d1145500e 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -210,22 +210,6 @@ static void x25_remove_socket(struct sock *sk)
write_unlock_bh(_list_lock);
}
-/*
- * Kill all bound sockets on a dropped device.
- */
-static void x25_kill_by_device(struct net_device *dev)
-{
- struct sock *s;
-
- write_lock_bh(_list_lock);
-
- sk_for_each(s, _list)
- if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
- x25_disconnect(s, ENETUNREACH, 0, 0);
-
- write_unlock_bh(_list_lock);
-}
-
/*
* Handle device status changes.
*/
@@ -266,7 +250,11 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
break;
case NETDEV_DOWN:
pr_debug("X.25: got event NETDEV_DOWN for device:
%s\n", dev->name);
- x25_kill_by_device(dev);
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_kill_by_neigh(nb);
+ x25_neigh_put(nb);
+ }
x25_route_device_down(dev);
x25_link_device_down(dev);
break;
--
2.20.1
[PATCH 4/6] net/x25: support NETDEV_CHANGE notifier
This makes it possible to handle carrier lost and detection.
In case of carrier lost, we shutdown layer 3 and flush all sessions.
Signed-off-by: Martin Schiller
---
net/x25/af_x25.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index d98d1145500e..d61a154b94e4 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -266,6 +266,17 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
pr_debug("X.25: got event NETDEV_UNREGISTER for device:
%s\n", dev->name);
x25_link_device_remove(dev);
break;
+ case NETDEV_CHANGE:
+ pr_debug("X.25: got event NETDEV_CHANGE for device:
%s\n", dev->name);
+ if (!netif_carrier_ok(dev)) {
+ pr_debug("X.25: Carrier lost -> set link state
down: %s\n", dev->name);
+ nb = x25_get_neigh(dev);
+ if (nb) {
+ x25_link_terminated(nb);
+ x25_neigh_put(nb);
+ }
+ }
+ break;
}
}
--
2.20.1
[PATCH 2/6] net/x25: make neighbour params configurable
Extended struct x25_neigh and x25_subscrip_struct to configure following
params through SIOCX25SSUBSCRIP:
o mode (DTE/DCE)
o number of channels
o facilities (packet size, window size)
o timer T20
Based on this configuration options the follwing changes/extensions
where made:
o DTE/DCE handling to select the next lc (DCE=from bottom / DTE=from
top)
o DTE/DCE handling to set correct clear/reset/restart cause
o take default facilities from neighbour settings
Signed-off-by: Martin Schiller
---
include/net/x25.h| 7 ++-
include/uapi/linux/x25.h | 54
net/x25/af_x25.c | 132 ---
net/x25/x25_facilities.c | 6 +-
net/x25/x25_link.c | 104 +-
net/x25/x25_subr.c | 22 ++-
6 files changed, 255 insertions(+), 70 deletions(-)
diff --git a/include/net/x25.h b/include/net/x25.h
index af841c5ede28..6e8600456d39 100644
--- a/include/net/x25.h
+++ b/include/net/x25.h
@@ -140,6 +140,9 @@ struct x25_neigh {
struct net_device *dev;
unsigned intstate;
unsigned intextended;
+ unsigned intdce;
+ unsigned intlc;
+ struct x25_facilities facilities;
struct sk_buff_head queue;
unsigned long t20;
struct timer_list t20timer;
@@ -164,6 +167,7 @@ struct x25_sock {
struct timer_list timer;
struct x25_causediagcausediag;
struct x25_facilities facilities;
+ unsigned intsocket_defined_facilities; /* set, if
facilities changed by SIOCX25SFACILITIES */
struct x25_dte_facilities dte_facilities;
struct x25_calluserdata calluserdata;
unsigned long vc_facil_mask; /* inc_call facilities mask */
@@ -215,7 +219,8 @@ int x25_create_facilities(unsigned char *, struct
x25_facilities *,
struct x25_dte_facilities *, unsigned long);
int x25_negotiate_facilities(struct sk_buff *, struct sock *,
struct x25_facilities *,
-struct x25_dte_facilities *);
+ struct x25_dte_facilities *,
+ struct x25_neigh *);
void x25_limit_facilities(struct x25_facilities *, struct x25_neigh *);
/* x25_forward.c */
diff --git a/include/uapi/linux/x25.h b/include/uapi/linux/x25.h
index 034b7dc5593a..963848e94880 100644
--- a/include/uapi/linux/x25.h
+++ b/include/uapi/linux/x25.h
@@ -63,31 +63,6 @@ struct sockaddr_x25 {
struct x25_address sx25_addr; /* X.121 Address */
};
-/*
- * DTE/DCE subscription options.
- *
- * As this is missing lots of options, user should expect major
- * changes of this structure in 2.5.x which might break compatibilty.
- * The somewhat ugly dimension 200-sizeof() is needed to maintain
- * backward compatibility.
- */
-struct x25_subscrip_struct {
- char device[200-sizeof(unsigned long)];
- unsigned long global_facil_mask; /* 0 to disable negotiation */
- unsigned intextended;
-};
-
-/* values for above global_facil_mask */
-
-#defineX25_MASK_REVERSE0x01
-#defineX25_MASK_THROUGHPUT 0x02
-#defineX25_MASK_PACKET_SIZE0x04
-#defineX25_MASK_WINDOW_SIZE0x08
-
-#define X25_MASK_CALLING_AE 0x10
-#define X25_MASK_CALLED_AE 0x20
-
-
/*
* Routing table control structure.
*/
@@ -127,6 +102,35 @@ struct x25_dte_facilities {
__u8 called_ae[20];
};
+/*
+ * DTE/DCE subscription options.
+ *
+ * As this is missing lots of options, user should expect major
+ * changes of this structure in 2.5.x which might break compatibilty.
+ * The somewhat ugly dimension 200-sizeof() is needed to maintain
+ * backward compatibility.
+ */
+struct x25_subscrip_struct {
+ char device[200-((2 * sizeof(unsigned long)) + sizeof(struct
x25_facilities) + (2 * sizeof(unsigned int)))];
+ unsigned intdce;
+ unsigned intlc;
+ struct x25_facilities facilities;
+ unsigned long t20;
+ unsigned long global_facil_mask; /* 0 to disable
negotiation */
+ unsigned intextended;
+};
+
+/* values for above global_facil_mask */
+
+#defineX25_MASK_REVERSE0x01
+#defineX25_MASK_THROUGHPUT 0x02
+#defineX25_MASK_PACKET_SIZE0x04
+#defineX25_MASK_WINDOW_SIZE0x08
+
+#define X25_MASK_CALLING_AE 0x10
+#define X25_MASK_CALLED_AE 0x20
+
+
/*
* Call User Data structure.
*/
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index d8e5ca251801..439ae65ab7a8 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -72,8 +72,19 @@ static const struct proto_ops x25_proto_ops;
static const struct x25_address null_x25_address = {" "};
#ifdef CONFIG_COMP
Re: linux-x25 mail list not working
On 2020-11-13 03:17, John 'Warthog9' Hawley wrote: Give it a try now, there was a little wonkiness with the alias setup for it, and I have no historical context for a 'why', but I adjusted a couple of things and I was able to subscribe myself. - John 'Warthog9' Hawley Thanks a lot John! Now it seems to work again. - Martin On 11/12/2020 10:27 AM, Xie He wrote: Hi Linux maintainers, The linux-x25 mail list doesn't seem to be working. We sent a lot of emails to linux-x25 but Martin Schiller as a subscriber hasn't received a single email from the mail list. Looking at the mail list archive at: https://www.spinics.net/lists/linux-x25/ I see the last email in the archive was in 2009. It's likely that this mail list has stopped working since 2009. Can you please help fix this mail list. Thanks! Xie He
Re: [PATCH net-next RFC] MAINTAINERS: Add Martin Schiller as a maintainer for the X.25 stack
On 2020-11-11 22:36, Xie He wrote: Hi Martin Schiller, would you like to be a maintainer for the X.25 stack? As we know the linux-x25 mail list has stopped working for a long time. Adding you as a maintainer may make you be Cc'd for new patches so that you can review them. About 1 year ago I was asked by Arnd Bergmann if I would like to become the maintainer for the X.25 stack: https://patchwork.ozlabs.org/project/netdev/patch/20191209151256.2497534-4-a...@arndb.de/#2320767 Yes, I would agree to be listed as a maintainer. But I still think it is important that we either repair or delete the linux-x25 mailing list. The current state that the messages are going into nirvana is very unpleasant. The original maintainer of X.25 network layer (Andrew Hendry) has stopped sending emails to the netdev mail list since 2013. So there is practically no maintainer for X.25 code currently. Cc: Martin Schiller Cc: Andrew Hendry Signed-off-by: Xie He --- MAINTAINERS | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 2a0fde12b650..9ebcb0708d5d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9832,13 +9832,6 @@ S: Maintained F: arch/mips/lantiq F: drivers/soc/lantiq -LAPB module -L: linux-...@vger.kernel.org -S: Orphan -F: Documentation/networking/lapb-module.rst -F: include/*/lapb.h -F: net/lapb/ - LASI 53c700 driver for PARISC M: "James E.J. Bottomley" L: linux-s...@vger.kernel.org @@ -18979,12 +18972,19 @@ L:linux-kernel@vger.kernel.org S: Maintained N: axp[128] -X.25 NETWORK LAYER -M: Andrew Hendry +X.25 STACK +M: Martin Schiller L: linux-...@vger.kernel.org -S: Odd Fixes +L: net...@vger.kernel.org +S: Maintained +F: Documentation/networking/lapb-module.txt F: Documentation/networking/x25* +F: drivers/net/wan/hdlc_x25.c +F: drivers/net/wan/lapbether.c +F: include/*/lapb.h F: include/net/x25* +F: include/uapi/linux/x25.h +F: net/lapb/ F: net/x25/ X86 ARCHITECTURE (32-BIT AND 64-BIT)
Re: [PATCH net] net: x25: Fix kernel crashes due to x25_disconnect releasing x25_neigh
On 2020-11-11 11:04, Xie He wrote:
The x25_disconnect function in x25_subr.c would decrease the refcount
of
"x25->neighbour" (struct x25_neigh) and reset this pointer to NULL.
However:
1) When we receive a connection, the x25_rx_call_request function in
af_x25.c does not increase the refcount when it assigns the pointer.
When we disconnect, x25_disconnect is called and the struct's refcount
is decreased without being increased in the first place.
Yes, this is a problem and should be fixed. As an alternative to your
approach, you could also go the way to prevent the call of
x25_neigh_put(nb) in x25_lapb_receive_frame() in case of a Call Request.
However, this would require more effort.
This causes frequent kernel crashes when using AF_X25 sockets.
2) When we initiate a connection but the connection is refused by the
remote side, x25_disconnect is called which decreases the refcount and
resets the pointer to NULL. But the x25_connect function in af_x25.c,
which is waiting for the connection to be established, notices the
failure and then tries to decrease the refcount again, resulting in a
NULL-pointer-dereference error.
This crashes the kernel every time a connection is refused by the
remote
side.
For this bug I already sent a fix some time ago (last time I sent a
RESEND yesterday), but unfortunately it was not merged yet:
https://lore.kernel.org/patchwork/patch/1334917/
Fixes: 4becb7ee5b3d ("net/x25: Fix x25_neigh refcnt leak when x25
disconnect")
Cc: Martin Schiller
Signed-off-by: Xie He
---
net/x25/af_x25.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 0bbb283f23c9..8e59f9ecbeab 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -826,10 +826,12 @@ static int x25_connect(struct socket *sock,
struct sockaddr *uaddr,
rc = 0;
out_put_neigh:
if (rc) {
- read_lock_bh(_list_lock);
- x25_neigh_put(x25->neighbour);
- x25->neighbour = NULL;
- read_unlock_bh(_list_lock);
+ if (x25->neighbour) {
+ read_lock_bh(_list_lock);
+ x25_neigh_put(x25->neighbour);
+ x25->neighbour = NULL;
+ read_unlock_bh(_list_lock);
+ }
x25->state = X25_STATE_0;
}
out_put_route:
@@ -1050,6 +1052,7 @@ int x25_rx_call_request(struct sk_buff *skb,
struct x25_neigh *nb,
makex25->lci = lci;
makex25->dest_addr = dest_addr;
makex25->source_addr = source_addr;
+ x25_neigh_hold(nb);
makex25->neighbour = nb;
makex25->facilities= facilities;
makex25->dte_facilities= dte_facilities;
