[PATCH net-next v3 0/6] net/x25: netdev event handling

[PATCH net-next v3 0/6] net/x25: netdev event handling

[Martin Schiller]

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

[PATCH net-next v3 1/6] net/x25: handle additional netdev events

[Martin Schiller]

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 <ms@dev.tdt.de>
---
 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 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(&x25_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 @@ void x25_link_device_up(struct net_device *dev)
  */
 static void __x25_remove_neigh(struct x25_neigh *nb)
 {
-	skb_queue_purge(&nb->queue);
-	x25_stop_t20timer(nb);
-
 	if (nb->node.next) {
 		list_del(&nb->node);
 		x25_neigh_put(nb);
@@ -289,7 +308,7 @@ static void __x25_remove_neigh(struct x25_neigh *nb)
 /*
  *	A device has been removed, remove its links.
  */
-void x25_link_device_down(struct net_device *dev)
+void x25_link_device_remove(struct net_device *dev)
 {
 	struct x25_neigh *nb;
 	struct list_head *entry, *tmp;
@@ -308,6 +327,20 @@ void x25_link_device_down(struct net_device *dev)
 	write_unlock_bh(&x25_neigh_list_lock);
 }
 
+/* A device is going down */
+void x25_link_device_down(struct net_device *dev)
+{
+	struct x25_neigh *nb = x25_get_neigh(dev);
+
+	if (!nb)
+		return;
+
+	skb_queue_purge(&nb->queue);
+	x25_stop_t20timer(nb);
+
+	x25_neigh_put(nb);
+}
+
 /*
  *	Given a device, return the neighbour address.
  */
-- 
2.20.1

[PATCH net-next v3 2/6] net/lapb: fix lapb_connect_request() for DCE

[Martin Schiller]

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 <ms@dev.tdt.de>
---
 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 3/6] net/lapb: handle carrier loss correctly

[Martin Schiller]

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 <ms@dev.tdt.de>
---
 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 4/6] net/lapb: fix t1 timer handling for DCE

[Martin Schiller]

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 <ms@dev.tdt.de>
---
 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 5/6] net/x25: fix restart request/confirm handling

[Martin Schiller]

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 <ms@dev.tdt.de>
---
 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()

[Martin Schiller]

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 <ms@dev.tdt.de>
---
 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(&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.
  */
@@ -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(&x25_route_list_lock);
-
-	/* Remove any related forwarding */
-	x25_clear_forward_by_dev(dev);
 }
 
 /*
-- 
2.20.1

Re: [PATCH net-next v3 0/6] net/x25: netdev event handling

[Xie He]

On Wed, Nov 18, 2020 at 5:59 AM Martin Schiller <ms@dev.tdt.de> 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.

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

Re: [PATCH net-next v3 0/6] net/x25: netdev event handling

[Martin Schiller]

On 2020-11-18 15:47, Xie He wrote:
> On Wed, Nov 18, 2020 at 5:59 AM Martin Schiller <ms@dev.tdt.de> 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()

Re: [PATCH net-next v3 0/6] net/x25: netdev event handling

[Xie He]

On Wed, Nov 18, 2020 at 11:02 PM Martin Schiller <ms@dev.tdt.de> wrote:
>
> On 2020-11-18 15:47, Xie He wrote:
> >
> > 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.

OK. Thanks so much!

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

I see. Maybe we can do it this way:

When L3 wants to initiate the first L3 connection, it can check
whether the L2 connection is established, and if it is not, it can
instruct L2 to connect. This is the same as what the current code
(before this series) does.

When the last L3 connection is terminated, we can let L3 use the
one-byte header to "suggest" (rather than "instruct") L2 to terminate
the L2 connection. L2 can choose to either terminate the connection or
continue to keep it, based on whether it is in on-demand mode.