[PATCH 0/4] firewire: cdev: code refactoring

[PATCH 0/4] firewire: cdev: code refactoring

[Takashi Sakamoto]

Hi,

The cdev implementation includes some useless locking, goto, and waking
up. This series arranges the implementation to remove them.

Takashi Sakamoto (4):
  firewire: cdev: remove unnecessary client locking for fw_device
    members
  firewire: cdev: wake up client from queue_event() only when any event
    is available
  firewire: cdev: use xchg() to exchange pointer value
  firewire: cdev: refactor event copying to remove goto statement

 drivers/firewire/core-cdev.c | 61 +++++++++++++++---------------------
 1 file changed, 26 insertions(+), 35 deletions(-)


base-commit: cc43480ac04cf909387d8b722f7491905ef7a7c3
-- 
2.53.0

[PATCH 1/4] firewire: cdev: remove unnecessary client locking for fw_device members

[Takashi Sakamoto]

Some accesses to fw_device members unnecessarily use the client-level
lock. Remove the locking from these accesses.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-cdev.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index a626468b4b0f..5020895c42a0 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1350,17 +1350,17 @@ static void iso_resource_auto_work(struct work_struct *work)
 	struct iso_resource_auto *r = from_work(r, work, work.work);
 	struct client *client = r->client;
 	unsigned long index = r->resource.handle;
-	int current_generation, resource_generation, channel, bandwidth, todo;
-	u64 reset_jiffies;
+	int channel, bandwidth, todo;
 	bool free;
 
-	scoped_guard(spinlock_irq, &client->lock) {
-		reset_jiffies = client->device->card->reset_jiffies;
-		current_generation = client->device->generation;
-		resource_generation = r->generation;
-		r->generation = current_generation;
+	u64 reset_jiffies = client->device->card->reset_jiffies;
+	int current_generation = client->device->generation;
+
+	int resource_generation = r->generation;
+	r->generation = current_generation;
+
+	scoped_guard(spinlock_irq, &client->lock)
 		todo = r->todo;
-	}
 
 	switch (todo) {
 	case ISO_RES_AUTO_ALLOC:
@@ -1514,12 +1514,10 @@ static void iso_resource_once_work(struct work_struct *work)
 	struct iso_resource_once *r = from_work(r, work, work);
 	struct client *client = r->client;
 	struct iso_resource_event *e = r->event;
-	int generation, channel, bandwidth;
-
-	scoped_guard(spinlock_irq, &client->lock)
-		generation = client->device->generation;
+	int channel;
 
-	bandwidth = r->params.bandwidth;
+	int generation = client->device->generation;
+	int bandwidth = r->params.bandwidth;
 
 	fw_iso_resource_manage(client->device->card, generation, r->params.channels_mask, &channel,
 			       &bandwidth, r->todo == ISO_RES_ONCE_ALLOC);
-- 
2.53.0

[PATCH 2/4] firewire: cdev: wake up client from queue_event() only when any event is available

[Takashi Sakamoto]

The in_shutdown member of client structure is set only when releasing
file descriptor, thus no need to wake up the client itself.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-cdev.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 5020895c42a0..9a539de9b0a1 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -338,13 +338,14 @@ static void queue_event(struct client *client, struct event *event,
 	event->v[1].size = size1;
 
 	scoped_guard(spinlock_irqsave, &client->lock) {
-		if (client->in_shutdown)
+		if (client->in_shutdown) {
 			kfree(event);
-		else
+		} else {
 			list_add_tail(&event->link, &client->event_list);
-	}
 
-	wake_up_interruptible(&client->wait);
+			wake_up_interruptible(&client->wait);
+		}
+	}
 }
 
 static int dequeue_event(struct client *client,
-- 
2.53.0

[PATCH 3/4] firewire: cdev: use xchg() to exchange pointer value

[Takashi Sakamoto]

xchg() is useful for exchanging a pointer and returning the old value,
even when atomicity is not required.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-cdev.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 9a539de9b0a1..73a940d40b39 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1357,8 +1357,7 @@ static void iso_resource_auto_work(struct work_struct *work)
 	u64 reset_jiffies = client->device->card->reset_jiffies;
 	int current_generation = client->device->generation;
 
-	int resource_generation = r->generation;
-	r->generation = current_generation;
+	int resource_generation = xchg(&r->generation, current_generation); // But no need to be atomic.
 
 	scoped_guard(spinlock_irq, &client->lock)
 		todo = r->todo;
@@ -1388,8 +1387,7 @@ static void iso_resource_auto_work(struct work_struct *work)
 
 	if (todo == ISO_RES_AUTO_DEALLOC) {
 		free = true;
-		e = r->e_dealloc;
-		r->e_dealloc = NULL;
+		e = xchg(&r->e_dealloc, NULL); // But no need to be atomic.
 	} else {
 		free = false;
 
@@ -1417,8 +1415,7 @@ static void iso_resource_auto_work(struct work_struct *work)
 				return;
 
 			// Notify the userspace client of the failure through a deallocation event.
-			e = r->e_dealloc;
-			r->e_dealloc = NULL;
+			e = xchg(&r->e_dealloc, NULL); // But no need to be atomic.
 		} else {
 			// Transit from allocation to reallocation, except if the client requested
 			// deallocation in the meantime.
@@ -1428,8 +1425,7 @@ static void iso_resource_auto_work(struct work_struct *work)
 			if (channel >= 0)
 				r->params.channels_mask = BIT_ULL(channel);
 
-			e = r->e_alloc;
-			r->e_alloc = NULL;
+			e = xchg(&r->e_alloc, NULL); // But no need to be atomic.
 		}
 	}
 
