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

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 18 May 2007 23:25:52 +0200
"Jiri Slaby" <jirislaby@gmail.com> wrote:

> On 5/18/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Fri, 18 May 2007 22:34:53 +0200 (CEST)
> > Jiri Slaby <jirislaby@gmail.com> 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.