@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 <[email protected]>
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 <[email protected]>
---
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
On Mon, Oct 12, 2009 at 6:24 AM, John Kacur <[email protected]> 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.
On Mon, 12 Oct 2009, Dave Airlie wrote:
> On Mon, Oct 12, 2009 at 6:24 AM, John Kacur <[email protected]> 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
>> On Mon, Oct 12, 2009 at 6:24 AM, John Kacur <[email protected]> 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 <[email protected]>
>
> Thanks
>
Commit-ID: 55e858c8483af427144f33b42b818b30612b82b0
Gitweb: http://git.kernel.org/tip/55e858c8483af427144f33b42b818b30612b82b0
Author: John Kacur <[email protected]>
AuthorDate: Sun, 11 Oct 2009 22:24:25 +0200
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Acked-by: David Airlie <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
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;
}