From: Russell King This is the start of me unblocking the queue of PCMCIA changes. Some of this patch has been seen before, but other bits haven't. The individual change comments are listed below. I've tested this stuff on both ARM and x86 platforms, and it appears fine for me. [PCMCIA] Remove unused variable warnings. Remove unused variable 'i' in fops methods. Fix debug macros which were the sole consumers of this variable. [PCMCIA] Remove write-only socket_dev No need for a local pointer for the struct device, especially when it is only ever written. If necessary, the device can be accessed using s->parent->dev.dev [PCMCIA] Get rid of racy interruptible_sleep_on() ds.c uses interruptible_sleep_on() without any protection. Use wait_event_interruptible() instead. In addition, fix a bug where threads waiting for cardmgr events to complete were left waiting if cardmgr exited. [PCMCIA] Add refcounting to struct pcmcia_bus_socket If you perform the following commands in order: # cardctl eject # rmmod yenta_socket # insmod drivers/pcmcia/yenta_socket.ko # killall cardmgr the rmmod ends up freeing the pcmcia_bus_socket while the wait queue is still active. The killall cardmgr cases the the select() to complete, and users to be removed from the "queue" - which ends up writing to freed memory. The following patch adds refcounting to pcmcia_bus_socket so we won't free it until all users have gone. We also add "SOCKET_DEAD" to mark the condition where the socket is no longer present in the system. Note that we don't wake up cardmgr when we remove sockets - unfortunately cardmgr doesn't like receiving errors from read(). Really, cardmgr should treat EIO from read() as a fatal error for that socket, and stop listening for events from it. --- drivers/pcmcia/ds.c | 117 +++++++++++++++++++++++++++++++--------------------- 1 files changed, 70 insertions(+), 47 deletions(-) diff -puN drivers/pcmcia/ds.c~pcmcia-update drivers/pcmcia/ds.c --- 25/drivers/pcmcia/ds.c~pcmcia-update 2004-01-19 14:21:29.000000000 -0800 +++ 25-akpm/drivers/pcmcia/ds.c 2004-01-19 14:21:29.000000000 -0800 @@ -51,6 +51,8 @@ #include #include +#include + #include #include #include @@ -95,10 +97,12 @@ typedef struct user_info_t { int event_head, event_tail; event_t event[MAX_EVENTS]; struct user_info_t *next; + struct pcmcia_bus_socket *socket; } user_info_t; /* Socket state information */ struct pcmcia_bus_socket { + atomic_t refcount; client_handle_t handle; int state; user_info_t *user; @@ -106,13 +110,13 @@ struct pcmcia_bus_socket { wait_queue_head_t queue, request; struct work_struct removal; socket_bind_t *bind; - struct device *socket_dev; struct pcmcia_socket *parent; }; #define SOCKET_PRESENT 0x01 #define SOCKET_BUSY 0x02 #define SOCKET_REMOVAL_PENDING 0x10 +#define SOCKET_DEAD 0x80 /*====================================================================*/ @@ -137,6 +141,24 @@ EXPORT_SYMBOL(cs_error); static struct pcmcia_driver * get_pcmcia_driver (dev_info_t *dev_info); static struct pcmcia_bus_socket * get_socket_info_by_nr(unsigned int nr); +static void pcmcia_put_bus_socket(struct pcmcia_bus_socket *s) +{ + if (atomic_dec_and_test(&s->refcount)) + kfree(s); +} + +static struct pcmcia_bus_socket *pcmcia_get_bus_socket(int nr) +{ + struct pcmcia_bus_socket *s; + + s = get_socket_info_by_nr(nr); + if (s) { + WARN_ON(atomic_read(&s->refcount) == 0); + atomic_inc(&s->refcount); + } + return s; +} + /** * pcmcia_register_driver - register a PCMCIA driver with the bus core * @@ -230,13 +252,10 @@ static int handle_request(struct pcmcia_ if (s->state & SOCKET_BUSY) s->req_pending = 1; handle_event(s, event); - if (s->req_pending > 0) { - interruptible_sleep_on(&s->request); - if (signal_pending(current)) - return CS_IN_USE; - else - return s->req_result; - } + if (wait_event_interruptible(s->request, s->req_pending <= 0)) + return CS_IN_USE; + if (s->state & SOCKET_BUSY) + return s->req_result; return CS_SUCCESS; } @@ -501,7 +520,7 @@ static int ds_open(struct inode *inode, DEBUG(0, "ds_open(socket %d)\n", i); - s = get_socket_info_by_nr(i); + s = pcmcia_get_bus_socket(i); if (!s) return -ENODEV; @@ -517,6 +536,7 @@ static int ds_open(struct inode *inode, user->event_tail = user->event_head = 0; user->next = s->user; user->user_magic = USER_MAGIC; + user->socket = s; s->user = user; file->private_data = user; @@ -529,23 +549,23 @@ static int ds_open(struct inode *inode, static int ds_release(struct inode *inode, struct file *file) { - socket_t i = iminor(inode); struct pcmcia_bus_socket *s; user_info_t *user, **link; - DEBUG(0, "ds_release(socket %d)\n", i); - - s = get_socket_info_by_nr(i); - if (!s) - return 0; + DEBUG(0, "ds_release(socket %d)\n", iminor(inode)); user = file->private_data; if (CHECK_USER(user)) goto out; + s = user->socket; + /* Unlink user data structure */ - if ((file->f_flags & O_ACCMODE) != O_RDONLY) + if ((file->f_flags & O_ACCMODE) != O_RDONLY) { s->state &= ~SOCKET_BUSY; + s->req_pending = 0; + wake_up_interruptible(&s->request); + } file->private_data = NULL; for (link = &s->user; *link; link = &(*link)->next) if (*link == user) break; @@ -554,6 +574,7 @@ static int ds_release(struct inode *inod *link = user->next; user->user_magic = 0; kfree(user); + pcmcia_put_bus_socket(s); out: return 0; } /* ds_release */ @@ -563,30 +584,28 @@ out: static ssize_t ds_read(struct file *file, char *buf, size_t count, loff_t *ppos) { - socket_t i = iminor(file->f_dentry->d_inode); struct pcmcia_bus_socket *s; user_info_t *user; + int ret; - DEBUG(2, "ds_read(socket %d)\n", i); + DEBUG(2, "ds_read(socket %d)\n", iminor(inode)); if (count < 4) return -EINVAL; - s = get_socket_info_by_nr(i); - if (!s) - return -ENODEV; - user = file->private_data; if (CHECK_USER(user)) return -EIO; - if (queue_empty(user)) { - interruptible_sleep_on(&s->queue); - if (signal_pending(current)) - return -EINTR; - } + s = user->socket; + if (s->state & SOCKET_DEAD) + return -EIO; + + ret = wait_event_interruptible(s->queue, !queue_empty(user)); + if (ret == 0) + ret = put_user(get_queued_event(user), (int *)buf) ? -EFAULT : 4; - return put_user(get_queued_event(user), (int *)buf) ? -EFAULT : 4; + return ret; } /* ds_read */ /*====================================================================*/ @@ -594,25 +613,24 @@ static ssize_t ds_read(struct file *file static ssize_t ds_write(struct file *file, const char *buf, size_t count, loff_t *ppos) { - socket_t i = iminor(file->f_dentry->d_inode); struct pcmcia_bus_socket *s; user_info_t *user; - DEBUG(2, "ds_write(socket %d)\n", i); + DEBUG(2, "ds_write(socket %d)\n", iminor(inode)); if (count != 4) return -EINVAL; if ((file->f_flags & O_ACCMODE) == O_RDONLY) return -EBADF; - s = get_socket_info_by_nr(i); - if (!s) - return -ENODEV; - user = file->private_data; if (CHECK_USER(user)) return -EIO; + s = user->socket; + if (s->state & SOCKET_DEAD) + return -EIO; + if (s->req_pending) { s->req_pending--; get_user(s->req_result, (int *)buf); @@ -629,19 +647,19 @@ static ssize_t ds_write(struct file *fil /* No kernel lock - fine */ static u_int ds_poll(struct file *file, poll_table *wait) { - socket_t i = iminor(file->f_dentry->d_inode); struct pcmcia_bus_socket *s; user_info_t *user; - DEBUG(2, "ds_poll(socket %d)\n", i); + DEBUG(2, "ds_poll(socket %d)\n", iminor(inode)); - s = get_socket_info_by_nr(i); - if (!s) - return POLLERR; - user = file->private_data; if (CHECK_USER(user)) return POLLERR; + s = user->socket; + /* + * We don't check for a dead socket here since that + * will send cardmgr into an endless spin. + */ poll_wait(file, &s->queue, wait); if (!queue_empty(user)) return POLLIN | POLLRDNORM; @@ -653,17 +671,21 @@ static u_int ds_poll(struct file *file, static int ds_ioctl(struct inode * inode, struct file * file, u_int cmd, u_long arg) { - socket_t i = iminor(inode); struct pcmcia_bus_socket *s; u_int size; int ret, err; ds_ioctl_arg_t buf; + user_info_t *user; - DEBUG(2, "ds_ioctl(socket %d, %#x, %#lx)\n", i, cmd, arg); + DEBUG(2, "ds_ioctl(socket %d, %#x, %#lx)\n", iminor(inode), cmd, arg); - s = get_socket_info_by_nr(i); - if (!s) - return -ENODEV; + user = file->private_data; + if (CHECK_USER(user)) + return -EIO; + + s = user->socket; + if (s->state & SOCKET_DEAD) + return -EIO; size = (cmd & IOCSIZE_MASK) >> IOCSIZE_SHIFT; if (size > sizeof(ds_ioctl_arg_t)) return -EINVAL; @@ -833,6 +855,7 @@ static int __devinit pcmcia_bus_add_sock if(!s) return -ENOMEM; memset(s, 0, sizeof(struct pcmcia_bus_socket)); + atomic_set(&s->refcount, 1); /* * Ugly. But we want to wait for the socket threads to have started up. @@ -845,7 +868,6 @@ static int __devinit pcmcia_bus_add_sock init_waitqueue_head(&s->request); /* initialize data */ - s->socket_dev = socket->dev.dev; INIT_WORK(&s->removal, handle_removal, s); s->parent = socket; @@ -894,7 +916,8 @@ static void pcmcia_bus_remove_socket(str pcmcia_deregister_client(socket->pcmcia->handle); - kfree(socket->pcmcia); + socket->pcmcia->state |= SOCKET_DEAD; + pcmcia_put_bus_socket(socket->pcmcia); socket->pcmcia = NULL; return; _