-- 
2.53.0

[PATCH 4/4] firewire: cdev: refactor event copying to remove goto statement

[Takashi Sakamoto]

Arrange the local variables to avoid a goto statement when copying events
to the userspace buffer.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-cdev.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 73a940d40b39..e017086ff641 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -348,12 +348,9 @@ static void queue_event(struct client *client, struct event *event,
 	}
 }
 
-static int dequeue_event(struct client *client,
-			 char __user *buffer, size_t count)
+static ssize_t dequeue_event(struct client *client, char __user *buffer, size_t count)
 {
 	struct event *event;
-	size_t size, total;
-	int i, ret;
 
 	// After the following block, the event pointer above is guaranteed to have a correct value.
 	{
@@ -378,18 +375,17 @@ static int dequeue_event(struct client *client,
 		spin_unlock_irq(&client->lock);
 	}
 
-	total = 0;
-	for (i = 0; i < ARRAY_SIZE(event->v) && total < count; i++) {
-		size = min(event->v[i].size, count - total);
-		if (copy_to_user(buffer + total, event->v[i].data, size)) {
+	ssize_t ret = 0;
+
+	for (int i = 0; i < ARRAY_SIZE(event->v) && ret < count; i++) {
+		size_t size = min(event->v[i].size, count - ret);
+		if (copy_to_user(buffer + ret, event->v[i].data, size)) {
 			ret = -EFAULT;
-			goto out;
+			break;
 		}
-		total += size;
+		ret += size;
 	}
-	ret = total;
 
- out:
 	kfree(event);
 
 	return ret;
-- 
2.53.0

Re: [PATCH 0/4] firewire: cdev: code refactoring

[Takashi Sakamoto]

On Sun, Sep 27, 2026 at 12:43:39PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> The cdev implementation includes some useless locking, goto, and waking
> up. This series arranges the implementation to remove them.
> 
> Takashi Sakamoto (4):
>   firewire: cdev: remove unnecessary client locking for fw_device
>     members
>   firewire: cdev: wake up client from queue_event() only when any event
>     is available
>   firewire: cdev: use xchg() to exchange pointer value
>   firewire: cdev: refactor event copying to remove goto statement
> 
>  drivers/firewire/core-cdev.c | 61 +++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 35 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto