---
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
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 <[email protected]>
---
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), &init_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(&lapb_dev_notifier);
}
static void __exit lapb_exit(void)
{
WARN_ON(!list_empty(&lapb_list));
+
+ unregister_netdevice_notifier(&lapb_dev_notifier);
}
MODULE_AUTHOR("Jonathan Naylor <[email protected]>");
--
2.20.1
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 <[email protected]>
---
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
Remove obsolete function x25_kill_by_device(). It's not used any more.
Signed-off-by: Martin Schiller <[email protected]>
---
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(&x25_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(&x25_list_lock);
-
- sk_for_each(s, &x25_list)
- if (x25_sk(s)->neighbour && x25_sk(s)->neighbour->dev == dev)
- x25_disconnect(s, ENETUNREACH, 0, 0);
-
- write_unlock_bh(&x25_list_lock);
-}
-
/*
* Handle device status changes.
*/
--
2.20.1
We have to take the actual link state into account to handle
restart requests/confirms well.
Signed-off-by: Martin Schiller <[email protected]>
---
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
Acked-by: Xie He <[email protected]>
On Thu, 26 Nov 2020 07:35:52 +0100 Martin Schiller wrote:
> 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
Applied, thank you!
On Wed, Nov 25, 2020 at 10:36 PM Martin Schiller <[email protected]> wrote:
>
> We have to take the actual link state into account to handle
> restart requests/confirms well.
>
> @@ -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;
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.
On Wed, Dec 9, 2020 at 1:01 AM Xie He <[email protected]> wrote:
>
> On Wed, Nov 25, 2020 at 10:36 PM Martin Schiller <[email protected]> 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.
On 2020-12-09 10:17, Xie He wrote:
> On Wed, Dec 9, 2020 at 1:01 AM Xie He <[email protected]> wrote:
>>
>> On Wed, Nov 25, 2020 at 10:36 PM Martin Schiller <[email protected]>
>> 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.
On Wed, Dec 9, 2020 at 1:41 AM Martin Schiller <[email protected]> wrote:
>
> Right.
> By the way: A "Restart Collision" is in practice a very common event to
> establish the Layer 3.
Oh, I see. Thanks!
On Wed, Dec 9, 2020 at 1:47 AM Xie He <[email protected]> wrote:
>
> On Wed, Dec 9, 2020 at 1:41 AM Martin Schiller <[email protected]> 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.
On 2020-12-09 23:11, Xie He wrote:
> On Wed, Dec 9, 2020 at 1:47 AM Xie He <[email protected]> wrote:
>>
>> On Wed, Dec 9, 2020 at 1:41 AM Martin Schiller <[email protected]> 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
On Wed, Dec 9, 2020 at 10:27 PM Martin Schiller <[email protected]> wrote:
>
> > 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.
Thanks! I admit that this series is a big change and is not easy to
split up properly. If I were you, I may end up sending a very big
patch first, and then follow up with small patches for "restart
request/confirm handling" and "add/remove x25_neigh on
device-register/unregister instead of device-up/down". (The little
change in x25_link_established should belong to the first big patch
instead of "restart request/confirm handling".)
But making each patch work on its own is indeed preferable. I see
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
says:
When dividing your change into a series of patches, take special care
to ensure that the kernel builds and runs properly after each patch in
the series. Developers using git bisect to track down a problem can
end up splitting your patch series at any point; they will not thank
you if you introduce bugs in the middle.