lkml.org 
[lkml]   [2008]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3, RFC] misc char dev BKL pushdown
From
Date
Mike Frysinger <vapier.adi@gmail.com> wrote:

> please drop the coreb.c changes from your patch

At a minimum, I would hope such a request would say something like "I've
looked at the driver's locking and am convinced that the BKL is not
needed." Have you done that? There is a certain leap of faith involved
in removing that protection from a driver.

I decided to take a quick look...

- You use spin_lock_irq(&coreb_lock) in a number of places, but you do
not take the lock in the interrupt handler. You also do not take the
lock in coreb_write() or coreb_read(), so those can race with the
interrupt handler, with ioctl(), and with each other.

- coreb_write() and coreb_read() do interruptible waits, but do not
check to see whether they were interrupted. They will, in fact,
continue in their I/O loops after a signal.

- In both functions you have:

unsigned long p = *ppos;

if (p + count > coreb_size)
return -EFAULT;

that calculation can overflow.

- You also do this:

static ssize_t coreb_write(struct file *file, const char *buf, size_t count,
loff_t * ppos)
/* ... */
set_dma_start_addr(CH_MEM_STREAM2_SRC, (unsigned long)buf);

In other words, the DMA is done directly to/from a user-space
address. Maybe that's safe on Blackfin, I don't know...

- I have no idea why some of your functions are using d_inode->i_mutex.

- In coreb_ioctl():

spin_lock_irq(&coreb_lock);
if (coreb_status & COREB_IS_RUNNING) {
retval = -EBUSY;
break;
}

this will exit the function with the spinlock still held and
interrupts disabled.

case CMD_COREB_RESET:
printk(KERN_INFO "Resetting Core B\n");
bfin_write_SICB_SYSCR(bfin_read_SICB_SYSCR() | 0x0080);
break;

You do not acquire the lock here, so this can race against other
ioctl() calls. And ioctl() can race against read() and write().

Registration and such seem reasonable, so I can't come up with a
scenario where loss of BKL protection will create trouble. Given the
other problems there, though, I'll confess to being a bit nervous about
it.

jon


\
 
 \ /
  Last update: 2008-05-21 01:27    [W:0.116 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site