From: Stefan Richter <stefanr@s5r6.in-berlin.de>
To: linux1394-devel@lists.sourceforge.net
Cc: linux-kernel@vger.kernel.org
Subject: [PATCH 01/11] firewire: cdev: reference-count client instances
Date: Sun, 4 Jan 2009 16:24:41 +0100 (CET) [thread overview]
Message-ID: <tkrat.8de770712ed54822@s5r6.in-berlin.de> (raw)
In-Reply-To: <tkrat.3905b2754a8fc519@s5r6.in-berlin.de>
The lifetime of struct client instances must be longer than the lifetime
of any client resource.
This fixes a possible race between fw_device_op_release and transaction
completions. It also prepares for new ioctls for isochronous resource
management which will involve delayed processing of client resources.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
drivers/firewire/fw-cdev.c | 55 ++++++++++++++++++++++++++++++-------
1 file changed, 46 insertions(+), 9 deletions(-)
Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/kernel.h>
+#include <linux/kref.h>
#include <linux/wait.h>
#include <linux/errno.h>
#include <linux/device.h>
@@ -94,8 +95,27 @@ struct client {
unsigned long vm_start;
struct list_head link;
+ struct kref kref;
};
+static inline void client_get(struct client *client)
+{
+ kref_get(&client->kref);
+}
+
+static void client_release(struct kref *kref)
+{
+ struct client *client = container_of(kref, struct client, kref);
+
+ fw_device_put(client->device);
+ kfree(client);
+}
+
+static void client_put(struct client *client)
+{
+ kref_put(&client->kref, client_release);
+}
+
static inline void __user *u64_to_uptr(__u64 value)
{
return (void __user *)(unsigned long)value;
@@ -131,6 +151,7 @@ static int fw_device_op_open(struct inod
idr_init(&client->resource_idr);
INIT_LIST_HEAD(&client->event_list);
init_waitqueue_head(&client->wait);
+ kref_init(&client->kref);
file->private_data = client;
@@ -326,6 +347,8 @@ static int add_client_resource(struct cl
else
ret = idr_get_new(&client->resource_idr, resource,
&resource->handle);
+ if (ret >= 0)
+ client_get(client);
spin_unlock_irqrestore(&client->lock, flags);
if (ret == -EAGAIN)
@@ -358,6 +381,8 @@ static int release_client_resource(struc
else
r->release(client, r);
+ client_put(client);
+
return 0;
}
@@ -385,11 +410,21 @@ static void complete_transaction(struct
spin_lock_irqsave(&client->lock, flags);
/*
- * If called while in shutdown, the idr tree must be left untouched.
- * The idr handle will be removed later.
+ * 1. If called while in shutdown, the idr tree must be left untouched.
+ * The idr handle will be removed and the client reference will be
+ * dropped later.
+ * 2. If the call chain was release_client_resource ->
+ * release_transaction -> complete_transaction (instead of a normal
+ * conclusion of the transaction), i.e. if this resource was already
+ * unregistered from the idr, the client reference will be dropped
+ * by release_client_resource and we must not drop it here.
*/
- if (!client->in_shutdown)
+ if (!client->in_shutdown &&
+ idr_find(&client->resource_idr, response->resource.handle)) {
idr_remove(&client->resource_idr, response->resource.handle);
+ /* Drop the idr's reference */
+ client_put(client);
+ }
spin_unlock_irqrestore(&client->lock, flags);
r->type = FW_CDEV_EVENT_RESPONSE;
@@ -408,6 +443,9 @@ static void complete_transaction(struct
else
queue_event(client, &response->event, r, sizeof(*r) + r->length,
NULL, 0);
+
+ /* Drop the transaction callback's reference */
+ client_put(client);
}
static int ioctl_send_request(struct client *client, void *buffer)
@@ -459,6 +497,9 @@ static int ioctl_send_request(struct cli
if (ret < 0)
goto failed;
+ /* Get a reference for the transaction callback */
+ client_get(client);
+
fw_send_request(device->card, &response->transaction,
request->tcode & 0x1f,
device->node->node_id,
@@ -1044,6 +1085,7 @@ static int shutdown_resource(int id, voi
struct client *client = data;
r->release(client, r);
+ client_put(client);
return 0;
}
@@ -1076,12 +1118,7 @@ static int fw_device_op_release(struct i
list_for_each_entry_safe(e, next_e, &client->event_list, link)
kfree(e);
- /*
- * FIXME: client should be reference-counted. It's extremely unlikely
- * but there may still be transactions being completed at this point.
- */
- fw_device_put(client->device);
- kfree(client);
+ client_put(client);
return 0;
}
--
Stefan Richter
-=====-==--= ---= --=--
http://arcgraph.de/sr/
next prev parent reply other threads:[~2009-01-04 15:25 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-01-04 15:23 [PATCH 00/11] firewire: cdev: proposed ABI extensions Stefan Richter
2009-01-04 15:24 ` Stefan Richter [this message]
2009-01-11 20:31 ` [PATCH 01/11] firewire: cdev: reference-count client instances David Moore
2009-01-04 15:25 ` [PATCH 02/11] firewire: cdev: unify names of struct types and of their instances Stefan Richter
2009-01-04 15:25 ` [PATCH 03/11] firewire: cdev: sort includes Stefan Richter
2009-01-04 15:26 ` [PATCH 04/11] firewire: core: topology header fix Stefan Richter
2009-01-04 15:26 ` [PATCH 05/11] firewire: cdev: add ioctls for isochronous resource management Stefan Richter
[not found] ` <1231404355.18613.68.camel@localhost.localdomain>
[not found] ` <4965D58E.1050606@s5r6.in-berlin.de>
[not found] ` <496648EC.3060806@s5r6.in-berlin.de>
2009-01-08 22:07 ` [PATCH] firewire: cdev: add ioctls for iso resource management, amendment Stefan Richter
2009-01-11 20:32 ` [PATCH 05/11] firewire: cdev: add ioctls for isochronous resource management David Moore
2009-01-04 15:27 ` [PATCH 06/11] firewire: cdev: add ioctls for manual iso " Stefan Richter
[not found] ` <1231643968.3538.59.camel@localhost.localdomain>
[not found] ` <1231656885.3538.67.camel@localhost.localdomain>
[not found] ` <4969CE1A.8010800@s5r6.in-berlin.de>
2009-01-11 12:44 ` [PATCH] firewire: cdev: simplify a schedule_delayed_work wrapper Stefan Richter
2009-01-04 15:28 ` [PATCH 07/11] firewire: cdev: add ioctl to query maximum transmission speed Stefan Richter
2009-01-04 15:29 ` [PATCH 08/11] firewire: cdev: add ioctl for broadcast write requests Stefan Richter
2009-01-04 15:30 ` [PATCH 09/11] firewire: cdev: restrict broadcast write requests to Units Space Stefan Richter
2009-01-04 15:30 ` [PATCH 10/11] firewire: cdev: extend transaction payload size check Stefan Richter
2009-01-04 15:31 ` [PATCH 11/11] firewire: cdev: replace some spin_lock_irqsave by spin_lock_irq Stefan Richter
2009-01-04 23:28 ` [PATCH 00/11] firewire: cdev: proposed ABI extensions Stefan Richter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=tkrat.8de770712ed54822@s5r6.in-berlin.de \
--to=stefanr@s5r6.in-berlin.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux1394-devel@lists.sourceforge.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.