[PATCH net-next v4 0/5] net/x25: netdev event handling

[PATCH net-next v4 0/5] net/x25: netdev event handling

[Martin Schiller]

---

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 | 72 +++++++++++++++++++++++++++++++++++++++++++
 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, 133 insertions(+), 38 deletions(-)

-- 
2.20.1

[PATCH net-next v4 1/5] net/x25: handle additional netdev events

[Martin Schiller]

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

[PATCH net-next v4 2/5] net/lapb: support netdev events

[Martin Schiller]

This patch allows layer2 (LAPB) to react to netdev events itself and
avoids the detour via layer3 (X.25).

1. Call lapb_disconnect_request() on NETDEV_GOING_DOWN events.

2. When a NETDEV_DOWN event occur, clear all queues, enter state
   LAPB_STATE_0 and stop all timers.

3. 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 | 72 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 3c03f6512c5f..52d59984fbe6 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -418,14 +418,86 @@ 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_GOING_DOWN:
+			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 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);
+			} 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>");
-- 
2.20.1

[PATCH net-next v4 3/5] net/lapb: fix t1 timer handling for LAPB_STATE_0

[Martin Schiller]

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 <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 v4 4/5] 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.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
 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

[PATCH net-next v4 5/5] net/x25: remove x25_kill_by_device()

[Martin Schiller]

Remove obsolete function x25_kill_by_device(). It's not used any more.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---
 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

Re: [PATCH net-next v4 0/5] net/x25: netdev event handling

[Xie He]

On Thu, Nov 19, 2020 at 9:40 PM Martin Schiller <ms@dev.tdt.de> wrote:
>
> Changes to v3:
> o another complete rework of the patch-set to split event handling
>   for layer2 (LAPB) and layer3 (X.25)

Thanks for your work!!

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Xie He]

Should we also handle the NETDEV_UP event here? In previous versions
of this patch series you seemed to want to establish the L2 connection
on device-up. But in this patch, you didn't handle NETDEV_UP.

Maybe on device-up, we need to check if the carrier is up, and if it
is, we do the same thing as we do on carrier-up.

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Xie He]

On Fri, Nov 20, 2020 at 3:11 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> Should we also handle the NETDEV_UP event here? In previous versions
> of this patch series you seemed to want to establish the L2 connection
> on device-up. But in this patch, you didn't handle NETDEV_UP.
>
> Maybe on device-up, we need to check if the carrier is up, and if it
> is, we do the same thing as we do on carrier-up.

Are the device up/down status and the carrier up/down status
independent of each other? If they are, on device-up or carrier-up, we
only need to try establishing the L2 connection if we see both are up.

On NETDEV_GOING_DOWN, we can also check the carrier status first and
if it is down, we don't need to call lapb_disconnect_request.

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Martin Schiller]

On 2020-11-21 00:50, Xie He wrote:
> On Fri, Nov 20, 2020 at 3:11 PM Xie He <xie.he.0141@gmail.com> wrote:
>> 
>> Should we also handle the NETDEV_UP event here? In previous versions
>> of this patch series you seemed to want to establish the L2 connection
>> on device-up. But in this patch, you didn't handle NETDEV_UP.
>> 
>> Maybe on device-up, we need to check if the carrier is up, and if it
>> is, we do the same thing as we do on carrier-up.
> 
> Are the device up/down status and the carrier up/down status
> independent of each other? If they are, on device-up or carrier-up, we
> only need to try establishing the L2 connection if we see both are up.

No, they aren't independent. The carrier can only be up if the device /
interface is UP. And as far as I can see a NETDEV_CHANGE event will also
only be generated on interfaces that are UP.

So you can be sure, that if there is a NETDEV_CHANGE event then the
device is UP.

I removed the NETDEV_UP handling because I don't think it makes sense
to implicitly try to establish layer2 (LAPB) if there is no carrier.
And with the first X.25 connection request on that interface, it will
be established anyway by x25_transmit_link().

I've tested it here with an HDLC WAN Adapter and it works as expected.

These are also the ideal conditions for the already mentioned "on
demand" scenario. The only necessary change would be to call
x25_terminate_link() on an interface after clearing the last X.25
session.

