Skip to contentSkip to conversation

Misc: phantom, move to unlocked_ioctl

PATCH In review Jiri Slaby 2 participants 4 messages May 18, 2007

Subsystems (2)
Char and misc drivers Sensable phantom
lore
Share & tools
lore

Jiri Slaby

author
#0 lore raw

PATCH 1/1 Misc: phantom, move to unlocked_ioctl

phantom, move to unlocked_ioctl

phantom's ioctl is often (4000 times a sec or so) invoked, don't acquire BKL and block other processes.

Signed-off-by Jiri Slaby <[email protected]>

---
commit 79b7336ec18e967de0026d2cc08de47da6333761
tree d180d46c4bf38ee42adf8949e9f746f893aae32b
parent 34762198ff54f921aa49be10dcc21efaf53f4a4c
author Jiri Slaby <[email protected]> Fri, 18 May 2007 22:02:59 +0200 committer Jiri Slaby <[email protected]> Fri, 18 May 2007 22:02:59 +0200

 drivers/misc/phantom.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
drivers/misc/phantom.c +16 -4
index 35b139b..6249767 100644
@@ -47,6 +47,7 @@ struct phantom_device {
 	struct cdev cdev;
 
 	struct mutex open_lock;
+	spinlock_t ioctl_lock;
 };
 
 static unsigned char phantom_devices[PHANTOM_MAX_MINORS];

@@ -71,8 +72,8 @@ static int phantom_status(struct phantom_device *dev, unsigned long newstat)
  * File ops
  */
 
-static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd,
-	u_long arg)
+static long phantom_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg)
 {
 	struct phantom_device *dev = file->private_data;
 	struct phm_regs rs;

@@ -92,24 +93,30 @@ static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd,
 		if (r.reg > 7)
 			return -EINVAL;
 
+		spin_lock(&dev->ioctl_lock);
 		if (r.reg == PHN_CONTROL && (r.value & PHN_CTL_IRQ) &&
-				phantom_status(dev, dev->status | PHB_RUNNING))
+				phantom_status(dev, dev->status | PHB_RUNNING)){
+			spin_unlock(&dev->ioctl_lock);
 			return -ENODEV;
+		}
 
 		pr_debug("phantom: writing %x to %u\n", r.value, r.reg);
 		iowrite32(r.value, dev->iaddr + r.reg);
 
 		if (r.reg == PHN_CONTROL && !(r.value & PHN_CTL_IRQ))
 			phantom_status(dev, dev->status & ~PHB_RUNNING);
+		spin_unlock(&dev->ioctl_lock);
 		break;
 	case PHN_SET_REGS:
 		if (copy_from_user(&rs, argp, sizeof(rs)))
 			return -EFAULT;
 
 		pr_debug("phantom: SRS %u regs %x\n", rs.count, rs.mask);
+		spin_lock(&dev->ioctl_lock);
 		for (i = 0; i < min(rs.count, 8U); i++)
 			if ((1 << i) & rs.mask)
 				iowrite32(rs.values[i], dev->oaddr + i);
+		spin_unlock(&dev->ioctl_lock);
 		break;
 	case PHN_GET_REG:
 		if (copy_from_user(&r, argp, sizeof(r)))

@@ -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);
 
 		if (copy_to_user(argp, &r, sizeof(r)))
 			return -EFAULT;

@@ -128,9 +137,11 @@ static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd,
 			return -EFAULT;
 
 		pr_debug("phantom: GRS %u regs %x\n", rs.count, rs.mask);
+		spin_lock(&dev->ioctl_lock);
 		for (i = 0; i < min(rs.count, 8U); i++)
 			if ((1 << i) & rs.mask)
 				rs.values[i] = ioread32(dev->iaddr + i);
+		spin_unlock(&dev->ioctl_lock);
 
 		if (copy_to_user(argp, &rs, sizeof(rs)))
 			return -EFAULT;

@@ -199,7 +210,7 @@ static unsigned int phantom_poll(struct file *file, poll_table *wait)
 static struct file_operations phantom_file_ops = {
 	.open = phantom_open,
 	.release = phantom_release,
-	.ioctl = phantom_ioctl,
+	.unlocked_ioctl = phantom_ioctl,
 	.poll = phantom_poll,
 };
 

@@ -282,6 +293,7 @@ static int __devinit phantom_probe(struct pci_dev *pdev,
 	}
 
 	mutex_init(&pht->open_lock);
+	spin_lock_init(&pht->ioctl_lock);
 	init_waitqueue_head(&pht->wait);
 	cdev_init(&pht->cdev, &phantom_file_ops);
 	pht->cdev.owner = THIS_MODULE;
#1 lore raw

drivers/misc/phantom.c static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd, · line 130

Quoted 7 lines · On Fri, 18 May 2007 22:34:53 +0200 (CEST) Jiri Slaby <[email protected]>…
drivers/misc/phantom.c +2 -0
@@ -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?

author
#2 lore raw
Quoted 7 lines · Andrew Morton
Quoted 4 lines · On Fri, 18 May 2007 22:34:53 +0200 (CEST) Jiri Slaby <[email protected]>…
drivers/misc/phantom.c +0 -0
@@ -118,7 +125,9 @@ static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd,
              if (r.reg > 7)
                      return -EINVAL;
Quoted 5 lines

drivers/misc/phantom.c static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd, · line 130

Quoted 3 lines
drivers/misc/phantom.c +2 -0
+             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?

thanks,

Signature · 4 lines
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E
#3 lore raw
Quoted 14 lines · On Fri, 18 May 2007 23:25:52 +0200 "Jiri Slaby" <[email protected]> wrote:
Quoted 7 lines · Andrew Morton
Quoted 4 lines · On Fri, 18 May 2007 22:34:53 +0200 (CEST) Jiri Slaby <[email protected]>…
drivers/misc/phantom.c +0 -0
@@ -118,7 +125,9 @@ static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd,
              if (r.reg > 7)
                      return -EINVAL;
Quoted 5 lines

drivers/misc/phantom.c static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd, · line 130

Quoted 3 lines
drivers/misc/phantom.c +2 -0
+             spin_lock(&dev->ioctl_lock);
              r.value = ioread32(dev->iaddr + r.reg);
+             spin_unlock(&dev->ioctl_lock);

What is that locking protecting in here?

Quoted 5 lines

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.