[PATCH] Remove BKL from PCI syscalls

[PATCH] Remove BKL from PCI syscalls

[Matthew Wilcox]

Remove use of the BKL from the PCI syscalls.  It was protecting against
PCI device removal.  By taking a reference (using pci_get_slot), we
ensure the device will not be removed while we're using it, and can
remove the BKL.

While I'm in this file, update the comment at the top to reflect the
status of the syscalls.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

diff --git a/drivers/pci/syscall.c b/drivers/pci/syscall.c
index 9d37fec..56dc193 100644
--- a/drivers/pci/syscall.c
+++ b/drivers/pci/syscall.c
@@ -1,10 +1,13 @@
 /*
  *	pci_syscall.c
  *
- * For architectures where we want to allow direct access
- * to the PCI config stuff - it would probably be preferable
- * on PCs too, but there people just do it by hand with the
- * magic northbridge registers..
+ * This is a legacy method for accessing PCI config space on some
+ * architectures.  These days, everybody should be using sysfs or procfs
+ * which allow non-privileged users to read the safe bits of config space,
+ * but we can't remove these syscalls.  These syscalls do not allow access
+ * to PCI domains other than zero, and while it would be possible to add
+ * that support, it is better to migrate any userspace application to use
+ * the sysfs method.
  */
 
 #include <linux/errno.h>
@@ -25,16 +28,13 @@ sys_pciconfig_read(unsigned long bus, unsigned long dfn,
 	u32 dword;
 	long err, cfg_ret;
 
-	err = -EPERM;
 	if (!capable(CAP_SYS_ADMIN))
-		goto error;
+		return -EPERM;
 
-	err = -ENODEV;
-	dev = pci_find_slot(bus, dfn);
+	dev = pci_get_slot(pci_find_bus(0, bus), dfn);
 	if (!dev)
-		goto error;
+		return -ENODEV;
 
-	lock_kernel();
 	switch (len) {
 	case 1:
 		cfg_ret = pci_user_read_config_byte(dev, off, &byte);
@@ -47,10 +47,8 @@ sys_pciconfig_read(unsigned long bus, unsigned long dfn,
 		break;
 	default:
 		err = -EINVAL;
-		unlock_kernel();
 		goto error;
 	};
-	unlock_kernel();
 
 	err = -EIO;
 	if (cfg_ret != PCIBIOS_SUCCESSFUL)
@@ -67,9 +65,10 @@ sys_pciconfig_read(unsigned long bus, unsigned long dfn,
 		err = put_user(dword, (unsigned int __user *)buf);
 		break;
 	};
-	return err;
 
-error:
+	goto out;
+
+ error:
 	/* ??? XFree86 doesn't even check the return value.  They
 	   just look for 0xffffffff in the output, since that's what
 	   they get instead of a machine check on x86.  */
@@ -84,6 +83,8 @@ error:
 		put_user(-1, (unsigned int __user *)buf);
 		break;
 	};
+ out:
+	pci_dev_put(dev);
 	return err;
 }
 
@@ -101,11 +102,10 @@ sys_pciconfig_write(unsigned long bus, unsigned long dfn,
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	dev = pci_find_slot(bus, dfn);
+	dev = pci_get_slot(pci_find_bus(0, bus), dfn);
 	if (!dev)
 		return -ENODEV;
 
-	lock_kernel();
 	switch(len) {
 	case 1:
 		err = get_user(byte, (u8 __user *)buf);
@@ -138,7 +138,7 @@ sys_pciconfig_write(unsigned long bus, unsigned long dfn,
 		err = -EINVAL;
 		break;
 	};
-	unlock_kernel();
 
+	pci_dev_put(dev);
 	return err;
 }

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Remove BKL from PCI syscalls

[Greg KH]

On Tue, Jul 10, 2007 at 10:55:32AM -0600, Matthew Wilcox wrote:
> 
> Remove use of the BKL from the PCI syscalls.  It was protecting against
> PCI device removal.  By taking a reference (using pci_get_slot), we
> ensure the device will not be removed while we're using it, and can
> remove the BKL.
> 
> While I'm in this file, update the comment at the top to reflect the
> status of the syscalls.
> 
> Signed-off-by: Matthew Wilcox <matthew@wil.cx>

What tree did you make this against?  I get the following errors trying
to apply it to my pci tree:
	Applying patch pci-remove-bkl-from-pci-syscalls.patch
	patching file drivers/pci/syscall.c
	Hunk #2 FAILED at 28.
	Hunk #4 FAILED at 65.
	Hunk #5 FAILED at 83.
	Hunk #6 FAILED at 102.
	Hunk #7 FAILED at 138.
	5 out of 7 hunks FAILED -- rejects in file drivers/pci/syscall.c

thanks,

greg k-h

Re: [PATCH] Remove BKL from PCI syscalls

[Matthew Wilcox]

On Tue, Jul 10, 2007 at 04:14:24PM -0700, Greg KH wrote:
> What tree did you make this against?

Linus' tree, as of this morning.

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Remove BKL from PCI syscalls

[Matthew Wilcox]

On Tue, Jul 10, 2007 at 05:36:47PM -0600, Matthew Wilcox wrote:
> On Tue, Jul 10, 2007 at 04:14:24PM -0700, Greg KH wrote:
> > What tree did you make this against?
> 
> Linus' tree, as of this morning.

Looks like it's clashing with:

http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f=pci/pci-syscall.c-switch-to-refcounting-api.patch;hå13fc9669456ee08884c496ce4e6899c006dc7a;hb=HEAD

I believe my patch to be a superset of this one, and recommend just
dropping Alan's patch.

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
-
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Remove BKL from PCI syscalls

[Greg KH]

On Tue, Jul 10, 2007 at 05:42:18PM -0600, Matthew Wilcox wrote:
> On Tue, Jul 10, 2007 at 05:36:47PM -0600, Matthew Wilcox wrote:
> > On Tue, Jul 10, 2007 at 04:14:24PM -0700, Greg KH wrote:
> > > What tree did you make this against?
> > 
> > Linus' tree, as of this morning.
> 
> Looks like it's clashing with:
> 
> http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f=pci/pci-syscall.c-switch-to-refcounting-api.patch;hå13fc9669456ee08884c496ce4e6899c006dc7a;hb=HEAD
> 
> I believe my patch to be a superset of this one, and recommend just
> dropping Alan's patch.

How about just doing your patch on top of Alan's instead?

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Remove BKL from PCI syscalls

[Matthew Wilcox]

On Tue, Jul 10, 2007 at 04:49:36PM -0700, Greg KH wrote:
> On Tue, Jul 10, 2007 at 05:42:18PM -0600, Matthew Wilcox wrote:
> > I believe my patch to be a superset of this one, and recommend just
> > dropping Alan's patch.
> 
> How about just doing your patch on top of Alan's instead?

Seems like a lot more work; wasn't the point of keeping patches in a git
tree like this to make it easy to drop old patches?  I'd understand if
it meant pulling apart and rebasing a git tree.

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Remove BKL from PCI syscalls

[Greg KH]

On Tue, Jul 10, 2007 at 06:16:14PM -0600, Matthew Wilcox wrote:
> On Tue, Jul 10, 2007 at 04:49:36PM -0700, Greg KH wrote:
> > On Tue, Jul 10, 2007 at 05:42:18PM -0600, Matthew Wilcox wrote:
> > > I believe my patch to be a superset of this one, and recommend just
> > > dropping Alan's patch.
> > 
> > How about just doing your patch on top of Alan's instead?
> 
> Seems like a lot more work; wasn't the point of keeping patches in a git
> tree like this to make it easy to drop old patches?  I'd understand if
> it meant pulling apart and rebasing a git tree.

Well, it depends.  Alan's patch fixes up the api to do the proper thing
now, and your patch builds on that showing that we can now just drop the
BKL safely.  They are two different things in a way as is shown by the
fact that two different people approached this differently :)

Also, Alan's patch has already gotten a lot of testing in -mm for a
while and is about to be sent to Linus, while your's would be a bit
further back in the queue.

thanks,

greg k-h

Re: [PATCH] Remove BKL from PCI syscalls

[Matthew Wilcox]

On Tue, Jul 10, 2007 at 05:30:01PM -0700, Greg KH wrote:
> On Tue, Jul 10, 2007 at 06:16:14PM -0600, Matthew Wilcox wrote:
> > On Tue, Jul 10, 2007 at 04:49:36PM -0700, Greg KH wrote:
> > > On Tue, Jul 10, 2007 at 05:42:18PM -0600, Matthew Wilcox wrote:
> > > > I believe my patch to be a superset of this one, and recommend just
> > > > dropping Alan's patch.
> > > 
> > > How about just doing your patch on top of Alan's instead?
> > 
> > Seems like a lot more work; wasn't the point of keeping patches in a git
> > tree like this to make it easy to drop old patches?  I'd understand if
> > it meant pulling apart and rebasing a git tree.
> 
> Well, it depends.  Alan's patch fixes up the api to do the proper thing
> now, and your patch builds on that showing that we can now just drop the
> BKL safely.  They are two different things in a way as is shown by the
> fact that two different people approached this differently :)

I suppose that's one way of looking at it.  It just feels wrong -- git
is supposed to show how development happened, and I didn't base my patch
on Alan's.

It sucks that Alan sent his patch three months ago and it
still didn't make it into the 2.6.22 release.  It's not even
controversial.  Indeed, as far as I can tell, it didn't even
make it into your tree until *after* I'd sent off this patch yesterday!
http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=commit;hÞ575d18a94e63c681435a16f5c72a3c13dd3345
is why I think that -- please tell me if I'm confused.

> Also, Alan's patch has already gotten a lot of testing in -mm for a
> while and is about to be sent to Linus, while your's would be a bit
> further back in the queue.

How much testing has Alan's patch actually had?  This file isn't even
built on x86, and I don't believe there's any userspace actually using
these syscalls.  lspci (and other libpci users) don't have the syscalls
as an access method.  I believe the only user was X, and I don't think X
uses these syscalls any more.

<ajax> looks like they're referenced in the alpha pci code, but i have
no idea if that crap gets called anymore
<ajax> the generic code uses sysfs

So is there anyone testing -mm on Alpha, and running X?

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
-
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Remove BKL from PCI syscalls

[Greg KH]

On Wed, Jul 11, 2007 at 02:20:54PM -0600, Matthew Wilcox wrote:
> On Tue, Jul 10, 2007 at 05:30:01PM -0700, Greg KH wrote:
> > On Tue, Jul 10, 2007 at 06:16:14PM -0600, Matthew Wilcox wrote:
> > > On Tue, Jul 10, 2007 at 04:49:36PM -0700, Greg KH wrote:
> > > > On Tue, Jul 10, 2007 at 05:42:18PM -0600, Matthew Wilcox wrote:
> > > > > I believe my patch to be a superset of this one, and recommend just
> > > > > dropping Alan's patch.
> > > > 
> > > > How about just doing your patch on top of Alan's instead?
> > > 
> > > Seems like a lot more work; wasn't the point of keeping patches in a git
> > > tree like this to make it easy to drop old patches?  I'd understand if
> > > it meant pulling apart and rebasing a git tree.
> > 
> > Well, it depends.  Alan's patch fixes up the api to do the proper thing
> > now, and your patch builds on that showing that we can now just drop the
> > BKL safely.  They are two different things in a way as is shown by the
> > fact that two different people approached this differently :)
> 
> I suppose that's one way of looking at it.  It just feels wrong -- git
> is supposed to show how development happened, and I didn't base my patch
> on Alan's.
> 
> It sucks that Alan sent his patch three months ago and it
> still didn't make it into the 2.6.22 release.  It's not even
> controversial.  Indeed, as far as I can tell, it didn't even
> make it into your tree until *after* I'd sent off this patch yesterday!
> http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=commit;hÞ575d18a94e63c681435a16f5c72a3c13dd3345
> is why I think that -- please tell me if I'm confused.

Yes, that is because Alan sent it to Andrew and not me, so it was in the
-mm tree for a while before Andrew sent it on to me.

> > Also, Alan's patch has already gotten a lot of testing in -mm for a
> > while and is about to be sent to Linus, while your's would be a bit
> > further back in the queue.
> 
> How much testing has Alan's patch actually had?

As much testing as any -mm release does :)

> So is there anyone testing -mm on Alpha, and running X?

I do not know, try asking on lkml.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Remove BKL from PCI syscalls

[Alan Cox]

> Yes, that is because Alan sent it to Andrew and not me, so it was in the
> -mm tree for a while before Andrew sent it on to me.

I should have cc'd you. Don't know if I forgot perhaps.

> > > Also, Alan's patch has already gotten a lot of testing in -mm for a
> > > while and is about to be sent to Linus, while your's would be a bit
> > > further back in the queue.
> > 
> > How much testing has Alan's patch actually had?
> 
> As much testing as any -mm release does :)

Really both problems want fixing - no pci_find_* and no lock_kernel. I
really don't care which patch or combination gets merged

Re: [PATCH] Remove BKL from PCI syscalls

[Matthew Wilcox]

On Tue, Jul 10, 2007 at 04:49:36PM -0700, Greg KH wrote:
> How about just doing your patch on top of Alan's instead?

Now that Alan's patch is in Linus' tree, here's an update of my patch:

----

Remove use of the BKL from the PCI syscalls since it no longer serves
any purpose.  It used to protect against PCI device removal, but now we
have a reference to the device, we no longer need that protection.

While I'm in this file, update the comment at the top to reflect the
status of the syscalls.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

diff --git a/drivers/pci/syscall.c b/drivers/pci/syscall.c
index 2ac050d..0934760 100644
--- a/drivers/pci/syscall.c
+++ b/drivers/pci/syscall.c
@@ -1,10 +1,13 @@
 /*
  *	pci_syscall.c
  *
- * For architectures where we want to allow direct access
- * to the PCI config stuff - it would probably be preferable
- * on PCs too, but there people just do it by hand with the
- * magic northbridge registers..
+ * This is a legacy method for accessing PCI config space on some
+ * architectures.  These days, everybody should be using sysfs or procfs
+ * which allow non-privileged users to read the safe bits of config space,
+ * but we can't remove these syscalls.  These syscalls do not allow access
+ * to PCI domains other than zero, and while it would be possible to add
+ * that support, it is better to migrate any userspace application to use
+ * the sysfs method.
  */
 
 #include <linux/errno.h>
@@ -23,18 +26,15 @@ sys_pciconfig_read(unsigned long bus, unsigned long dfn,
 	u8 byte;
 	u16 word;
 	u32 dword;
-	long err;
-	long cfg_ret;
+	long err, cfg_ret;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	err = -ENODEV;
-	dev = pci_get_bus_and_slot(bus, dfn);
+	dev = pci_get_slot(pci_find_bus(0, bus), dfn);
 	if (!dev)
-		goto error;
+		return -ENODEV;
 
-	lock_kernel();
 	switch (len) {
 	case 1:
 		cfg_ret = pci_user_read_config_byte(dev, off, &byte);
@@ -47,10 +47,8 @@ sys_pciconfig_read(unsigned long bus, unsigned long dfn,
 		break;
 	default:
 		err = -EINVAL;
-		unlock_kernel();
 		goto error;
-	};
-	unlock_kernel();
+	}
 
 	err = -EIO;
 	if (cfg_ret != PCIBIOS_SUCCESSFUL)
@@ -67,10 +65,10 @@ sys_pciconfig_read(unsigned long bus, unsigned long dfn,
 		err = put_user(dword, (unsigned int __user *)buf);
 		break;
 	}
-	pci_dev_put(dev);
-	return err;
 
-error:
+	goto out;
+
+ error:
 	/* ??? XFree86 doesn't even check the return value.  They
 	   just look for 0xffffffff in the output, since that's what
 	   they get instead of a machine check on x86.  */
@@ -85,6 +83,7 @@ error:
 		put_user(-1, (unsigned int __user *)buf);
 		break;
 	}
+ out:
 	pci_dev_put(dev);
 	return err;
 }
@@ -103,11 +102,10 @@ sys_pciconfig_write(unsigned long bus, unsigned long dfn,
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	dev = pci_get_bus_and_slot(bus, dfn);
+	dev = pci_get_slot(pci_find_bus(0, bus), dfn);
 	if (!dev)
 		return -ENODEV;
 
-	lock_kernel();
 	switch(len) {
 	case 1:
 		err = get_user(byte, (u8 __user *)buf);
@@ -140,7 +138,7 @@ sys_pciconfig_write(unsigned long bus, unsigned long dfn,
 		err = -EINVAL;
 		break;
 	}
-	unlock_kernel();
+
 	pci_dev_put(dev);
 	return err;
 }

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Remove BKL from PCI syscalls

[Alan Cox]

> -	dev = pci_get_bus_and_slot(bus, dfn);
> +	dev = pci_get_slot(pci_find_bus(0, bus), dfn);
>  	if (!dev)

What happens if pci_find_bus(0, bus) returns NULL.

And this change is not needed as pci_get_bus_and_slot now checks for
domain 0 and that was fixed.

Alan