| Message ID | 20201124093938.22012-3-ms@dev.tdt.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/x25: netdev event handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 108 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Nov 24, 2020 at 1:40 AM Martin Schiller <ms@dev.tdt.de> 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 <ms@dev.tdt.de> Acked-by: Xie He <xie.he.0141@gmail.com> Thanks!
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 <ms@dev.tdt.de> > +/* 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 <g4klx@g4klx.demon.co.uk>");
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 <ms@dev.tdt.de> > >> +/* 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 <g4klx@g4klx.demon.co.uk>"); 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
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 <g4klx@g4klx.demon.co.uk>");
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 <ms@dev.tdt.de> --- net/lapb/lapb_iface.c | 94 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+)