> On NETDEV_GOING_DOWN, we can also check the carrier status first and
> if it is down, we don't need to call lapb_disconnect_request.

This is not necessary because lapb_disconnect_request() checks the
current state. And if the carrier is DOWN then the state should also be
LAPB_STATE_0 and so lapb_disconnect_request() does nothing.

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Xie He]

On Sun, Nov 22, 2020 at 10:55 PM Martin Schiller <ms@dev.tdt.de> wrote:
>
> No, they aren't independent. The carrier can only be up if the device /
> interface is UP. And as far as I can see a NETDEV_CHANGE event will also
> only be generated on interfaces that are UP.
>
> So you can be sure, that if there is a NETDEV_CHANGE event then the
> device is UP.

OK. Thanks for your explanation!

> I removed the NETDEV_UP handling because I don't think it makes sense
> to implicitly try to establish layer2 (LAPB) if there is no carrier.

As I understand, when the device goes up, the carrier can be either
down or up. Right?

If this is true, when a device goes up and the carrier then goes up
after that, L2 will automatically connect, but if a device goes up and
the carrier is already up, L2 will not automatically connect. I think
it might be better to eliminate this difference in handling. It might
be better to make it automatically connect in both situations, or in
neither situations.

If you want to go with the second way (auto connect in neither
situations), the next (3rd) patch of this series might be also not
needed.

I just want to make the behavior of LAPB more consistent. I think we
should either make LAPB auto-connect in all situations, or make LAPB
wait for L3's instruction to connect in all situations.

> And with the first X.25 connection request on that interface, it will
> be established anyway by x25_transmit_link().
>
> I've tested it here with an HDLC WAN Adapter and it works as expected.
>
> These are also the ideal conditions for the already mentioned "on
> demand" scenario. The only necessary change would be to call
> x25_terminate_link() on an interface after clearing the last X.25
> session.
>
> > On NETDEV_GOING_DOWN, we can also check the carrier status first and
> > if it is down, we don't need to call lapb_disconnect_request.
>
> This is not necessary because lapb_disconnect_request() checks the
> current state. And if the carrier is DOWN then the state should also be
> LAPB_STATE_0 and so lapb_disconnect_request() does nothing.

Yes, I understand. I just thought adding this check might make the
code cleaner. But you are right.

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Martin Schiller]

On 2020-11-23 09:31, Xie He wrote:
> On Sun, Nov 22, 2020 at 10:55 PM Martin Schiller <ms@dev.tdt.de> wrote:
>> 
>> No, they aren't independent. The carrier can only be up if the device 
>> /
>> interface is UP. And as far as I can see a NETDEV_CHANGE event will 
>> also
>> only be generated on interfaces that are UP.
>> 
>> So you can be sure, that if there is a NETDEV_CHANGE event then the
>> device is UP.
> 
> OK. Thanks for your explanation!
> 
>> I removed the NETDEV_UP handling because I don't think it makes sense
>> to implicitly try to establish layer2 (LAPB) if there is no carrier.
> 
> As I understand, when the device goes up, the carrier can be either
> down or up. Right?
> 
> If this is true, when a device goes up and the carrier then goes up
> after that, L2 will automatically connect, but if a device goes up and
> the carrier is already up, L2 will not automatically connect. I think
> it might be better to eliminate this difference in handling. It might
> be better to make it automatically connect in both situations, or in
> neither situations.

AFAIK the carrier can't be up before the device is up. Therefore, there
will be a NETDEV_CHANGE event after the NETDEV_UP event.

This is what I can see in my tests (with the HDLC interface).

Is the behaviour different for e.g. lapbether?

