LiS ldl driver with 2.6 kernels
Hai Zaar <[email protected]>
| Newsgroups | gmane.linux.kernel.streams |
|---|---|
| Message-ID | <[email protected]> |
Hello!
I'm using the ldl driver module in LiS-2.18 on 2.6.7 Linux kernel
and I've had various warnings in the kernel log due to improper
locking in the driver. The problems are easiliy reproducible using
ldltest that is supplied with LiS. I have fixed all of those problems,
but due to my limited undestanding of both LiS and kernel locking and
internals, I'd be glad if someone who actually understands both
reviews my patches.
The locking fixes patch, fixes the following problems:
1) Calling sap_create() under lock. The problem is that sap_create()
might invoke dev_add_pack() that can sleep, since kernel 2.5.70. I
choose to remove the call to dev_add_pack() from sap_create() and make
sap_create() return pointer to struct packet_type to the caller, that
will invoke dev_add_pack() after unlocking. It seem that attaching
packet device later cannot do any harm.
2) Calling sleeping dev_remove_pack() in sap_destory(), while
sap_destroy() holds lock. The fix is to delay calling
dev_remove_pack() until unlock is done. During the call sap_destroy()
dl->state is either DL_UNBIND_PENDING or DL_SUBS_UNBIND_PENDING so
delaying unregistration seems to be safe to me.
3) Calling sap_destroy_all() while holding lock. The change to to make
dl->state to be DL_UNBIND_PENDING, unlock, do the call and re-lock.
I'm not completely sure if it is safe, I was just blindnly copying the
code that already existed for one of the invocations of
sap_destroy_all().
4) Doing spin_unlock_bh() while holding irq-disabling lock. I choose
to change the locking to be irq-safe, that does not have that problem.
Due to my limited understaing of Linux kernel locking rules, I'm not
completely sure it is correct. I thing that disabling local irqs
guarantees that no bh processing is done, but I might be wrong.
Another small off-subject patch is attached, it fixes problematic
uses of HZ. We're running here wigth HZ==2000, thus 1000/HZ is always
zero (each timer tick is less than 1msec apart from the previous one).
I've converted all users of HZ to jiffie/ms/jiffie conversion routines
of the kernell
Regards,
Zaar
P.S. It's kind of strange that nobody noticed those problems because
they appeared during 2.5.70, when the networking code was coverted
from using brlocks to RCU. It was the change that made dev_add_pack()
and dev_remove_pack() to be sleeping functions. Maybe the problem was
raised on this list, but the web archive of it is unavailable at the
present.
LiS-2.18.fix_ldl_locking.patch
(application/octet-stream, 5.5 KB)
diff -ur LiS-2.18.orig/drivers/str/linux/ldl.c LiS-2.18/drivers/str/linux/ldl.c
--- LiS-2.18.orig/drivers/str/linux/ldl.c 2004-09-13 18:12:45.000000000 +0300
+++ LiS-2.18/drivers/str/linux/ldl.c 2005-03-31 14:35:50.440813077 +0200
@@ -122,8 +122,8 @@
#define ldldev net_device
#define driver_started(dev) 1
-#define START_BH_ATOMIC(dev) spin_lock_bh(&(dev)->queue_lock)
-#define END_BH_ATOMIC(dev) spin_unlock_bh(&(dev)->queue_lock)
+#define START_BH_ATOMIC(dev, flags) spin_lock_irqsave(&(dev)->queue_lock, flags)
+#define END_BH_ATOMIC(dev,flags) spin_unlock_irqrestore(&(dev)->queue_lock, flags)
#if defined(ARPHRD_IEEE802_TR)
#define IS_ARPHRD_IEEE802_TR(dev) (dev)->type == ARPHRD_IEEE802_TR
@@ -340,7 +340,7 @@
ldl_gstats_ioctl_t ldl_gstats ;
#define ginc(field) lis_atomic_inc(&ldl_gstats.field)
-STATIC unsigned long ldl_debug_mask ;
+STATIC unsigned long ldl_debug_mask;
STATIC struct pt *first_pt;
STATIC lis_spin_lock_t first_pt_lock;
@@ -569,11 +569,12 @@
/*
* sap_create - create and add another packet type (sap) to a device.
*
- * Returns 0 on success, -1 on failure.
+ * Returns 0 on success, -1 on failure. If the returned pt is not NULL,
+ * than dev_add_pack(pt) must be called later, when interrupts are enabled
*
* Notice that sap_create is always called under SPLSTR()
*/
-STATIC int sap_create(struct dl *dl, sap_t dlsap, dl_ushort saptype)
+STATIC int sap_create(struct dl *dl, sap_t dlsap, dl_ushort saptype, struct packet_type **return_pt)
{
struct pt *pt, *npt;
struct sap *sap;
@@ -582,6 +583,7 @@
LISASSERT(dl->magic == DL_MAGIC);
LISASSERT(dl->ndev != NULL);
+ *return_pt = NULL;
saptype = htons(saptype);
if ((sap = ALLOC(sizeof *sap)) == NULL)
@@ -649,7 +651,8 @@
pt->pt.next = NULL;
#endif
lis_spin_unlock(&first_pt_lock);
- dev_add_pack(&pt->pt);
+ /* dev_add_pack(&pt->pt); will be called later, when no locks are held */
+ *return_pt = &pt->pt;
} else {
/* Re-use of packet_type */
LISASSERT(pt->magic == PT_MAGIC);
@@ -676,6 +679,7 @@
int psw;
struct pt *pt, *opt;
struct sap **sapp_dl, **sapp_pt;
+ struct packet_type * to_remove = NULL;
LISASSERT(dl != NULL);
LISASSERT(dl->magic == DL_MAGIC);
@@ -703,7 +707,8 @@
}
}
lis_spin_unlock(&first_pt_lock);
- dev_remove_pack(&pt->pt);
+ /* dev_remove_pack(&pt->pt); will be called later, when no locks are held */
+ to_remove = &pt->pt;
pt->magic = 0;
FREE(pt);
--pt_n_alloc;
@@ -742,6 +747,9 @@
* Emergency brake
*/
SPLX(psw);
+ if ( to_remove ) {
+ dev_remove_pack(to_remove);
+ }
return -1;
}
LISASSERT((*sapp_dl)->magic == SAP_MAGIC);
@@ -760,6 +768,9 @@
--sap_n_alloc;
SPLX(psw);
+ if ( to_remove ) {
+ dev_remove_pack(to_remove);
+ }
return 0;
}
@@ -859,7 +870,10 @@
* should come from this endpoint.
*/
if (dl->dlstate == DL_IDLE) {
+ dl->dlstate = DL_UNBIND_PENDING;
+ SPLX(psw);
sap_destroy_all(dl);
+ SPLSTR(psw);
dl->dlstate = DL_UNBOUND;
}
if (dl->dlstate == DL_UNBOUND) {
@@ -1020,9 +1034,10 @@
* clients of the driver, but that's just the
* way has to be. DMG 8/25/00
*/
- START_BH_ATOMIC(ndev->dev) ;
+ unsigned long flags;
+ START_BH_ATOMIC(ndev->dev, flags) ;
qdisc_reset(ndev->dev->qdisc) ;
- END_BH_ATOMIC(ndev->dev) ;
+ END_BH_ATOMIC(ndev->dev, flags) ;
ndev->dev = NULL;
}
ndev_free(ndev);
@@ -1209,7 +1224,8 @@
if (atomic_read(&ndev->wr_cur) <= ndev->wr_max) {
struct Qdisc *q;
- START_BH_ATOMIC(dev);
+ unsigned long flags;
+ START_BH_ATOMIC(dev, flags);
q = dev->qdisc;
LISASSERT(q != NULL);
if (q->enqueue) {
@@ -1217,11 +1233,11 @@
ret = q->enqueue(skb, q);
qdisc_wakeup(dev);
- END_BH_ATOMIC(dev);
+ END_BH_ATOMIC(dev. flags);
if (ret == 1)
return DONE;
} else {
- END_BH_ATOMIC(dev);
+ END_BH_ATOMIC(dev, flags);
if (dev_queue_xmit(skb) >= 0)
return DONE;
}
@@ -3352,6 +3368,7 @@
unsigned short saptype;
int psw;
int len;
+ struct packet_type *pt;
ginc(bind_req_cnt) ;
SPLSTR(psw);
@@ -3416,7 +3433,7 @@
return reply_error_ack(dl, mp, DL_BIND_REQ, DL_BADADDR, 0);
}
- if (sap_create(dl, dlsap, saptype) < 0) {
+ if (sap_create(dl, dlsap, saptype, &pt) < 0) {
SPLX(psw);
return reply_error_ack(dl, mp, DL_BIND_REQ, DL_SYSERR, ENOMEM);
}
@@ -3453,6 +3470,11 @@
dl->dlstate = DL_IDLE;
SPLX(psw);
+ if ( pt ) {
+ /* Can be called only when no locks are held */
+ dev_add_pack(pt);
+ }
+
if (ldl_debug_mask & LDL_DEBUG_BIND)
printk("ldl: ws_bind: "
"dl_sap=%x framing=%s pkt-type=%s\n",
@@ -3488,6 +3510,7 @@
dl->dlstate = DL_UNBIND_PENDING;
SPLX(psw);
+ /* Call only when no locks are held */
sap_destroy_all(dl);
if (putctl1(dl->rq->q_next, M_FLUSH, FLUSHRW) == 0)
@@ -3517,6 +3540,7 @@
dl_subs_bind_ack_t *ackp;
int len;
int psw;
+ struct packet_type *pt;
ginc(subs_bind_req_cnt) ;
SPLSTR(psw);
@@ -3625,12 +3649,17 @@
mp = bp;
}
- if (sap_create(dl, dlsap, saptype) < 0) {
+ if (sap_create(dl, dlsap, saptype, &pt) < 0) {
SPLX(psw);
return reply_error_ack(dl, mp, DL_SUBS_BIND_REQ, DL_TOOMANY, 0);
}
SPLX(psw);
+ /* Can be called only when no locks are held */
+ if ( pt ) {
+ dev_add_pack(pt);
+ }
+
mp->b_datap->db_type = M_PCPROTO;
ackp = (dl_subs_bind_ack_t *)mp->b_wptr;
ackp->dl_primitive = DL_SUBS_BIND_ACK;
@@ -4157,7 +4186,10 @@
}
if (dl->dlstate == DL_IDLE) {
+ dl->dlstate = DL_UNBIND_PENDING;
+ SPLX(psw);
sap_destroy_all(dl);
+ SPLSTR(psw);
dl->dlstate = DL_UNBOUND;
}
if (dl->dlstate != DL_UNATTACHED) {
LiS-2.18.fix_HZ_usage.patch
(application/octet-stream, 3.7 KB)
diff -ur LiS-2.18.orig/head/linux-mdep.c LiS-2.18/head/linux-mdep.c
--- LiS-2.18.orig/head/linux-mdep.c 2004-10-13 18:07:36.000000000 +0200
+++ LiS-2.18/head/linux-mdep.c 2005-03-31 15:08:55.679753385 +0200
@@ -808,7 +808,7 @@
************************************************************************/
long _RP lis_time_till(long target_time)
{
- return( target_time - jiffies*(1000/HZ) ) ;
+ return( target_time - jiffies_to_msecs(jiffies) ) ;
} /* lis_time_till */
@@ -823,7 +823,7 @@
************************************************************************/
long _RP lis_target_time(long milli_sec)
{
- return( jiffies*(1000/HZ) + milli_sec ) ;
+ return( jiffies_to_msecs(jiffies) + milli_sec ) ;
} /* lis_target_time */
@@ -837,7 +837,7 @@
************************************************************************/
long _RP lis_milli_to_ticks(long milli_sec)
{
- return(milli_sec/(1000/HZ)) ;
+ return msecs_to_jiffies(milli_sec) ;
}
diff -ur LiS-2.18.orig/head/poll.c LiS-2.18/head/poll.c
--- LiS-2.18.orig/head/poll.c 2004-09-07 19:11:22.000000000 +0300
+++ LiS-2.18/head/poll.c 2005-03-31 14:52:55.838753847 +0200
@@ -180,7 +180,6 @@
long target_time ; /* in ms */
int timer_id = 0 ;
long time_interval = time_out ; /* in ms */
- long ms_per_tick = 1000/HZ ;
long ticks ; /* in system ticks */
struct pollfd *pfds;
struct pollfd *pfd_ptr ;
@@ -204,8 +203,7 @@
/*
* Round time interval up to a multiple of system ticks.
*/
- ticks = (time_interval + ms_per_tick - 1) / ms_per_tick ;
- target_time = lis_target_time(ticks*ms_per_tick) ;
+ target_time = lis_target_time(time_interval) ;
if ( (err=lis_check_umem(NULL,VERIFY_WRITE,fds,size))<0
|| (err=lis_check_umem(NULL,VERIFY_READ,fds,size))<0
diff -ur LiS-2.18.orig/head/port-mdep.c LiS-2.18/head/port-mdep.c
--- LiS-2.18.orig/head/port-mdep.c 2004-09-02 22:11:17.000000000 +0300
+++ LiS-2.18/head/port-mdep.c 2005-03-31 14:43:19.148915820 +0200
@@ -312,7 +312,7 @@
tl->prev = NULL ;
tl->function= fn;
tl->data = arg;
- tl->tdelta = ticks * (1000/HZ) ; /* convert to milli-secs */
+ tl->tdelta = jiffies_to_msecs(ticks) ; /* convert to milli-secs */
port_add_timer(tl);
}
diff -ur LiS-2.18.orig/head/user/usrio.c LiS-2.18/head/user/usrio.c
--- LiS-2.18.orig/head/user/usrio.c 2002-10-30 00:38:07.000000000 +0200
+++ LiS-2.18/head/user/usrio.c 2005-03-31 14:52:18.635249739 +0200
@@ -1091,7 +1091,6 @@
long target_time ; /* in ms */
int timer_id = 0 ;
long time_interval = time_out ; /* in ms */
- long ms_per_tick = 1000/HZ ;
long ticks ; /* in system ticks */
pollfd_t *pfds;
pollfd_t *pfd_ptr ;
@@ -1103,11 +1102,7 @@
int ready_fd_cnt = 0 ;
lis_semaphore_t poll_sem ;
- /*
- * Round time interval up to a multiple of system ticks.
- */
- ticks = (time_interval + ms_per_tick - 1) / ms_per_tick ;
- target_time = lis_target_time(ticks*ms_per_tick) ;
+ target_time = lis_target_time(time_interval);
if ( (err=lis_check_umem(NULL,VERIFY_WRITE,fds,size))<0
|| (err=lis_check_umem(NULL,VERIFY_READ,fds,size))<0
diff -ur LiS-2.18.orig/include/sys/LiS/wait.h LiS-2.18/include/sys/LiS/wait.h
--- LiS-2.18.orig/include/sys/LiS/wait.h 2004-09-03 23:41:28.000000000 +0300
+++ LiS-2.18/include/sys/LiS/wait.h 2005-03-31 14:54:55.529305278 +0200
@@ -49,8 +49,6 @@
/* ------------------------------------------------------------------- */
/* Shared global variables */
-#define ONESEC (1000000/HZ) /* ? */
-
/* ------------------------------------------------------------------- */
/* Exported functions & macros */