Re: [PATCH 1/1] Misc: phantom, move to unlocked_ioctl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 18 May 2007 23:25:52 +0200
"Jiri Slaby" <[email protected]> wrote:

> On 5/18/07, Andrew Morton <[email protected]> wrote:
> > On Fri, 18 May 2007 22:34:53 +0200 (CEST)
> > Jiri Slaby <[email protected]> wrote:
> >
> > > @@ -118,7 +125,9 @@ static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd,
> > >               if (r.reg > 7)
> > >                       return -EINVAL;
> > >
> > > +             spin_lock(&dev->ioctl_lock);
> > >               r.value = ioread32(dev->iaddr + r.reg);
> > > +             spin_unlock(&dev->ioctl_lock);
> >
> > What is that locking protecting in here?
> 
> Well, what led me to do it is that I didn't know how much atomic are
> ioread and iowrite. If concurrent process writes something to the
> place in that space while the other one is reading it, doesn't matter,
> correct?
> 

I don't think there are any atomicity concerns with the IO operation
itself: it's just a 32-bit read or write against a PCI device.

But there may be issues at a higher level: if some other thread of control
can be writing that register then this read is basically meaningless, as
the value which it returns can become wrong at any time.

But that's just the nature of the ioctl API which is being implemented -
there isn't much we can do about that, apart from perhaps deciding not to
implement it at all.  This is a "read an arbitrary register" interface,
yes?  I dunno what it's there for, but it is obviously racy against the
write-a-register ioctls, so userspace just has to be aware of that.

umm, assuming that this interface is actually valuable to userspace then
I'd say that we can remove the spin_lock() and leave the rest alone.

It _may_ make sense to change the SET_REGS operations to do a dummy read
from the device though: PCI posting could cause the user's register-write
to not actually hit the device for an arbitrarily long period after the
ioctl has returned.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux