[PATCH RFC] frontend.c: Remove the BKL from agp_open

[PATCH RFC] frontend.c: Remove the BKL from agp_open

[John Kacur]

@David Airlie. I was looking at the agp driver for removal of the bkl 
lock. From what I could tell, the mutex_lock(&(agp_fe.agp_mutex)) should 
cover what we need. However, I may have easily missed something, and would 
appreciate your review / comments on the patch.

>From 261cd08816c2b4c767b6ae038737beef32fe1085 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sun, 11 Oct 2009 21:37:51 +0200
Subject: [PATCH] frontend.c: Remove the BKL from agp_open

- Remove the BKL from agp_open
- Perform a few clean-ups.

Analysis:
---------
int minor is local to the function.

The following are protected by agp_fe.agp_mutex
struct agp_file_private *priv;
struct agp_client *client;

Call-outs:

kzalloc should be safe to call under the mutex_lock

agp_find_client_by_pid:
	- agp_mmap calls that under agp_fe.agp_mutex which we hold in agp_open
	- agpioc_reserve_wrap calls it without any locking what-so-ever.
		- Is that an error? Or is that okay because it has pid that is
		  a unique handle?

agp_insert_file_private:
	- This function only manipulates struct agp_file_private, once again
	  while agp_fe.agp_mutex is held

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/char/agp/frontend.c |   28 +++++++++++-----------------
 1 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/char/agp/frontend.c b/drivers/char/agp/frontend.c
index a96f319..43412c0 100644
--- a/drivers/char/agp/frontend.c
+++ b/drivers/char/agp/frontend.c
@@ -676,25 +676,25 @@ static int agp_open(struct inode *inode, struct file *file)
 	int minor = iminor(inode);
 	struct agp_file_private *priv;
 	struct agp_client *client;
-	int rc = -ENXIO;
-
-	lock_kernel();
-	mutex_lock(&(agp_fe.agp_mutex));
 
 	if (minor != AGPGART_MINOR)
-		goto err_out;
+		return -ENXIO;
+
+	mutex_lock(&(agp_fe.agp_mutex));
 
 	priv = kzalloc(sizeof(struct agp_file_private), GFP_KERNEL);
-	if (priv == NULL)
-		goto err_out_nomem;
+	if (priv == NULL) {
+		mutex_unlock(&(agp_fe.agp_mutex));
+		return -ENOMEM;
+	}
 
 	set_bit(AGP_FF_ALLOW_CLIENT, &priv->access_flags);
 	priv->my_pid = current->pid;
 
-	if (capable(CAP_SYS_RAWIO)) {
+	if (capable(CAP_SYS_RAWIO))
 		/* Root priv, can be controller */
 		set_bit(AGP_FF_ALLOW_CONTROLLER, &priv->access_flags);
-	}
+
 	client = agp_find_client_by_pid(current->pid);
 
 	if (client != NULL) {
@@ -704,16 +704,10 @@ static int agp_open(struct inode *inode, struct file *file)
 	file->private_data = (void *) priv;
 	agp_insert_file_private(priv);
 	DBG("private=%p, client=%p", priv, client);
-	mutex_unlock(&(agp_fe.agp_mutex));
-	unlock_kernel();
-	return 0;
 
-err_out_nomem:
-	rc = -ENOMEM;
-err_out:
 	mutex_unlock(&(agp_fe.agp_mutex));
-	unlock_kernel();
-	return rc;
+
+	return 0;
 }
 
 
-- 
1.6.0.6

Re: [PATCH RFC] frontend.c: Remove the BKL from agp_open

[Dave Airlie]

On Mon, Oct 12, 2009 at 6:24 AM, John Kacur <jkacur@redhat.com> wrote:
>
> @David Airlie. I was looking at the agp driver for removal of the bkl
> lock. From what I could tell, the mutex_lock(&(agp_fe.agp_mutex)) should
> cover what we need. However, I may have easily missed something, and would
> appreciate your review / comments on the patch.

This seems fine, though I suck at locking logic,

One thing to note is the number of users of this interface is probably
close to 0,
so even if this does break something we probably won't ever find out.

Dave.

Re: [PATCH RFC] frontend.c: Remove the BKL from agp_open

[John Kacur]

On Mon, 12 Oct 2009, Dave Airlie wrote:

> On Mon, Oct 12, 2009 at 6:24 AM, John Kacur <jkacur@redhat.com> wrote:
> >
> > @David Airlie. I was looking at the agp driver for removal of the bkl
> > lock. From what I could tell, the mutex_lock(&(agp_fe.agp_mutex)) should
> > cover what we need. However, I may have easily missed something, and would
> > appreciate your review / comments on the patch.
> 
> This seems fine, though I suck at locking logic,
> 
> One thing to note is the number of users of this interface is probably
> close to 0,
> so even if this does break something we probably won't ever find out.
> 
> Dave.
> 

