In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer", "lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. In lapb_unregister, change "lapb_stop_t1timer" and "lapb_stop_t2timer"
to "del_timer_sync" to make sure all running timers have exited.
4. In lapb_device_event, replace the "lapb_disconnect_request" call with
the content of "lapb_disconnect_request", to avoid trying to hold the
lock twice. When I do this, I removed "lapb_start_t1timer" because I
don't think it's necessary to start the timer when "NETDEV_GOING_DOWN".
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller <[email protected]>
Signed-off-by: Xie He <[email protected]>
---
include/net/lapb.h | 2 ++
net/lapb/lapb_iface.c | 56 +++++++++++++++++++++++++++++++++++++++----
net/lapb/lapb_timer.c | 30 +++++++++++++++++++----
3 files changed, 80 insertions(+), 8 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ bool t1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..45f332607685 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(&lapb->t1timer, NULL, 0);
timer_setup(&lapb->t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode = LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(&lapb->lock);
refcount_set(&lapb->refcnt, 1);
out:
return lapb;
@@ -178,8 +182,8 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
- lapb_stop_t1timer(lapb);
- lapb_stop_t2timer(lapb);
+ del_timer_sync(&lapb->t1timer);
+ del_timer_sync(&lapb->t2timer);
lapb_clear_queues(lapb);
@@ -201,6 +205,8 @@ int lapb_getparms(struct net_device *dev, struct lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(&lapb->lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +225,7 @@ int lapb_getparms(struct net_device *dev, struct lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(&lapb->lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +241,8 @@ int lapb_setparms(struct net_device *dev, struct lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(&lapb->lock);
+
rc = LAPB_INVALUE;
if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
goto out_put;
@@ -256,6 +265,7 @@ int lapb_setparms(struct net_device *dev, struct lapb_parms_struct *parms)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(&lapb->lock);
lapb_put(lapb);
out:
return rc;
@@ -270,6 +280,8 @@ int lapb_connect_request(struct net_device *dev)
if (!lapb)
goto out;
+ spin_lock_bh(&lapb->lock);
+
rc = LAPB_OK;
if (lapb->state == LAPB_STATE_1)
goto out_put;
@@ -285,6 +297,7 @@ int lapb_connect_request(struct net_device *dev)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(&lapb->lock);
lapb_put(lapb);
out:
return rc;
@@ -299,6 +312,8 @@ int lapb_disconnect_request(struct net_device *dev)
if (!lapb)
goto out;
+ spin_lock_bh(&lapb->lock);
+
switch (lapb->state) {
case LAPB_STATE_0:
rc = LAPB_NOTCONNECTED;
@@ -330,6 +345,7 @@ int lapb_disconnect_request(struct net_device *dev)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(&lapb->lock);
lapb_put(lapb);
out:
return rc;
@@ -344,6 +360,8 @@ int lapb_data_request(struct net_device *dev, struct sk_buff *skb)
if (!lapb)
goto out;
+ spin_lock_bh(&lapb->lock);
+
rc = LAPB_NOTCONNECTED;
if (lapb->state != LAPB_STATE_3 && lapb->state != LAPB_STATE_4)
goto out_put;
@@ -352,6 +370,7 @@ int lapb_data_request(struct net_device *dev, struct sk_buff *skb)
lapb_kick(lapb);
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(&lapb->lock);
lapb_put(lapb);
out:
return rc;
@@ -364,7 +383,9 @@ int lapb_data_received(struct net_device *dev, struct sk_buff *skb)
int rc = LAPB_BADTOKEN;
if (lapb) {
+ spin_lock_bh(&lapb->lock);
lapb_data_input(lapb, skb);
+ spin_unlock_bh(&lapb->lock);
lapb_put(lapb);
rc = LAPB_OK;
}
@@ -435,6 +456,8 @@ static int lapb_device_event(struct notifier_block *this, unsigned long event,
if (!lapb)
return NOTIFY_DONE;
+ spin_lock_bh(&lapb->lock);
+
switch (event) {
case NETDEV_UP:
lapb_dbg(0, "(%p) Interface up: %s\n", dev, dev->name);
@@ -453,8 +476,32 @@ static int lapb_device_event(struct notifier_block *this, unsigned long event,
}
break;
case NETDEV_GOING_DOWN:
- if (netif_carrier_ok(dev))
- lapb_disconnect_request(dev);
+ switch (lapb->state) {
+ case LAPB_STATE_0:
+ break;
+
+ case LAPB_STATE_1:
+ lapb_dbg(1, "(%p) S1 TX DISC(1)\n", lapb->dev);
+ lapb_dbg(0, "(%p) S1 -> S0\n", lapb->dev);
+ lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON,
+ LAPB_COMMAND);
+ lapb->state = LAPB_STATE_0;
+ break;
+
+ case LAPB_STATE_2:
+ break;
+
+ default:
+ lapb_clear_queues(lapb);
+ lapb->n2count = 0;
+ lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON,
+ LAPB_COMMAND);
+ lapb_stop_t2timer(lapb);
+ lapb->state = LAPB_STATE_2;
+ lapb_dbg(1, "(%p) S3 DISC(1)\n", lapb->dev);
+ lapb_dbg(0, "(%p) S3 -> S2\n", lapb->dev);
+ }
+
break;
case NETDEV_DOWN:
lapb_dbg(0, "(%p) Interface down: %s\n", dev, dev->name);
@@ -489,6 +536,7 @@ static int lapb_device_event(struct notifier_block *this, unsigned long event,
break;
}
+ spin_unlock_bh(&lapb->lock);
lapb_put(lapb);
return NOTIFY_DONE;
}
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index baa247fe4ed0..0230b272b7d1 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -40,6 +40,7 @@ void lapb_start_t1timer(struct lapb_cb *lapb)
lapb->t1timer.function = lapb_t1timer_expiry;
lapb->t1timer.expires = jiffies + lapb->t1;
+ lapb->t1timer_stop = false;
add_timer(&lapb->t1timer);
}
@@ -50,16 +51,19 @@ void lapb_start_t2timer(struct lapb_cb *lapb)
lapb->t2timer.function = lapb_t2timer_expiry;
lapb->t2timer.expires = jiffies + lapb->t2;
+ lapb->t2timer_stop = false;
add_timer(&lapb->t2timer);
}
void lapb_stop_t1timer(struct lapb_cb *lapb)
{
+ lapb->t1timer_stop = true;
del_timer(&lapb->t1timer);
}
void lapb_stop_t2timer(struct lapb_cb *lapb)
{
+ lapb->t2timer_stop = true;
del_timer(&lapb->t2timer);
}
@@ -72,16 +76,31 @@ static void lapb_t2timer_expiry(struct timer_list *t)
{
struct lapb_cb *lapb = from_timer(lapb, t, t2timer);
+ spin_lock_bh(&lapb->lock);
+ if (timer_pending(&lapb->t2timer)) /* A new timer has been set up */
+ goto out;
+ if (lapb->t2timer_stop) /* The timer has been stopped */
+ goto out;
+
if (lapb->condition & LAPB_ACK_PENDING_CONDITION) {
lapb->condition &= ~LAPB_ACK_PENDING_CONDITION;
lapb_timeout_response(lapb);
}
+
+out:
+ spin_unlock_bh(&lapb->lock);
}
static void lapb_t1timer_expiry(struct timer_list *t)
{
struct lapb_cb *lapb = from_timer(lapb, t, t1timer);
+ spin_lock_bh(&lapb->lock);
+ if (timer_pending(&lapb->t1timer)) /* A new timer has been set up */
+ goto out;
+ if (lapb->t1timer_stop) /* The timer has been stopped */
+ goto out;
+
switch (lapb->state) {
/*
@@ -108,7 +127,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
lapb->state = LAPB_STATE_0;
lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
lapb_dbg(0, "(%p) S1 -> S0\n", lapb->dev);
- return;
+ goto out;
} else {
lapb->n2count++;
if (lapb->mode & LAPB_EXTENDED) {
@@ -132,7 +151,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
lapb->state = LAPB_STATE_0;
lapb_disconnect_confirmation(lapb, LAPB_TIMEDOUT);
lapb_dbg(0, "(%p) S2 -> S0\n", lapb->dev);
- return;
+ goto out;
} else {
lapb->n2count++;
lapb_dbg(1, "(%p) S2 TX DISC(1)\n", lapb->dev);
@@ -150,7 +169,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
lapb_stop_t2timer(lapb);
lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
lapb_dbg(0, "(%p) S3 -> S0\n", lapb->dev);
- return;
+ goto out;
} else {
lapb->n2count++;
lapb_requeue_frames(lapb);
@@ -167,7 +186,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
lapb->state = LAPB_STATE_0;
lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
lapb_dbg(0, "(%p) S4 -> S0\n", lapb->dev);
- return;
+ goto out;
} else {
lapb->n2count++;
lapb_transmit_frmr(lapb);
@@ -176,4 +195,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
}
lapb_start_t1timer(lapb);
+
+out:
+ spin_unlock_bh(&lapb->lock);
}
--
2.27.0
On 2021-01-17 23:47, Xie He wrote:
> In the lapb module, the timers may run concurrently with other code in
> this module, and there is currently no locking to prevent the code from
> racing on "struct lapb_cb". This patch adds locking to prevent racing.
>
> 1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
> "spin_unlock_bh" to APIs, timer functions and notifier functions.
>
> 2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
> able to ask running timers to abort; Modify "lapb_stop_t1timer" and
> "lapb_stop_t2timer" to make them able to abort running timers;
> Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
> abort after they are stopped by "lapb_stop_t1timer",
> "lapb_stop_t2timer",
> and "lapb_start_t1timer", "lapb_start_t2timer".
>
> 3. In lapb_unregister, change "lapb_stop_t1timer" and
> "lapb_stop_t2timer"
> to "del_timer_sync" to make sure all running timers have exited.
>
> 4. In lapb_device_event, replace the "lapb_disconnect_request" call
> with
> the content of "lapb_disconnect_request", to avoid trying to hold the
> lock twice. When I do this, I removed "lapb_start_t1timer" because I
> don't think it's necessary to start the timer when "NETDEV_GOING_DOWN".
I don't like the code redundancy this creates. Maybe you should move the
actual functionality from lapb_disconnect_request() to a
__lapb_disconnect_request(), and in lapb_disconnect_request() call this
function including locking around it and also in lapb_device_event
(without locking).
Calling lapb_start_t1timer() on a "NETDEV_GOING_DOWN" event does not
hurt and is correct from a protocol flow point of view after sending
the DISC.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: Martin Schiller <[email protected]>
> Signed-off-by: Xie He <[email protected]>
> ---
> include/net/lapb.h | 2 ++
> net/lapb/lapb_iface.c | 56 +++++++++++++++++++++++++++++++++++++++----
> net/lapb/lapb_timer.c | 30 +++++++++++++++++++----
> 3 files changed, 80 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/lapb.h b/include/net/lapb.h
> index ccc3d1f020b0..eee73442a1ba 100644
> --- a/include/net/lapb.h
> +++ b/include/net/lapb.h
> @@ -92,6 +92,7 @@ struct lapb_cb {
> unsigned short n2, n2count;
> unsigned short t1, t2;
> struct timer_list t1timer, t2timer;
> + bool t1timer_stop, t2timer_stop;
>
> /* Internal control information */
> struct sk_buff_head write_queue;
> @@ -103,6 +104,7 @@ struct lapb_cb {
> struct lapb_frame frmr_data;
> unsigned char frmr_type;
>
> + spinlock_t lock;
> refcount_t refcnt;
> };
>
> diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
> index 40961889e9c0..45f332607685 100644
> --- a/net/lapb/lapb_iface.c
> +++ b/net/lapb/lapb_iface.c
> @@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
>
> timer_setup(&lapb->t1timer, NULL, 0);
> timer_setup(&lapb->t2timer, NULL, 0);
> + lapb->t1timer_stop = true;
> + lapb->t2timer_stop = true;
>
> lapb->t1 = LAPB_DEFAULT_T1;
> lapb->t2 = LAPB_DEFAULT_T2;
> @@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
> lapb->mode = LAPB_DEFAULT_MODE;
> lapb->window = LAPB_DEFAULT_WINDOW;
> lapb->state = LAPB_STATE_0;
> +
> + spin_lock_init(&lapb->lock);
> refcount_set(&lapb->refcnt, 1);
> out:
> return lapb;
> @@ -178,8 +182,8 @@ int lapb_unregister(struct net_device *dev)
> goto out;
> lapb_put(lapb);
>
> - lapb_stop_t1timer(lapb);
> - lapb_stop_t2timer(lapb);
> + del_timer_sync(&lapb->t1timer);
> + del_timer_sync(&lapb->t2timer);
>
> lapb_clear_queues(lapb);
>
> @@ -201,6 +205,8 @@ int lapb_getparms(struct net_device *dev, struct
> lapb_parms_struct *parms)
> if (!lapb)
> goto out;
>
> + spin_lock_bh(&lapb->lock);
> +
> parms->t1 = lapb->t1 / HZ;
> parms->t2 = lapb->t2 / HZ;
> parms->n2 = lapb->n2;
> @@ -219,6 +225,7 @@ int lapb_getparms(struct net_device *dev, struct
> lapb_parms_struct *parms)
> else
> parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
>
> + spin_unlock_bh(&lapb->lock);
> lapb_put(lapb);
> rc = LAPB_OK;
> out:
> @@ -234,6 +241,8 @@ int lapb_setparms(struct net_device *dev, struct
> lapb_parms_struct *parms)
> if (!lapb)
> goto out;
>
> + spin_lock_bh(&lapb->lock);
> +
> rc = LAPB_INVALUE;
> if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
> goto out_put;
> @@ -256,6 +265,7 @@ int lapb_setparms(struct net_device *dev, struct
> lapb_parms_struct *parms)
>
> rc = LAPB_OK;
> out_put:
> + spin_unlock_bh(&lapb->lock);
> lapb_put(lapb);
> out:
> return rc;
> @@ -270,6 +280,8 @@ int lapb_connect_request(struct net_device *dev)
> if (!lapb)
> goto out;
>
> + spin_lock_bh(&lapb->lock);
> +
> rc = LAPB_OK;
> if (lapb->state == LAPB_STATE_1)
> goto out_put;
> @@ -285,6 +297,7 @@ int lapb_connect_request(struct net_device *dev)
>
> rc = LAPB_OK;
> out_put:
> + spin_unlock_bh(&lapb->lock);
> lapb_put(lapb);
> out:
> return rc;
> @@ -299,6 +312,8 @@ int lapb_disconnect_request(struct net_device *dev)
> if (!lapb)
> goto out;
>
> + spin_lock_bh(&lapb->lock);
> +
> switch (lapb->state) {
> case LAPB_STATE_0:
> rc = LAPB_NOTCONNECTED;
> @@ -330,6 +345,7 @@ int lapb_disconnect_request(struct net_device *dev)
>
> rc = LAPB_OK;
> out_put:
> + spin_unlock_bh(&lapb->lock);
> lapb_put(lapb);
> out:
> return rc;
> @@ -344,6 +360,8 @@ int lapb_data_request(struct net_device *dev,
> struct sk_buff *skb)
> if (!lapb)
> goto out;
>
> + spin_lock_bh(&lapb->lock);
> +
> rc = LAPB_NOTCONNECTED;
> if (lapb->state != LAPB_STATE_3 && lapb->state != LAPB_STATE_4)
> goto out_put;
> @@ -352,6 +370,7 @@ int lapb_data_request(struct net_device *dev,
> struct sk_buff *skb)
> lapb_kick(lapb);
> rc = LAPB_OK;
> out_put:
> + spin_unlock_bh(&lapb->lock);
> lapb_put(lapb);
> out:
> return rc;
> @@ -364,7 +383,9 @@ int lapb_data_received(struct net_device *dev,
> struct sk_buff *skb)
> int rc = LAPB_BADTOKEN;
>
> if (lapb) {
> + spin_lock_bh(&lapb->lock);
> lapb_data_input(lapb, skb);
> + spin_unlock_bh(&lapb->lock);
> lapb_put(lapb);
> rc = LAPB_OK;
> }
> @@ -435,6 +456,8 @@ static int lapb_device_event(struct notifier_block
> *this, unsigned long event,
> if (!lapb)
> return NOTIFY_DONE;
>
> + spin_lock_bh(&lapb->lock);
> +
> switch (event) {
> case NETDEV_UP:
> lapb_dbg(0, "(%p) Interface up: %s\n", dev, dev->name);
> @@ -453,8 +476,32 @@ static int lapb_device_event(struct
> notifier_block *this, unsigned long event,
> }
> break;
> case NETDEV_GOING_DOWN:
> - if (netif_carrier_ok(dev))
> - lapb_disconnect_request(dev);
> + switch (lapb->state) {
> + case LAPB_STATE_0:
> + break;
> +
> + case LAPB_STATE_1:
> + lapb_dbg(1, "(%p) S1 TX DISC(1)\n", lapb->dev);
> + lapb_dbg(0, "(%p) S1 -> S0\n", lapb->dev);
> + lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON,
> + LAPB_COMMAND);
> + lapb->state = LAPB_STATE_0;
> + break;
> +
> + case LAPB_STATE_2:
> + break;
> +
> + default:
> + lapb_clear_queues(lapb);
> + lapb->n2count = 0;
> + lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON,
> + LAPB_COMMAND);
> + lapb_stop_t2timer(lapb);
> + lapb->state = LAPB_STATE_2;
> + lapb_dbg(1, "(%p) S3 DISC(1)\n", lapb->dev);
> + lapb_dbg(0, "(%p) S3 -> S2\n", lapb->dev);
> + }
> +
> break;
> case NETDEV_DOWN:
> lapb_dbg(0, "(%p) Interface down: %s\n", dev, dev->name);
> @@ -489,6 +536,7 @@ static int lapb_device_event(struct notifier_block
> *this, unsigned long event,
> break;
> }
>
> + spin_unlock_bh(&lapb->lock);
> lapb_put(lapb);
> return NOTIFY_DONE;
> }
> diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
> index baa247fe4ed0..0230b272b7d1 100644
> --- a/net/lapb/lapb_timer.c
> +++ b/net/lapb/lapb_timer.c
> @@ -40,6 +40,7 @@ void lapb_start_t1timer(struct lapb_cb *lapb)
> lapb->t1timer.function = lapb_t1timer_expiry;
> lapb->t1timer.expires = jiffies + lapb->t1;
>
> + lapb->t1timer_stop = false;
> add_timer(&lapb->t1timer);
> }
>
> @@ -50,16 +51,19 @@ void lapb_start_t2timer(struct lapb_cb *lapb)
> lapb->t2timer.function = lapb_t2timer_expiry;
> lapb->t2timer.expires = jiffies + lapb->t2;
>
> + lapb->t2timer_stop = false;
> add_timer(&lapb->t2timer);
> }
>
> void lapb_stop_t1timer(struct lapb_cb *lapb)
> {
> + lapb->t1timer_stop = true;
> del_timer(&lapb->t1timer);
> }
>
> void lapb_stop_t2timer(struct lapb_cb *lapb)
> {
> + lapb->t2timer_stop = true;
> del_timer(&lapb->t2timer);
> }
>
> @@ -72,16 +76,31 @@ static void lapb_t2timer_expiry(struct timer_list
> *t)
> {
> struct lapb_cb *lapb = from_timer(lapb, t, t2timer);
>
> + spin_lock_bh(&lapb->lock);
> + if (timer_pending(&lapb->t2timer)) /* A new timer has been set up */
> + goto out;
> + if (lapb->t2timer_stop) /* The timer has been stopped */
> + goto out;
> +
> if (lapb->condition & LAPB_ACK_PENDING_CONDITION) {
> lapb->condition &= ~LAPB_ACK_PENDING_CONDITION;
> lapb_timeout_response(lapb);
> }
> +
> +out:
> + spin_unlock_bh(&lapb->lock);
> }
>
> static void lapb_t1timer_expiry(struct timer_list *t)
> {
> struct lapb_cb *lapb = from_timer(lapb, t, t1timer);
>
> + spin_lock_bh(&lapb->lock);
> + if (timer_pending(&lapb->t1timer)) /* A new timer has been set up */
> + goto out;
> + if (lapb->t1timer_stop) /* The timer has been stopped */
> + goto out;
> +
> switch (lapb->state) {
>
> /*
> @@ -108,7 +127,7 @@ static void lapb_t1timer_expiry(struct timer_list
> *t)
> lapb->state = LAPB_STATE_0;
> lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
> lapb_dbg(0, "(%p) S1 -> S0\n", lapb->dev);
> - return;
> + goto out;
> } else {
> lapb->n2count++;
> if (lapb->mode & LAPB_EXTENDED) {
> @@ -132,7 +151,7 @@ static void lapb_t1timer_expiry(struct timer_list
> *t)
> lapb->state = LAPB_STATE_0;
> lapb_disconnect_confirmation(lapb, LAPB_TIMEDOUT);
> lapb_dbg(0, "(%p) S2 -> S0\n", lapb->dev);
> - return;
> + goto out;
> } else {
> lapb->n2count++;
> lapb_dbg(1, "(%p) S2 TX DISC(1)\n", lapb->dev);
> @@ -150,7 +169,7 @@ static void lapb_t1timer_expiry(struct timer_list
> *t)
> lapb_stop_t2timer(lapb);
> lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
> lapb_dbg(0, "(%p) S3 -> S0\n", lapb->dev);
> - return;
> + goto out;
> } else {
> lapb->n2count++;
> lapb_requeue_frames(lapb);
> @@ -167,7 +186,7 @@ static void lapb_t1timer_expiry(struct timer_list
> *t)
> lapb->state = LAPB_STATE_0;
> lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
> lapb_dbg(0, "(%p) S4 -> S0\n", lapb->dev);
> - return;
> + goto out;
> } else {
> lapb->n2count++;
> lapb_transmit_frmr(lapb);
> @@ -176,4 +195,7 @@ static void lapb_t1timer_expiry(struct timer_list
> *t)
> }
>
> lapb_start_t1timer(lapb);
> +
> +out:
> + spin_unlock_bh(&lapb->lock);
> }
On Tue, Jan 19, 2021 at 3:34 AM Martin Schiller <[email protected]> wrote:
>
> > 4. In lapb_device_event, replace the "lapb_disconnect_request" call
> > with
> > the content of "lapb_disconnect_request", to avoid trying to hold the
> > lock twice. When I do this, I removed "lapb_start_t1timer" because I
> > don't think it's necessary to start the timer when "NETDEV_GOING_DOWN".
>
> I don't like the code redundancy this creates. Maybe you should move the
> actual functionality from lapb_disconnect_request() to a
> __lapb_disconnect_request(), and in lapb_disconnect_request() call this
> function including locking around it and also in lapb_device_event
> (without locking).
>
> Calling lapb_start_t1timer() on a "NETDEV_GOING_DOWN" event does not
> hurt and is correct from a protocol flow point of view after sending
> the DISC.
Thanks! I created a new __lapb_disconnect_request function and the
code indeed looked cleaner. I'll send a new version.