---
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
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 <[email protected]>
---
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(&nb->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(&nb->queue);
- x25_stop_t20timer(nb);
-
if (nb->node.next) {
list_del(&nb->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(&x25_route_list_lock);
-
- /* Remove any related forwarding */
- x25_clear_forward_by_dev(dev);
}
/*
--
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 | 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), &init_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_t2timer(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(&lapb_dev_notifier);
+
return 0;
}
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
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
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
On Tue, Nov 24, 2020 at 1:40 AM Martin Schiller <[email protected]> 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 <[email protected]>
Acked-by: Xie He <[email protected]>
Thanks!
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
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 <[email protected]>
> +/* 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) {
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(&lapb_dev_notifier);
This can fail, so:
return register_netdevice_notifier(&lapb_dev_notifier);
> return 0;
> }
>
> static void __exit lapb_exit(void)
> {
> WARN_ON(!list_empty(&lapb_list));
> +
> + unregister_netdevice_notifier(&lapb_dev_notifier);
> }
>
> MODULE_AUTHOR("Jonathan Naylor <[email protected]>");
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:
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 @@
#define LAPB_REFUSED 5
#define LAPB_TIMEDOUT 6
#define LAPB_NOMEM 7
+#define LAPB_NOCARRIER 8
#define LAPB_STANDARD 0x00
#define LAPB_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.
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 <[email protected]>
>
>> +/* 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) {
>
> 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(&lapb_dev_notifier);
>
> This can fail, so:
>
> return register_netdevice_notifier(&lapb_dev_notifier);
>
>> return 0;
>> }
>>
>> static void __exit lapb_exit(void)
>> {
>> WARN_ON(!list_empty(&lapb_list));
>> +
>> + unregister_netdevice_notifier(&lapb_dev_notifier);
>> }
>>
>> MODULE_AUTHOR("Jonathan Naylor <[email protected]>");
Thanks for your hints! I will send an update.
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 @@
> #define LAPB_REFUSED 5
> #define LAPB_TIMEDOUT 6
> #define LAPB_NOMEM 7
> +#define LAPB_NOCARRIER 8
>
> #define LAPB_STANDARD 0x00
> #define LAPB_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