| Message ID | 20201116135522.21791-6-ms@dev.tdt.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netdev event handling + neighbour config | 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, 97 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 Mon, Nov 16, 2020 at 6:01 AM Martin Schiller <ms@dev.tdt.de> wrote: > > 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). > + 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); > + } > + } Do you mean we will now automatically establish LAPB connections without upper layers instructing us to do so? If that is the case, is the one-byte header for instructing the LAPB layer to connect / disconnect no longer needed?
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.
On Tue, Nov 17, 2020 at 1:53 AM Martin Schiller <ms@dev.tdt.de> 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? 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. 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. > 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? > 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.0141@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? > > 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.
On 2020-11-17 12:32, Xie He wrote: > On Tue, Nov 17, 2020 at 1:53 AM Martin Schiller <ms@dev.tdt.de> 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-ms@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.0141@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/
On Tue, Nov 17, 2020 at 5:26 AM Martin Schiller <ms@dev.tdt.de> 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-ms@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. > > 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.
On 2020-11-17 19:28, Xie He wrote: > On Tue, Nov 17, 2020 at 5:26 AM Martin Schiller <ms@dev.tdt.de> 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-ms@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.
On Wed, Nov 18, 2020 at 12:49 AM Martin Schiller <ms@dev.tdt.de> wrote: > > Ah, ok. Now I see what you mean. > Yes, we should check the lapb->mode in lapb_connect_request(). ... > 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.
On Wed, Nov 18, 2020 at 5:03 AM Xie He <xie.he.0141@gmail.com> wrote: > > On Wed, Nov 18, 2020 at 12:49 AM Martin Schiller <ms@dev.tdt.de> 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.
On 2020-11-18 14:46, Xie He wrote: > On Wed, Nov 18, 2020 at 5:03 AM Xie He <xie.he.0141@gmail.com> wrote: >> >> On Wed, Nov 18, 2020 at 12:49 AM Martin Schiller <ms@dev.tdt.de> >> 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.
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), &init_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(&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 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 <ms@dev.tdt.de> --- Change from v1: fix 'subject_prefix' and 'checkpatch' warnings --- net/lapb/lapb_iface.c | 83 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)