> 
> If you want to go with the second way (auto connect in neither
> situations), the next (3rd) patch of this series might be also not
> needed.
> 
> I just want to make the behavior of LAPB more consistent. I think we
> should either make LAPB auto-connect in all situations, or make LAPB
> wait for L3's instruction to connect in all situations.
> 
>> And with the first X.25 connection request on that interface, it will
>> be established anyway by x25_transmit_link().
>> 
>> I've tested it here with an HDLC WAN Adapter and it works as expected.
>> 
>> These are also the ideal conditions for the already mentioned "on
>> demand" scenario. The only necessary change would be to call
>> x25_terminate_link() on an interface after clearing the last X.25
>> session.
>> 
>> > On NETDEV_GOING_DOWN, we can also check the carrier status first and
>> > if it is down, we don't need to call lapb_disconnect_request.
>> 
>> This is not necessary because lapb_disconnect_request() checks the
>> current state. And if the carrier is DOWN then the state should also 
>> be
>> LAPB_STATE_0 and so lapb_disconnect_request() does nothing.
> 
> Yes, I understand. I just thought adding this check might make the
> code cleaner. But you are right.

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Xie He]

On Mon, Nov 23, 2020 at 1:00 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> AFAIK the carrier can't be up before the device is up. Therefore, there
> will be a NETDEV_CHANGE event after the NETDEV_UP event.
>
> This is what I can see in my tests (with the HDLC interface).
>
> Is the behaviour different for e.g. lapbether?

Some drivers don't support carrier status and will never change it.
Their carrier status will always be UP. There will not be a
NETDEV_CHANGE event.

lapbether doesn't change carrier status. I also have my own virtual
HDLC WAN driver (for testing) which also doesn't change carrier
status.

I just tested with lapbether. When I bring up the interface, there
will only be NETDEV_PRE_UP and then NETDEV_UP. There will not be
NETDEV_CHANGE. The carrier status is alway UP.

I haven't tested whether a device can receive NETDEV_CHANGE when it is
down. It's possible for a device driver to call netif_carrier_on when
the interface is down. Do you know what will happen if a device driver
calls netif_carrier_on when the interface is down?

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Xie He]

On Mon, Nov 23, 2020 at 1:36 AM Xie He <xie.he.0141@gmail.com> wrote:
>
> Some drivers don't support carrier status and will never change it.
> Their carrier status will always be UP. There will not be a
> NETDEV_CHANGE event.
>
> lapbether doesn't change carrier status. I also have my own virtual
> HDLC WAN driver (for testing) which also doesn't change carrier
> status.
>
> I just tested with lapbether. When I bring up the interface, there
> will only be NETDEV_PRE_UP and then NETDEV_UP. There will not be
> NETDEV_CHANGE. The carrier status is alway UP.
>
> I haven't tested whether a device can receive NETDEV_CHANGE when it is
> down. It's possible for a device driver to call netif_carrier_on when
> the interface is down. Do you know what will happen if a device driver
> calls netif_carrier_on when the interface is down?

I just did a test on lapbether and saw there would be no NETDEV_CHANGE
event when the netif is down, even if netif_carrier_on/off is called.
So we can rest assured of this part.

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Martin Schiller]

On 2020-11-23 11:08, Xie He wrote:
> On Mon, Nov 23, 2020 at 1:36 AM Xie He <xie.he.0141@gmail.com> wrote:
>> 
>> Some drivers don't support carrier status and will never change it.
>> Their carrier status will always be UP. There will not be a
>> NETDEV_CHANGE event.

Well, one could argue that we would have to repair these drivers, but I
don't think that will get us anywhere.

 From this point of view it will be the best to handle the NETDEV_UP in
the lapb event handler and establish the link analog to the
NETDEV_CHANGE event if the carrier is UP.

>> 
>> lapbether doesn't change carrier status. I also have my own virtual
>> HDLC WAN driver (for testing) which also doesn't change carrier
>> status.
>> 
>> I just tested with lapbether. When I bring up the interface, there
>> will only be NETDEV_PRE_UP and then NETDEV_UP. There will not be
>> NETDEV_CHANGE. The carrier status is alway UP.
>> 
>> I haven't tested whether a device can receive NETDEV_CHANGE when it is
>> down. It's possible for a device driver to call netif_carrier_on when
>> the interface is down. Do you know what will happen if a device driver
>> calls netif_carrier_on when the interface is down?
> 
> I just did a test on lapbether and saw there would be no NETDEV_CHANGE
> event when the netif is down, even if netif_carrier_on/off is called.
> So we can rest assured of this part.

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Xie He]

On Mon, Nov 23, 2020 at 2:38 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> Well, one could argue that we would have to repair these drivers, but I
> don't think that will get us anywhere.