Hmnn, you are listed as the maintainer - don't at least YOU use this 
driver? Also, if it seem fine to you, can I have an ack?

Thanks

Re: [PATCH RFC] frontend.c: Remove the BKL from agp_open

[Dave Airlie]

>> On Mon, Oct 12, 2009 at 6:24 AM, John Kacur <jkacur@redhat.com> wrote:
>> >
>> > @David Airlie. I was looking at the agp driver for removal of the bkl
>> > lock. From what I could tell, the mutex_lock(&(agp_fe.agp_mutex)) should
>> > cover what we need. However, I may have easily missed something, and would
>> > appreciate your review / comments on the patch.
>>
>> This seems fine, though I suck at locking logic,
>>
>> One thing to note is the number of users of this interface is probably
>> close to 0,
>> so even if this does break something we probably won't ever find out.
>>
>> Dave.
>>
>
> Hmnn, you are listed as the maintainer - don't at least YOU use this
> driver? Also, if it seem fine to you, can I have an ack?

The AGP frontend isn't a driver, its just an interface to the AGP driver,
for very old X servers pre-drm support, most X servers don't use this
interface at all anymore, the graphics driver deals with it.

But it looks fine to me, so

Acked-by: Dave Airlie <airlied@redhat.com>

>
> Thanks
>

[tip:bkl/drivers] agp: Remove the BKL from agp_open

[tip-bot for John Kacur]

Commit-ID:  55e858c8483af427144f33b42b818b30612b82b0
Gitweb:     http://git.kernel.org/tip/55e858c8483af427144f33b42b818b30612b82b0
Author:     John Kacur <jkacur@redhat.com>
AuthorDate: Sun, 11 Oct 2009 22:24:25 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:54 +0200

agp: Remove the BKL from agp_open

- Remove the BKL from agp_open
- Perform a few clean-ups.

Analysis:
---------
int minor is local to the function.

The following are protected by agp_fe.agp_mutex
struct agp_file_private *priv;
struct agp_client *client;

Call-outs:

kzalloc should be safe to call under the mutex_lock

agp_find_client_by_pid:
	- agp_mmap calls that under agp_fe.agp_mutex which we hold in agp_open
	- agpioc_reserve_wrap calls it without any locking what-so-ever.
		- Is that an error? Or is that okay because it has pid that is
		  a unique handle?

agp_insert_file_private:
	- This function only manipulates struct agp_file_private, once again
	  while agp_fe.agp_mutex is held

Signed-off-by: John Kacur <jkacur@redhat.com>
Acked-by: David Airlie <airlied@linux.ie>
LKML-Reference: <alpine.LFD.2.00.0910112216060.12574@localhost.localdomain>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/char/agp/frontend.c |   28 +++++++++++-----------------
 1 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/char/agp/frontend.c b/drivers/char/agp/frontend.c
index a96f319..43412c0 100644
--- a/drivers/char/agp/frontend.c
+++ b/drivers/char/agp/frontend.c
@@ -676,25 +676,25 @@ static int agp_open(struct inode *inode, struct file *file)
 	int minor = iminor(inode);
 	struct agp_file_private *priv;
 	struct agp_client *client;
-	int rc = -ENXIO;
-
-	lock_kernel();
-	mutex_lock(&(agp_fe.agp_mutex));
 
 	if (minor != AGPGART_MINOR)
-		goto err_out;
+		return -ENXIO;
+
+	mutex_lock(&(agp_fe.agp_mutex));
 
 	priv = kzalloc(sizeof(struct agp_file_private), GFP_KERNEL);
-	if (priv == NULL)
-		goto err_out_nomem;
+	if (priv == NULL) {
+		mutex_unlock(&(agp_fe.agp_mutex));
+		return -ENOMEM;
+	}
 
 	set_bit(AGP_FF_ALLOW_CLIENT, &priv->access_flags);
 	priv->my_pid = current->pid;
 
-	if (capable(CAP_SYS_RAWIO)) {
+	if (capable(CAP_SYS_RAWIO))
 		/* Root priv, can be controller */
 		set_bit(AGP_FF_ALLOW_CONTROLLER, &priv->access_flags);
-	}
+
 	client = agp_find_client_by_pid(current->pid);
 
 	if (client != NULL) {
@@ -704,16 +704,10 @@ static int agp_open(struct inode *inode, struct file *file)
 	file->private_data = (void *) priv;
 	agp_insert_file_private(priv);
 	DBG("private=%p, client=%p", priv, client);
-	mutex_unlock(&(agp_fe.agp_mutex));
-	unlock_kernel();
-	return 0;
 
-err_out_nomem:
-	rc = -ENOMEM;
-err_out:
 	mutex_unlock(&(agp_fe.agp_mutex));
-	unlock_kernel();
-	return rc;
+
+	return 0;
 }