Yeah... One problem I see with the Linux project is the lack of
docs/specs. Often we don't know what is right and what is wrong.

>  From this point of view it will be the best to handle the NETDEV_UP in
> the lapb event handler and establish the link analog to the
> NETDEV_CHANGE event if the carrier is UP.

Thanks! This way we can make sure LAPB would automatically connect in
all situations.

Since we'll have a netif_carrier_ok check in NETDEV_UP handing, it
might make the code look prettier to also have a netif_carrier_ok
check in NETDEV_GOING_DOWN handing (for symmetry). Just a suggestion.
You can do whatever looks good to you :)

Thanks!

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Jakub Kicinski]

On Mon, 23 Nov 2020 03:17:54 -0800 Xie He wrote:
> On Mon, Nov 23, 2020 at 2:38 AM Martin Schiller <ms@dev.tdt.de> wrote:
> > Well, one could argue that we would have to repair these drivers, but I
> > don't think that will get us anywhere.  
> 
> Yeah... One problem I see with the Linux project is the lack of
> docs/specs. Often we don't know what is right and what is wrong.

More of a historic thing than a requirement AFAIK. Some software
devices, e.g. loopback will not generate carrier events. But in this
case looks like all the devices Martin wants to handle are lapb.

> >  From this point of view it will be the best to handle the NETDEV_UP in
> > the lapb event handler and establish the link analog to the
> > NETDEV_CHANGE event if the carrier is UP.  
> 
> Thanks! This way we can make sure LAPB would automatically connect in
> all situations.
> 
> Since we'll have a netif_carrier_ok check in NETDEV_UP handing, it
> might make the code look prettier to also have a netif_carrier_ok
> check in NETDEV_GOING_DOWN handing (for symmetry). Just a suggestion.
> You can do whatever looks good to you :)

Xie other than this the patches look good to you?

Martin should I expect a respin to follow Xie's suggestion 
or should I apply v4?

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Xie He]

On Mon, Nov 23, 2020 at 11:36 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> > >  From this point of view it will be the best to handle the NETDEV_UP in
> > > the lapb event handler and establish the link analog to the
> > > NETDEV_CHANGE event if the carrier is UP.
> >
> > Thanks! This way we can make sure LAPB would automatically connect in
> > all situations.
> >
> > Since we'll have a netif_carrier_ok check in NETDEV_UP handing, it
> > might make the code look prettier to also have a netif_carrier_ok
> > check in NETDEV_GOING_DOWN handing (for symmetry). Just a suggestion.
> > You can do whatever looks good to you :)
>
> Xie other than this the patches look good to you?
>
> Martin should I expect a respin to follow Xie's suggestion
> or should I apply v4?

There should be a respin because we need to handle the NETDEV_UP
event. The lapbether driver (and possibly some HDLC WAN drivers)
doesn't generate carrier events so we need to do auto-connect in the
NETDEV_UP event.

Re: [PATCH net-next v4 2/5] net/lapb: support netdev events

[Martin Schiller]

On 2020-11-23 23:09, Xie He wrote:
> On Mon, Nov 23, 2020 at 11:36 AM Jakub Kicinski <kuba@kernel.org> 
> wrote:
>> 
>> > >  From this point of view it will be the best to handle the NETDEV_UP in
>> > > the lapb event handler and establish the link analog to the
>> > > NETDEV_CHANGE event if the carrier is UP.
>> >
>> > Thanks! This way we can make sure LAPB would automatically connect in
>> > all situations.
>> >
>> > Since we'll have a netif_carrier_ok check in NETDEV_UP handing, it
>> > might make the code look prettier to also have a netif_carrier_ok
>> > check in NETDEV_GOING_DOWN handing (for symmetry). Just a suggestion.
>> > You can do whatever looks good to you :)
>> 
>> Xie other than this the patches look good to you?
>> 
>> Martin should I expect a respin to follow Xie's suggestion
>> or should I apply v4?
> 
> There should be a respin because we need to handle the NETDEV_UP
> event. The lapbether driver (and possibly some HDLC WAN drivers)
> doesn't generate carrier events so we need to do auto-connect in the
> NETDEV_UP event.

I'll send a v5 with the appropriate change.