linux1394-devel

Re: [PATCH 0/4] firewire: core: code refactoring for isoc resource management work in cdev layer

[Takashi S. <o-t...@sa...>]

On Fri, May 01, 2026 at 10:58:19PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> In the cdev layer of this subsystem, there are two ways to manage
> isochronous resources. My previous work separated the logic for these
> approaches[1]. However, there is still room to improve the current
> implementation, particularly in the code path that maintains
> isochronous resources managed by the kernel, where the current code can
> be simplified.
> 
> This patchset refactors the relevant code accordingly.
> 
> 
> [1] https://lore.kernel.org/lkml/202...@sa.../
> 
> Takashi Sakamoto (4):
>   firewire: core: reduce critical section duration in pre-processing of
>     isoc resource management in cdev
>   firewire: core: use switch statement for post-processing of isoc
>     resource management in cdev
>   firewire: core: refactor notification type determination after isoc
>     resource management in cdev
>   firewire: core: move allocation/reallocation paths into specific branch
>     after isoc resource management in cdev
> 
>  drivers/firewire/core-cdev.c | 115 +++++++++++++++++++----------------
>  1 file changed, 64 insertions(+), 51 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto

[PATCH 3/4] firewire: core: refactor notification type determination after isoc resource management in cdev

[Takashi S. <o-t...@sa...>]

After managing the actual isochronous resources, there is
post-processing logic to determine what type of event should be
notified. However, there is room for improvement.

This commit refactors the logic.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 0d57b61ade12..4ce8754da93f 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1390,20 +1390,27 @@ static void iso_resource_auto_work(struct work_struct *work)
 		}
 	}
 
-	if (todo == ISO_RES_AUTO_ALLOC && channel >= 0)
-		r->params.channels = 1ULL << channel;
-
-	if (todo == ISO_RES_AUTO_REALLOC && success)
-		goto out;
-
-	if (todo == ISO_RES_AUTO_ALLOC) {
-		e = r->e_alloc;
-		r->e_alloc = NULL;
-	} else {
+	if (todo == ISO_RES_AUTO_DEALLOC) {
 		free = true;
 		e = r->e_dealloc;
 		r->e_dealloc = NULL;
+	} else {
+		if (todo == ISO_RES_AUTO_REALLOC) {
+			if (success)
+				goto out;
+
+			// Notify the userspace client of the failure through a deallocation event.
+			e = r->e_dealloc;
+			r->e_dealloc = NULL;
+		} else {
+			if (channel >= 0)
+				r->params.channels = 1ULL << channel;
+
+			e = r->e_alloc;
+			r->e_alloc = NULL;
+		}
 	}
+
 	e->iso_resource.handle    = r->resource.handle;
 	e->iso_resource.channel   = channel;
 	e->iso_resource.bandwidth = bandwidth;
-- 
2.53.0

[PATCH 4/4] firewire: core: move allocation/reallocation paths into specific branch after isoc resource management in cdev

[Takashi S. <o-t...@sa...>]

After managing the actual isochronous resources, there is
post-processing logic to determine what type of event should be
notified. However, there is room for improvement.

This commit refactors the logic.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 53 ++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 4ce8754da93f..c166e7617d2a 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1331,7 +1331,7 @@ static void iso_resource_auto_work(struct work_struct *work)
 	unsigned long index = r->resource.handle;
 	int current_generation, resource_generation, channel, bandwidth, todo;
 	u64 reset_jiffies;
-	bool free = false, success;
+	bool free;
 
 	scoped_guard(spinlock_irq, &client->lock) {
 		reset_jiffies = client->device->card->reset_jiffies;
@@ -1364,37 +1364,31 @@ static void iso_resource_auto_work(struct work_struct *work)
 	fw_iso_resource_manage(client->device->card, current_generation, r->params.channels,
 			       &channel, &bandwidth, todo != ISO_RES_AUTO_DEALLOC);
 
-	/*
-	 * Is this generation outdated already?  As long as this resource sticks
-	 * in the xarray, it will be scheduled again for a newer generation or at
-	 * shutdown.
-	 */
-	if (channel == -EAGAIN &&
-	    (todo == ISO_RES_AUTO_ALLOC || todo == ISO_RES_AUTO_REALLOC))
-		goto out;
-
-	success = channel >= 0 || bandwidth > 0;
-
-	scoped_guard(spinlock_irq, &client->lock) {
-		// Transit from allocation to reallocation, except if the client
-		// requested deallocation in the meantime.
-		if (r->todo == ISO_RES_AUTO_ALLOC)
-			r->todo = ISO_RES_AUTO_REALLOC;
-		// Allocation or reallocation failure?  Pull this resource out of the
-		// xarray and prepare for deletion, unless the client is shutting down.
-		if (r->todo == ISO_RES_AUTO_REALLOC && !success &&
-		    !client->in_shutdown &&
-		    xa_erase(&client->resource_xa, index)) {
-			client_put(client);
-			free = true;
-		}
-	}
-
 	if (todo == ISO_RES_AUTO_DEALLOC) {
 		free = true;
 		e = r->e_dealloc;
 		r->e_dealloc = NULL;
 	} else {
+		free = false;
+
+		// Is this generation outdated already?  As long as this resource sticks in the
+		// xarray, it will be scheduled again for a newer generation or at shutdown.
+		if (channel == -EAGAIN)
+			goto out;
+
+		bool success = channel >= 0 || bandwidth > 0;
+
+		if (!success) {
+			// Allocation or reallocation failure?  Pull this resource out of the
+			// xarray and prepare for deletion, unless the client is shutting down.
+			scoped_guard(spinlock_irq,  &client->lock) {
+				if (!client->in_shutdown && xa_erase(&client->resource_xa, index)) {
+					client_put(client);
+					free = true;
+				}
+			}
+		}
+
 		if (todo == ISO_RES_AUTO_REALLOC) {
 			if (success)
 				goto out;
@@ -1403,6 +1397,11 @@ static void iso_resource_auto_work(struct work_struct *work)
 			e = r->e_dealloc;
 			r->e_dealloc = NULL;
 		} else {
+			// Transit from allocation to reallocation, except if the client requested
+			// deallocation in the meantime.
+			scoped_guard(spinlock_irq,  &client->lock)
+				r->todo = ISO_RES_AUTO_REALLOC;
+
 			if (channel >= 0)
 				r->params.channels = 1ULL << channel;
 
-- 
2.53.0

[PATCH 1/4] firewire: core: reduce critical section duration in pre-processing of isoc resource management in cdev

[Takashi S. <o-t...@sa...>]

It is preferable for the critical section to be as small as possible.
Current implementation of iso_resource_auto_work() function uses a
spinlock to control concurrent access to members of fw_card, fw_device,
iso_resource_auto structures, however the locking duration could be
reduced.

This commit refactors to shorten that duration.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index bcfb20b770df..887783e4bd52 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1329,32 +1329,36 @@ 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 generation, channel, bandwidth, todo;
+	int current_generation, resource_generation, channel, bandwidth, todo;
+	u64 reset_jiffies;
 	bool skip, free, success;
 
 	scoped_guard(spinlock_irq, &client->lock) {
-		generation = client->device->generation;
+		reset_jiffies = client->device->card->reset_jiffies;
+		current_generation = client->device->generation;
+		resource_generation = r->params.generation;
+		r->params.generation = current_generation;
 		todo = r->todo;
-		// Allow 1000ms grace period for other reallocations.
-		if (todo == ISO_RES_AUTO_ALLOC &&
-		    time_is_after_jiffies64(client->device->card->reset_jiffies + secs_to_jiffies(1))) {
-			schedule_iso_resource_auto(r, msecs_to_jiffies(333));
-			skip = true;
-		} else {
-			// We could be called twice within the same generation.
-			skip = todo == ISO_RES_AUTO_REALLOC &&
-			       r->params.generation == generation;
-		}
-		free = todo == ISO_RES_AUTO_DEALLOC;
-		r->params.generation = generation;
 	}
 
+	// Allow 1000ms grace period for other reallocations.
+	if (todo == ISO_RES_AUTO_ALLOC &&
+	    time_is_after_jiffies64(reset_jiffies + secs_to_jiffies(1))) {
+		schedule_iso_resource_auto(r, msecs_to_jiffies(333));
+		skip = true;
+	} else {
+		// We could be called twice within the same generation.
+		skip = todo == ISO_RES_AUTO_REALLOC &&
+		       resource_generation == current_generation;
+	}
+	free = todo == ISO_RES_AUTO_DEALLOC;
+
 	if (skip)
 		goto out;
 
 	bandwidth = r->params.bandwidth;
 
-	fw_iso_resource_manage(client->device->card, generation,
+	fw_iso_resource_manage(client->device->card, current_generation,
 			r->params.channels, &channel, &bandwidth,
 			todo == ISO_RES_AUTO_ALLOC ||
 			todo == ISO_RES_AUTO_REALLOC);
-- 
2.53.0

[PATCH 2/4] firewire: core: use switch statement for post-processing of isoc resource management in cdev

[Takashi S. <o-t...@sa...>]

The iso_resource_auto structure object has three states. The current
implementation of state evaluation before managing the actual isochronous
resources can be improved.

This commit refactors the evaluation logic using a switch statement.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 37 +++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 887783e4bd52..0d57b61ade12 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1331,7 +1331,7 @@ static void iso_resource_auto_work(struct work_struct *work)
 	unsigned long index = r->resource.handle;
 	int current_generation, resource_generation, channel, bandwidth, todo;
 	u64 reset_jiffies;
-	bool skip, free, success;
+	bool free = false, success;
 
 	scoped_guard(spinlock_irq, &client->lock) {
 		reset_jiffies = client->device->card->reset_jiffies;
@@ -1341,27 +1341,29 @@ static void iso_resource_auto_work(struct work_struct *work)
 		todo = r->todo;
 	}
 
-	// Allow 1000ms grace period for other reallocations.
-	if (todo == ISO_RES_AUTO_ALLOC &&
-	    time_is_after_jiffies64(reset_jiffies + secs_to_jiffies(1))) {
-		schedule_iso_resource_auto(r, msecs_to_jiffies(333));
-		skip = true;
-	} else {
+	switch (todo) {
+	case ISO_RES_AUTO_ALLOC:
+		// Allow 1000ms grace period for other reallocations.
+		if (time_is_after_jiffies64(reset_jiffies + secs_to_jiffies(1))) {
+			schedule_iso_resource_auto(r, msecs_to_jiffies(333));
+			goto out;
+		}
+		break;
+	case ISO_RES_AUTO_REALLOC:
 		// We could be called twice within the same generation.
-		skip = todo == ISO_RES_AUTO_REALLOC &&
-		       resource_generation == current_generation;
+		if (resource_generation == current_generation)
+			goto out;
+		break;
+	case ISO_RES_AUTO_DEALLOC:
+	default:
+		break;
 	}
-	free = todo == ISO_RES_AUTO_DEALLOC;
-
-	if (skip)
-		goto out;
 
 	bandwidth = r->params.bandwidth;
 
-	fw_iso_resource_manage(client->device->card, current_generation,
-			r->params.channels, &channel, &bandwidth,
-			todo == ISO_RES_AUTO_ALLOC ||
-			todo == ISO_RES_AUTO_REALLOC);
+	fw_iso_resource_manage(client->device->card, current_generation, r->params.channels,
+			       &channel, &bandwidth, todo != ISO_RES_AUTO_DEALLOC);
+
 	/*
 	 * Is this generation outdated already?  As long as this resource sticks
 	 * in the xarray, it will be scheduled again for a newer generation or at
@@ -1398,6 +1400,7 @@ static void iso_resource_auto_work(struct work_struct *work)
 		e = r->e_alloc;
 		r->e_alloc = NULL;
 	} else {
+		free = true;
 		e = r->e_dealloc;
 		r->e_dealloc = NULL;
 	}
-- 
2.53.0

[PATCH 0/4] firewire: core: code refactoring for isoc resource management work in cdev layer

[Takashi S. <o-t...@sa...>]

Hi,

In the cdev layer of this subsystem, there are two ways to manage
isochronous resources. My previous work separated the logic for these
approaches[1]. However, there is still room to improve the current
implementation, particularly in the code path that maintains
isochronous resources managed by the kernel, where the current code can
be simplified.

This patchset refactors the relevant code accordingly.


[1] https://lore.kernel.org/lkml/202...@sa.../

Takashi Sakamoto (4):
  firewire: core: reduce critical section duration in pre-processing of
    isoc resource management in cdev
  firewire: core: use switch statement for post-processing of isoc
    resource management in cdev
  firewire: core: refactor notification type determination after isoc
    resource management in cdev
  firewire: core: move allocation/reallocation paths into specific branch
    after isoc resource management in cdev

 drivers/firewire/core-cdev.c | 115 +++++++++++++++++++----------------
 1 file changed, 64 insertions(+), 51 deletions(-)


base-commit: 6dbe7653fa01edeefc77b4d7c063562eb3debd48
-- 
2.53.0

Re: [PATCH 0/7] firewire: core: separate iso_resource paths

[Takashi S. <o-t...@sa...>]

On Wed, Apr 29, 2026 at 06:34:41PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> (Repost since lkml was excluded.)
> 
> Dingisoul has reported that a case where the reference count of a
> client structure is leaked when handling iso_resource in cdev layer[1].
> Fixing the bug immediately s difficult due to the complexity of
> per-client resource lifetime.
> 
> As a first step toward addressing this issue, this patchset refactors the
> existing code for isochronous resource operation. Userspace application
> can allocate and deallocate isochronous resources on IEEE 1394 bus in two
> ways:
>  * FW_CDEV_IOC_[DE]ALLOCATE_ISO_RESOURCE
>  * FW_CDEV_IOC_[DE]ALLOCATE_ISO_RESOURCE_ONCE
> 
> With the former, the application delegates the maintenance of the
> allocated isochronous resources to kernel and obtain a handle for the
> client resource. With the latter, the application should maintain
> isochronous resources every time receiving bus reset event, without
> relying on a handle.
> 
> Currently, both  operations are handled by the same code, although they
> differ in terms of client resource management.
> 
> This patchset separates these two paths. As a result, it becomes clear
> that the reported issue only affects client resource allocated via the
> former method. While the actual bug fix is deferred, this refactoring
> lays the groundwork for it.
> 
> [1] https://sourceforge.net/p/linux1394/mailman/linux1394-devel/thread/20260404110936.GA282614%40sakamocchi.jp/#msg59317811
> 
> Takashi Sakamoto (7):
>   firewire: core: code refactoring for early return at client resource
>     allocation
>   firewire: core: code refactoring to queue work item for iso_resource
>   firewire: core: code refactoring for helper function to fill
>     iso_resource parameters
>   firewire: core: split functions for iso_resource once operation
>   firewire: core: code cleanup to remove old implementations for once
>     operation
>   firewire: core: append _auto suffix for non-once iso resource
>     operations
>   firewire: core: code cleanup for iso resource auto creation
> 
>  drivers/firewire/core-cdev.c | 285 +++++++++++++++++++++--------------
>  1 file changed, 176 insertions(+), 109 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto

[PATCH 7/7] firewire: core: code cleanup for iso resource auto creation

[Takashi S. <o-t...@sa...>]

The init_iso_resource function is only called by
ioctl_allocate_iso_resource(), thus no need to be unique.

This commit unifies them with minor code refactoring.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 53 ++++++++++++++----------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index b3ce34d777c3..bcfb20b770df 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1424,23 +1424,20 @@ static void release_iso_resource_auto(struct client *client, struct client_resou
 	schedule_iso_resource_auto(r, 0);
 }
 
-static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_resource *request)
+static int ioctl_allocate_iso_resource(struct client *client, union ioctl_arg *arg)
 {
-	struct iso_resource_event *e1, *e2;
-	struct iso_resource_auto *r;
-	int ret;
+	struct fw_cdev_allocate_iso_resource *request = &arg->allocate_iso_resource;
+	struct iso_resource_event *e1 __free(kfree) = kmalloc_obj(*e1);
+	struct iso_resource_event *e2 __free(kfree) = kmalloc_obj(*e2);
+	struct iso_resource_auto *r  __free(kfree) = kmalloc_obj(*r);
+	int err;
 
-	r = kmalloc_obj(*r);
-	e1 = kmalloc_obj(*e1);
-	e2 = kmalloc_obj(*e2);
-	if (r == NULL || e1 == NULL || e2 == NULL) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!r || !e1 || !e2)
+		return -ENOMEM;
 
-	ret = fill_iso_resource_params(&r->params, request);
-	if (ret < 0)
-		goto fail;
+	err = fill_iso_resource_params(&r->params, request);
+	if (err < 0)
+		return  err;
 
 	INIT_DELAYED_WORK(&r->work, iso_resource_auto_work);
 	r->client	= client;
@@ -1449,31 +1446,21 @@ static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_
 	r->e_dealloc	= e2;
 
 	e1->iso_resource.closure = request->closure;
-	e1->iso_resource.type    = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED;
+	e1->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED;
 	e2->iso_resource.closure = request->closure;
-	e2->iso_resource.type    = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
+	e2->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
 
 	r->resource.release = release_iso_resource_auto;
-	ret = add_client_resource(client, &r->resource, GFP_KERNEL);
-	if (ret < 0)
-		goto fail;
-	schedule_iso_resource_auto(r, 0);
-
+	err = add_client_resource(client, &r->resource, GFP_KERNEL);
+	if (err < 0)
+		return err;
 	request->handle = r->resource.handle;
 
-	return 0;
- fail:
-	kfree(r);
-	kfree(e1);
-	kfree(e2);
-
-	return ret;
-}
+	retain_and_null_ptr(e1);
+	retain_and_null_ptr(e2);
+	schedule_iso_resource_auto(no_free_ptr(r), 0);
 
-static int ioctl_allocate_iso_resource(struct client *client,
-				       union ioctl_arg *arg)
-{
-	return init_iso_resource(client, &arg->allocate_iso_resource);
+	return 0;
 }
 
 static int ioctl_deallocate_iso_resource(struct client *client,
-- 
2.53.0

[PATCH 4/7] firewire: core: split functions for iso_resource once operation

[Takashi S. <o-t...@sa...>]

Unlike FW_CDEV_IOC_ALLOCATE_ISO_RESOURCE operation, the operations of
FW_CDEV_IOC_[DE]ALLOCATE_ISO_RESOURCE_ONCE require no client resource,
thus they keeps no handle value.

This commit adds the series of functions to separate these operations,
according to divide-and-conquer methodology.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 83 ++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 4 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index effa03739679..478e8f6400f0 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -145,6 +145,18 @@ struct iso_resource {
 	struct iso_resource_event *e_alloc, *e_dealloc;
 };
 
+struct iso_resource_once {
+	struct client *client;
+	// Schedule work and access todo only with client->lock held.
+	struct delayed_work work;
+	enum {
+		ISO_RES_ONCE_ALLOC,
+		ISO_RES_ONCE_DEALLOC,
+	} todo;
+	struct iso_resource_params params;
+	struct iso_resource_event *event;
+};
+
 static struct address_handler_resource *to_address_handler_resource(struct client_resource *resource)
 {
 	return container_of(resource, struct address_handler_resource, resource);
@@ -1479,18 +1491,81 @@ static int ioctl_deallocate_iso_resource(struct client *client,
 			arg->deallocate.handle, release_iso_resource, NULL);
 }
 
+#define UNAVAILABLE_HANDLE	-1
+
+static void iso_resource_once_work(struct work_struct *work)
+{
+	struct iso_resource_once *r = from_work(r, work, 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;
+
+	r->params.generation = generation;
+	bandwidth = r->params.bandwidth;
+
+	fw_iso_resource_manage(client->device->card, generation, r->params.channels, &channel,
+			       &bandwidth, r->todo == ISO_RES_ONCE_ALLOC);
+
+	e->iso_resource.handle = UNAVAILABLE_HANDLE;
+	e->iso_resource.channel = channel;
+	e->iso_resource.bandwidth = bandwidth;
+
+	queue_event(client, &e->event, &e->iso_resource, sizeof(e->iso_resource), NULL, 0);
+
+	cancel_delayed_work(&r->work);
+	kfree(r);
+
+	client_put(client);
+}
+
+static int init_iso_resource_once(struct client *client,
+				  struct fw_cdev_allocate_iso_resource *request, int todo)
+{
+	struct iso_resource_event *e __free(kfree) = kmalloc_obj(*e);
+	struct iso_resource_once *r __free(kfree) = kmalloc_obj(*r);
+	int err;
+
+	if (!r || !e)
+		return -ENOMEM;
+
+	err = fill_iso_resource_params(&r->params, request);
+	if (err < 0)
+		return err;
+
+	INIT_DELAYED_WORK(&r->work, iso_resource_once_work);
+	r->client = client;
+	r->todo	= todo;
+
+	if (todo == ISO_RES_ONCE_ALLOC)
+		e->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED;
+	else
+		e->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
+	e->iso_resource.closure = request->closure;
+	r->event = no_free_ptr(e);
+
+	// Keep the client until work item finishing.
+	client_get(r->client);
+
+	queue_delayed_work(fw_workqueue, &no_free_ptr(r)->work, 0);
+
+	request->handle = UNAVAILABLE_HANDLE;
+
+	return 0;
+}
+
 static int ioctl_allocate_iso_resource_once(struct client *client,
 					    union ioctl_arg *arg)
 {
-	return init_iso_resource(client,
-			&arg->allocate_iso_resource, ISO_RES_ALLOC_ONCE);
+	return init_iso_resource_once(client, &arg->allocate_iso_resource, ISO_RES_ONCE_ALLOC);
 }
 
 static int ioctl_deallocate_iso_resource_once(struct client *client,
 					      union ioctl_arg *arg)
 {
-	return init_iso_resource(client,
-			&arg->allocate_iso_resource, ISO_RES_DEALLOC_ONCE);
+	return init_iso_resource_once(client, &arg->allocate_iso_resource, ISO_RES_ONCE_DEALLOC);
 }
 
 /*
-- 
2.53.0

[PATCH 6/7] firewire: core: append _auto suffix for non-once iso resource operations

[Takashi S. <o-t...@sa...>]

The functions for iso_resource once operations are carefully split from
another type of operation.

This commit adds _auto suffix to functions for the another type so that
it is easily to distinguish them.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 75 ++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 38 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index f81a8aa4bcbc..b3ce34d777c3 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -134,15 +134,15 @@ struct iso_resource_params {
 	s32 bandwidth;
 };
 
-struct iso_resource {
+struct iso_resource_auto {
 	struct client_resource resource;
 	struct client *client;
 	/* Schedule work and access todo only with client->lock held. */
 	struct delayed_work work;
 	enum {
-		ISO_RES_ALLOC,
-		ISO_RES_REALLOC,
-		ISO_RES_DEALLOC,
+		ISO_RES_AUTO_ALLOC,
+		ISO_RES_AUTO_REALLOC,
+		ISO_RES_AUTO_DEALLOC,
 	} todo;
 	struct iso_resource_params params;
 	struct iso_resource_event *e_alloc, *e_dealloc;
@@ -175,16 +175,16 @@ static struct descriptor_resource *to_descriptor_resource(struct client_resource
 	return container_of(resource, struct descriptor_resource, resource);
 }
 
-static struct iso_resource *to_iso_resource(struct client_resource *resource)
+static struct iso_resource_auto *to_iso_resource_auto(struct client_resource *resource)
 {
-	return container_of(resource, struct iso_resource, resource);
+	return container_of(resource, struct iso_resource_auto, resource);
 }
 
-static void release_iso_resource(struct client *, struct client_resource *);
+static void release_iso_resource_auto(struct client *, struct client_resource *);
 
-static int is_iso_resource(const struct client_resource *resource)
+static int is_iso_resource_auto(const struct client_resource *resource)
 {
-	return resource->release == release_iso_resource;
+	return resource->release == release_iso_resource_auto;
 }
 
 static void release_transaction(struct client *client,
@@ -195,7 +195,7 @@ static int is_outbound_transaction_resource(const struct client_resource *resour
 	return resource->release == release_transaction;
 }
 
-static void schedule_iso_resource(struct iso_resource *r, unsigned long delay)
+static void schedule_iso_resource_auto(struct iso_resource_auto *r, unsigned long delay)
 {
 	client_get(r->client);
 	if (!queue_delayed_work(fw_workqueue, &r->work, delay))
@@ -443,8 +443,8 @@ static void queue_bus_reset_event(struct client *client)
 	guard(spinlock_irq)(&client->lock);
 
 	xa_for_each(&client->resource_xa, index, resource) {
-		if (is_iso_resource(resource))
-			schedule_iso_resource(to_iso_resource(resource), 0);
+		if (is_iso_resource_auto(resource))
+			schedule_iso_resource_auto(to_iso_resource_auto(resource), 0);
 	}
 }
 
@@ -1323,10 +1323,10 @@ static int fill_iso_resource_params(struct iso_resource_params *params,
 	return 0;
 }
 
-static void iso_resource_work(struct work_struct *work)
+static void iso_resource_auto_work(struct work_struct *work)
 {
 	struct iso_resource_event *e;
-	struct iso_resource *r = from_work(r, work, work.work);
+	struct iso_resource_auto *r = from_work(r, work, work.work);
 	struct client *client = r->client;
 	unsigned long index = r->resource.handle;
 	int generation, channel, bandwidth, todo;
@@ -1336,16 +1336,16 @@ static void iso_resource_work(struct work_struct *work)
 		generation = client->device->generation;
 		todo = r->todo;
 		// Allow 1000ms grace period for other reallocations.
-		if (todo == ISO_RES_ALLOC &&
+		if (todo == ISO_RES_AUTO_ALLOC &&
 		    time_is_after_jiffies64(client->device->card->reset_jiffies + secs_to_jiffies(1))) {
-			schedule_iso_resource(r, msecs_to_jiffies(333));
+			schedule_iso_resource_auto(r, msecs_to_jiffies(333));
 			skip = true;
 		} else {
 			// We could be called twice within the same generation.
-			skip = todo == ISO_RES_REALLOC &&
+			skip = todo == ISO_RES_AUTO_REALLOC &&
 			       r->params.generation == generation;
 		}
-		free = todo == ISO_RES_DEALLOC;
+		free = todo == ISO_RES_AUTO_DEALLOC;
 		r->params.generation = generation;
 	}
 
@@ -1356,15 +1356,15 @@ static void iso_resource_work(struct work_struct *work)
 
 	fw_iso_resource_manage(client->device->card, generation,
 			r->params.channels, &channel, &bandwidth,
-			todo == ISO_RES_ALLOC ||
-			todo == ISO_RES_REALLOC);
+			todo == ISO_RES_AUTO_ALLOC ||
+			todo == ISO_RES_AUTO_REALLOC);
 	/*
 	 * Is this generation outdated already?  As long as this resource sticks
 	 * in the xarray, it will be scheduled again for a newer generation or at
 	 * shutdown.
 	 */
 	if (channel == -EAGAIN &&
-	    (todo == ISO_RES_ALLOC || todo == ISO_RES_REALLOC))
+	    (todo == ISO_RES_AUTO_ALLOC || todo == ISO_RES_AUTO_REALLOC))
 		goto out;
 
 	success = channel >= 0 || bandwidth > 0;
@@ -1372,11 +1372,11 @@ static void iso_resource_work(struct work_struct *work)
 	scoped_guard(spinlock_irq, &client->lock) {
 		// Transit from allocation to reallocation, except if the client
 		// requested deallocation in the meantime.
-		if (r->todo == ISO_RES_ALLOC)
-			r->todo = ISO_RES_REALLOC;
+		if (r->todo == ISO_RES_AUTO_ALLOC)
+			r->todo = ISO_RES_AUTO_REALLOC;
 		// Allocation or reallocation failure?  Pull this resource out of the
 		// xarray and prepare for deletion, unless the client is shutting down.
-		if (r->todo == ISO_RES_REALLOC && !success &&
+		if (r->todo == ISO_RES_AUTO_REALLOC && !success &&
 		    !client->in_shutdown &&
 		    xa_erase(&client->resource_xa, index)) {
 			client_put(client);
@@ -1384,13 +1384,13 @@ static void iso_resource_work(struct work_struct *work)
 		}
 	}
 
-	if (todo == ISO_RES_ALLOC && channel >= 0)
+	if (todo == ISO_RES_AUTO_ALLOC && channel >= 0)
 		r->params.channels = 1ULL << channel;
 
-	if (todo == ISO_RES_REALLOC && success)
+	if (todo == ISO_RES_AUTO_REALLOC && success)
 		goto out;
 
-	if (todo == ISO_RES_ALLOC) {
+	if (todo == ISO_RES_AUTO_ALLOC) {
 		e = r->e_alloc;
 		r->e_alloc = NULL;
 	} else {
@@ -1414,21 +1414,20 @@ static void iso_resource_work(struct work_struct *work)
 	client_put(client);
 }
 
-static void release_iso_resource(struct client *client,
-				 struct client_resource *resource)
+static void release_iso_resource_auto(struct client *client, struct client_resource *resource)
 {
-	struct iso_resource *r = to_iso_resource(resource);
+	struct iso_resource_auto *r = to_iso_resource_auto(resource);
 
 	guard(spinlock_irq)(&client->lock);
 
-	r->todo = ISO_RES_DEALLOC;
-	schedule_iso_resource(r, 0);
+	r->todo = ISO_RES_AUTO_DEALLOC;
+	schedule_iso_resource_auto(r, 0);
 }
 
 static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_resource *request)
 {
 	struct iso_resource_event *e1, *e2;
-	struct iso_resource *r;
+	struct iso_resource_auto *r;
 	int ret;
 
 	r = kmalloc_obj(*r);
@@ -1443,9 +1442,9 @@ static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_
 	if (ret < 0)
 		goto fail;
 
-	INIT_DELAYED_WORK(&r->work, iso_resource_work);
+	INIT_DELAYED_WORK(&r->work, iso_resource_auto_work);
 	r->client	= client;
-	r->todo		= ISO_RES_ALLOC;
+	r->todo		= ISO_RES_AUTO_ALLOC;
 	r->e_alloc	= e1;
 	r->e_dealloc	= e2;
 
@@ -1454,11 +1453,11 @@ static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_
 	e2->iso_resource.closure = request->closure;
 	e2->iso_resource.type    = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
 
-	r->resource.release = release_iso_resource;
+	r->resource.release = release_iso_resource_auto;
 	ret = add_client_resource(client, &r->resource, GFP_KERNEL);
 	if (ret < 0)
 		goto fail;
-	schedule_iso_resource(r, 0);
+	schedule_iso_resource_auto(r, 0);
 
 	request->handle = r->resource.handle;
 
@@ -1481,7 +1480,7 @@ static int ioctl_deallocate_iso_resource(struct client *client,
 					 union ioctl_arg *arg)
 {
 	return release_client_resource(client,
-			arg->deallocate.handle, release_iso_resource, NULL);
+			arg->deallocate.handle, release_iso_resource_auto, NULL);
 }
 
 #define UNAVAILABLE_HANDLE	-1
-- 
2.53.0

[PATCH 5/7] firewire: core: code cleanup to remove old implementations for once operation

[Takashi S. <o-t...@sa...>]

The helper functions for iso_resource allocation and work item still
include codes for once operation.

This commit refactors them to remove the old implementations.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 37 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 478e8f6400f0..f81a8aa4bcbc 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -139,8 +139,11 @@ struct iso_resource {
 	struct client *client;
 	/* Schedule work and access todo only with client->lock held. */
 	struct delayed_work work;
-	enum {ISO_RES_ALLOC, ISO_RES_REALLOC, ISO_RES_DEALLOC,
-	      ISO_RES_ALLOC_ONCE, ISO_RES_DEALLOC_ONCE,} todo;
+	enum {
+		ISO_RES_ALLOC,
+		ISO_RES_REALLOC,
+		ISO_RES_DEALLOC,
+	} todo;
 	struct iso_resource_params params;
 	struct iso_resource_event *e_alloc, *e_dealloc;
 };
@@ -1342,9 +1345,7 @@ static void iso_resource_work(struct work_struct *work)
 			skip = todo == ISO_RES_REALLOC &&
 			       r->params.generation == generation;
 		}
-		free = todo == ISO_RES_DEALLOC ||
-		       todo == ISO_RES_ALLOC_ONCE ||
-		       todo == ISO_RES_DEALLOC_ONCE;
+		free = todo == ISO_RES_DEALLOC;
 		r->params.generation = generation;
 	}
 
@@ -1356,8 +1357,7 @@ static void iso_resource_work(struct work_struct *work)
 	fw_iso_resource_manage(client->device->card, generation,
 			r->params.channels, &channel, &bandwidth,
 			todo == ISO_RES_ALLOC ||
-			todo == ISO_RES_REALLOC ||
-			todo == ISO_RES_ALLOC_ONCE);
+			todo == ISO_RES_REALLOC);
 	/*
 	 * Is this generation outdated already?  As long as this resource sticks
 	 * in the xarray, it will be scheduled again for a newer generation or at
@@ -1390,7 +1390,7 @@ static void iso_resource_work(struct work_struct *work)
 	if (todo == ISO_RES_REALLOC && success)
 		goto out;
 
-	if (todo == ISO_RES_ALLOC || todo == ISO_RES_ALLOC_ONCE) {
+	if (todo == ISO_RES_ALLOC) {
 		e = r->e_alloc;
 		r->e_alloc = NULL;
 	} else {
@@ -1425,8 +1425,7 @@ static void release_iso_resource(struct client *client,
 	schedule_iso_resource(r, 0);
 }
 
-static int init_iso_resource(struct client *client,
-		struct fw_cdev_allocate_iso_resource *request, int todo)
+static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_resource *request)
 {
 	struct iso_resource_event *e1, *e2;
 	struct iso_resource *r;
@@ -1446,7 +1445,7 @@ static int init_iso_resource(struct client *client,
 
 	INIT_DELAYED_WORK(&r->work, iso_resource_work);
 	r->client	= client;
-	r->todo		= todo;
+	r->todo		= ISO_RES_ALLOC;
 	r->e_alloc	= e1;
 	r->e_dealloc	= e2;
 
@@ -1455,15 +1454,10 @@ static int init_iso_resource(struct client *client,
 	e2->iso_resource.closure = request->closure;
 	e2->iso_resource.type    = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
 
-	if (todo == ISO_RES_ALLOC) {
-		r->resource.release = release_iso_resource;
-		ret = add_client_resource(client, &r->resource, GFP_KERNEL);
-		if (ret < 0)
-			goto fail;
-	} else {
-		r->resource.release = NULL;
-		r->resource.handle = -1;
-	}
+	r->resource.release = release_iso_resource;
+	ret = add_client_resource(client, &r->resource, GFP_KERNEL);
+	if (ret < 0)
+		goto fail;
 	schedule_iso_resource(r, 0);
 
 	request->handle = r->resource.handle;
@@ -1480,8 +1474,7 @@ static int init_iso_resource(struct client *client,
 static int ioctl_allocate_iso_resource(struct client *client,
 				       union ioctl_arg *arg)
 {
-	return init_iso_resource(client,
-			&arg->allocate_iso_resource, ISO_RES_ALLOC);
+	return init_iso_resource(client, &arg->allocate_iso_resource);
 }
 
 static int ioctl_deallocate_iso_resource(struct client *client,
-- 
2.53.0

[PATCH 3/7] firewire: core: code refactoring for helper function to fill iso_resource parameters

[Takashi S. <o-t...@sa...>]

This change is a preparation for future changes. The added helper function
will be reused in the changes to fill iso_resource parameters according to
the users' request.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 45 ++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 8391c7efab2c..effa03739679 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -128,6 +128,12 @@ struct descriptor_resource {
 	u32 data[];
 };
 
+struct iso_resource_params {
+	int generation;
+	u64 channels;
+	s32 bandwidth;
+};
+
 struct iso_resource {
 	struct client_resource resource;
 	struct client *client;
@@ -135,9 +141,7 @@ struct iso_resource {
 	struct delayed_work work;
 	enum {ISO_RES_ALLOC, ISO_RES_REALLOC, ISO_RES_DEALLOC,
 	      ISO_RES_ALLOC_ONCE, ISO_RES_DEALLOC_ONCE,} todo;
-	int generation;
-	u64 channels;
-	s32 bandwidth;
+	struct iso_resource_params params;
 	struct iso_resource_event *e_alloc, *e_dealloc;
 };
 
@@ -1290,6 +1294,20 @@ static int ioctl_get_cycle_timer(struct client *client, union ioctl_arg *arg)
 	return 0;
 }
 
+static int fill_iso_resource_params(struct iso_resource_params *params,
+				    struct fw_cdev_allocate_iso_resource *request)
+{
+	if ((request->channels == 0 && request->bandwidth == 0) ||
+	    request->bandwidth > BANDWIDTH_AVAILABLE_INITIAL)
+		return -EINVAL;
+
+	params->generation = -1;
+	params->channels = request->channels;
+	params->bandwidth = request->bandwidth;
+
+	return 0;
+}
+
 static void iso_resource_work(struct work_struct *work)
 {
 	struct iso_resource_event *e;
@@ -1310,21 +1328,21 @@ static void iso_resource_work(struct work_struct *work)
 		} else {
 			// We could be called twice within the same generation.
 			skip = todo == ISO_RES_REALLOC &&
-			       r->generation == generation;
+			       r->params.generation == generation;
 		}
 		free = todo == ISO_RES_DEALLOC ||
 		       todo == ISO_RES_ALLOC_ONCE ||
 		       todo == ISO_RES_DEALLOC_ONCE;
-		r->generation = generation;
+		r->params.generation = generation;
 	}
 
 	if (skip)
 		goto out;
 
-	bandwidth = r->bandwidth;
+	bandwidth = r->params.bandwidth;
 
 	fw_iso_resource_manage(client->device->card, generation,
-			r->channels, &channel, &bandwidth,
+			r->params.channels, &channel, &bandwidth,
 			todo == ISO_RES_ALLOC ||
 			todo == ISO_RES_REALLOC ||
 			todo == ISO_RES_ALLOC_ONCE);
@@ -1355,7 +1373,7 @@ static void iso_resource_work(struct work_struct *work)
 	}
 
 	if (todo == ISO_RES_ALLOC && channel >= 0)
-		r->channels = 1ULL << channel;
+		r->params.channels = 1ULL << channel;
 
 	if (todo == ISO_RES_REALLOC && success)
 		goto out;
@@ -1402,10 +1420,6 @@ static int init_iso_resource(struct client *client,
 	struct iso_resource *r;
 	int ret;
 
-	if ((request->channels == 0 && request->bandwidth == 0) ||
-	    request->bandwidth > BANDWIDTH_AVAILABLE_INITIAL)
-		return -EINVAL;
-
 	r = kmalloc_obj(*r);
 	e1 = kmalloc_obj(*e1);
 	e2 = kmalloc_obj(*e2);
@@ -1414,12 +1428,13 @@ static int init_iso_resource(struct client *client,
 		goto fail;
 	}
 
+	ret = fill_iso_resource_params(&r->params, request);
+	if (ret < 0)
+		goto fail;
+
 	INIT_DELAYED_WORK(&r->work, iso_resource_work);
 	r->client	= client;
 	r->todo		= todo;
-	r->generation	= -1;
-	r->channels	= request->channels;
-	r->bandwidth	= request->bandwidth;
 	r->e_alloc	= e1;
 	r->e_dealloc	= e2;
 
-- 
2.53.0

[PATCH 2/7] firewire: core: code refactoring to queue work item for iso_resource

[Takashi S. <o-t...@sa...>]

The add_client_resource() function checks the type of client resource
every time to be called. If the type is for iso_resource, it schedules
work item.

However, the iso_resource client resource is only added by the call of
init_iso_resource(). There is no need to check the type every time adding
any client resource.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 144625c34be2..8391c7efab2c 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -526,8 +526,6 @@ static int add_client_resource(struct client *client, struct client_resource *re
 
 		resource->handle = index;
 		client_get(client);
-		if (is_iso_resource(resource))
-			schedule_iso_resource(to_iso_resource(resource), 0);
 	}
 
 	return 0;
@@ -1438,8 +1436,9 @@ static int init_iso_resource(struct client *client,
 	} else {
 		r->resource.release = NULL;
 		r->resource.handle = -1;
-		schedule_iso_resource(r, 0);
 	}
+	schedule_iso_resource(r, 0);
+
 	request->handle = r->resource.handle;
 
 	return 0;
-- 
2.53.0

[PATCH 1/7] firewire: core: code refactoring for early return at client resource allocation

[Takashi S. <o-t...@sa...>]

The add_client_resource() function returns zero at success or negative
value at error. The critical section is already protected by
scoped_guard() macro. In this case, the programming pattern of early
return improves code readability.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index f791db4c8dff..144625c34be2 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -507,31 +507,30 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 static int add_client_resource(struct client *client, struct client_resource *resource,
 			       gfp_t gfp_mask)
 {
-	int ret;
-
 	scoped_guard(spinlock_irqsave, &client->lock) {
 		u32 index;
+		int ret;
+
+		if (client->in_shutdown)
+			return  -ECANCELED;
 
-		if (client->in_shutdown) {
-			ret = -ECANCELED;
+		if (gfpflags_allow_blocking(gfp_mask)) {
+			ret = xa_alloc(&client->resource_xa, &index, resource, xa_limit_32b,
+				       GFP_NOWAIT);
 		} else {
-			if (gfpflags_allow_blocking(gfp_mask)) {
-				ret = xa_alloc(&client->resource_xa, &index, resource, xa_limit_32b,
-					       GFP_NOWAIT);
-			} else {
-				ret = xa_alloc_bh(&client->resource_xa, &index, resource,
-						  xa_limit_32b, GFP_NOWAIT);
-			}
-		}
-		if (ret >= 0) {
-			resource->handle = index;
-			client_get(client);
-			if (is_iso_resource(resource))
-				schedule_iso_resource(to_iso_resource(resource), 0);
+			ret = xa_alloc_bh(&client->resource_xa, &index, resource,
+					  xa_limit_32b, GFP_NOWAIT);
 		}
+		if (ret < 0)
+			return ret;
+
+		resource->handle = index;
+		client_get(client);
+		if (is_iso_resource(resource))
+			schedule_iso_resource(to_iso_resource(resource), 0);
 	}
 
-	return ret < 0 ? ret : 0;
+	return 0;
 }
 
 static int release_client_resource(struct client *client, u32 handle,
-- 
2.53.0

[PATCH 0/7] firewire: core: separate iso_resource paths

[Takashi S. <o-t...@sa...>]

Hi,

(Repost since lkml was excluded.)

Dingisoul has reported that a case where the reference count of a
client structure is leaked when handling iso_resource in cdev layer[1].
Fixing the bug immediately s difficult due to the complexity of
per-client resource lifetime.

As a first step toward addressing this issue, this patchset refactors the
existing code for isochronous resource operation. Userspace application
can allocate and deallocate isochronous resources on IEEE 1394 bus in two
ways:
 * FW_CDEV_IOC_[DE]ALLOCATE_ISO_RESOURCE
 * FW_CDEV_IOC_[DE]ALLOCATE_ISO_RESOURCE_ONCE

With the former, the application delegates the maintenance of the
allocated isochronous resources to kernel and obtain a handle for the
client resource. With the latter, the application should maintain
isochronous resources every time receiving bus reset event, without
relying on a handle.

Currently, both  operations are handled by the same code, although they
differ in terms of client resource management.

This patchset separates these two paths. As a result, it becomes clear
that the reported issue only affects client resource allocated via the
former method. While the actual bug fix is deferred, this refactoring
lays the groundwork for it.

[1] https://sourceforge.net/p/linux1394/mailman/linux1394-devel/thread/20260404110936.GA282614%40sakamocchi.jp/#msg59317811

Takashi Sakamoto (7):
  firewire: core: code refactoring for early return at client resource
    allocation
  firewire: core: code refactoring to queue work item for iso_resource
  firewire: core: code refactoring for helper function to fill
    iso_resource parameters
  firewire: core: split functions for iso_resource once operation
  firewire: core: code cleanup to remove old implementations for once
    operation
  firewire: core: append _auto suffix for non-once iso resource
    operations
  firewire: core: code cleanup for iso resource auto creation

 drivers/firewire/core-cdev.c | 285 +++++++++++++++++++++--------------
 1 file changed, 176 insertions(+), 109 deletions(-)


base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
-- 
2.53.0

[PATCH 7/7] firewire: core: code cleanup for iso resource auto creation

[Takashi S. <o-t...@sa...>]

The init_iso_resource function is only called by
ioctl_allocate_iso_resource(), thus no need to be unique.

This commit unifies them with minor code refactoring.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 53 ++++++++++++++----------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index b3ce34d777c3..bcfb20b770df 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1424,23 +1424,20 @@ static void release_iso_resource_auto(struct client *client, struct client_resou
 	schedule_iso_resource_auto(r, 0);
 }
 
-static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_resource *request)
+static int ioctl_allocate_iso_resource(struct client *client, union ioctl_arg *arg)
 {
-	struct iso_resource_event *e1, *e2;
-	struct iso_resource_auto *r;
-	int ret;
+	struct fw_cdev_allocate_iso_resource *request = &arg->allocate_iso_resource;
+	struct iso_resource_event *e1 __free(kfree) = kmalloc_obj(*e1);
+	struct iso_resource_event *e2 __free(kfree) = kmalloc_obj(*e2);
+	struct iso_resource_auto *r  __free(kfree) = kmalloc_obj(*r);
+	int err;
 
-	r = kmalloc_obj(*r);
-	e1 = kmalloc_obj(*e1);
-	e2 = kmalloc_obj(*e2);
-	if (r == NULL || e1 == NULL || e2 == NULL) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!r || !e1 || !e2)
+		return -ENOMEM;
 
-	ret = fill_iso_resource_params(&r->params, request);
-	if (ret < 0)
-		goto fail;
+	err = fill_iso_resource_params(&r->params, request);
+	if (err < 0)
+		return  err;
 
 	INIT_DELAYED_WORK(&r->work, iso_resource_auto_work);
 	r->client	= client;
@@ -1449,31 +1446,21 @@ static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_
 	r->e_dealloc	= e2;
 
 	e1->iso_resource.closure = request->closure;
-	e1->iso_resource.type    = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED;
+	e1->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED;
 	e2->iso_resource.closure = request->closure;
-	e2->iso_resource.type    = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
+	e2->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
 
 	r->resource.release = release_iso_resource_auto;
-	ret = add_client_resource(client, &r->resource, GFP_KERNEL);
-	if (ret < 0)
-		goto fail;
-	schedule_iso_resource_auto(r, 0);
-
+	err = add_client_resource(client, &r->resource, GFP_KERNEL);
+	if (err < 0)
+		return err;
 	request->handle = r->resource.handle;
 
-	return 0;
- fail:
-	kfree(r);
-	kfree(e1);
-	kfree(e2);
-
-	return ret;
-}
+	retain_and_null_ptr(e1);
+	retain_and_null_ptr(e2);
+	schedule_iso_resource_auto(no_free_ptr(r), 0);
 
-static int ioctl_allocate_iso_resource(struct client *client,
-				       union ioctl_arg *arg)
-{
-	return init_iso_resource(client, &arg->allocate_iso_resource);
+	return 0;
 }
 
 static int ioctl_deallocate_iso_resource(struct client *client,
-- 
2.53.0

[PATCH 6/7] firewire: core: append _auto suffix for non-once iso resource operations

[Takashi S. <o-t...@sa...>]

The functions for iso_resource once operations are carefully split from
another type of operation.

This commit adds _auto suffix to functions for the another type so that
it is easily to distinguish them.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 75 ++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 38 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index f81a8aa4bcbc..b3ce34d777c3 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -134,15 +134,15 @@ struct iso_resource_params {
 	s32 bandwidth;
 };
 
-struct iso_resource {
+struct iso_resource_auto {
 	struct client_resource resource;
 	struct client *client;
 	/* Schedule work and access todo only with client->lock held. */
 	struct delayed_work work;
 	enum {
-		ISO_RES_ALLOC,
-		ISO_RES_REALLOC,
-		ISO_RES_DEALLOC,
+		ISO_RES_AUTO_ALLOC,
+		ISO_RES_AUTO_REALLOC,
+		ISO_RES_AUTO_DEALLOC,
 	} todo;
 	struct iso_resource_params params;
 	struct iso_resource_event *e_alloc, *e_dealloc;
@@ -175,16 +175,16 @@ static struct descriptor_resource *to_descriptor_resource(struct client_resource
 	return container_of(resource, struct descriptor_resource, resource);
 }
 
-static struct iso_resource *to_iso_resource(struct client_resource *resource)
+static struct iso_resource_auto *to_iso_resource_auto(struct client_resource *resource)
 {
-	return container_of(resource, struct iso_resource, resource);
+	return container_of(resource, struct iso_resource_auto, resource);
 }
 
-static void release_iso_resource(struct client *, struct client_resource *);
+static void release_iso_resource_auto(struct client *, struct client_resource *);
 
-static int is_iso_resource(const struct client_resource *resource)
+static int is_iso_resource_auto(const struct client_resource *resource)
 {
-	return resource->release == release_iso_resource;
+	return resource->release == release_iso_resource_auto;
 }
 
 static void release_transaction(struct client *client,
@@ -195,7 +195,7 @@ static int is_outbound_transaction_resource(const struct client_resource *resour
 	return resource->release == release_transaction;
 }
 
-static void schedule_iso_resource(struct iso_resource *r, unsigned long delay)
+static void schedule_iso_resource_auto(struct iso_resource_auto *r, unsigned long delay)
 {
 	client_get(r->client);
 	if (!queue_delayed_work(fw_workqueue, &r->work, delay))
@@ -443,8 +443,8 @@ static void queue_bus_reset_event(struct client *client)
 	guard(spinlock_irq)(&client->lock);
 
 	xa_for_each(&client->resource_xa, index, resource) {
-		if (is_iso_resource(resource))
-			schedule_iso_resource(to_iso_resource(resource), 0);
+		if (is_iso_resource_auto(resource))
+			schedule_iso_resource_auto(to_iso_resource_auto(resource), 0);
 	}
 }
 
@@ -1323,10 +1323,10 @@ static int fill_iso_resource_params(struct iso_resource_params *params,
 	return 0;
 }
 
-static void iso_resource_work(struct work_struct *work)
+static void iso_resource_auto_work(struct work_struct *work)
 {
 	struct iso_resource_event *e;
-	struct iso_resource *r = from_work(r, work, work.work);
+	struct iso_resource_auto *r = from_work(r, work, work.work);
 	struct client *client = r->client;
 	unsigned long index = r->resource.handle;
 	int generation, channel, bandwidth, todo;
@@ -1336,16 +1336,16 @@ static void iso_resource_work(struct work_struct *work)
 		generation = client->device->generation;
 		todo = r->todo;
 		// Allow 1000ms grace period for other reallocations.
-		if (todo == ISO_RES_ALLOC &&
+		if (todo == ISO_RES_AUTO_ALLOC &&
 		    time_is_after_jiffies64(client->device->card->reset_jiffies + secs_to_jiffies(1))) {
-			schedule_iso_resource(r, msecs_to_jiffies(333));
+			schedule_iso_resource_auto(r, msecs_to_jiffies(333));
 			skip = true;
 		} else {
 			// We could be called twice within the same generation.
-			skip = todo == ISO_RES_REALLOC &&
+			skip = todo == ISO_RES_AUTO_REALLOC &&
 			       r->params.generation == generation;
 		}
-		free = todo == ISO_RES_DEALLOC;
+		free = todo == ISO_RES_AUTO_DEALLOC;
 		r->params.generation = generation;
 	}
 
@@ -1356,15 +1356,15 @@ static void iso_resource_work(struct work_struct *work)
 
 	fw_iso_resource_manage(client->device->card, generation,
 			r->params.channels, &channel, &bandwidth,
-			todo == ISO_RES_ALLOC ||
-			todo == ISO_RES_REALLOC);
+			todo == ISO_RES_AUTO_ALLOC ||
+			todo == ISO_RES_AUTO_REALLOC);
 	/*
 	 * Is this generation outdated already?  As long as this resource sticks
 	 * in the xarray, it will be scheduled again for a newer generation or at
 	 * shutdown.
 	 */
 	if (channel == -EAGAIN &&
-	    (todo == ISO_RES_ALLOC || todo == ISO_RES_REALLOC))
+	    (todo == ISO_RES_AUTO_ALLOC || todo == ISO_RES_AUTO_REALLOC))
 		goto out;
 
 	success = channel >= 0 || bandwidth > 0;
@@ -1372,11 +1372,11 @@ static void iso_resource_work(struct work_struct *work)
 	scoped_guard(spinlock_irq, &client->lock) {
 		// Transit from allocation to reallocation, except if the client
 		// requested deallocation in the meantime.
-		if (r->todo == ISO_RES_ALLOC)
-			r->todo = ISO_RES_REALLOC;
+		if (r->todo == ISO_RES_AUTO_ALLOC)
+			r->todo = ISO_RES_AUTO_REALLOC;
 		// Allocation or reallocation failure?  Pull this resource out of the
 		// xarray and prepare for deletion, unless the client is shutting down.
-		if (r->todo == ISO_RES_REALLOC && !success &&
+		if (r->todo == ISO_RES_AUTO_REALLOC && !success &&
 		    !client->in_shutdown &&
 		    xa_erase(&client->resource_xa, index)) {
 			client_put(client);
@@ -1384,13 +1384,13 @@ static void iso_resource_work(struct work_struct *work)
 		}
 	}
 
-	if (todo == ISO_RES_ALLOC && channel >= 0)
+	if (todo == ISO_RES_AUTO_ALLOC && channel >= 0)
 		r->params.channels = 1ULL << channel;
 
-	if (todo == ISO_RES_REALLOC && success)
+	if (todo == ISO_RES_AUTO_REALLOC && success)
 		goto out;
 
-	if (todo == ISO_RES_ALLOC) {
+	if (todo == ISO_RES_AUTO_ALLOC) {
 		e = r->e_alloc;
 		r->e_alloc = NULL;
 	} else {
@@ -1414,21 +1414,20 @@ static void iso_resource_work(struct work_struct *work)
 	client_put(client);
 }
 
-static void release_iso_resource(struct client *client,
-				 struct client_resource *resource)
+static void release_iso_resource_auto(struct client *client, struct client_resource *resource)
 {
-	struct iso_resource *r = to_iso_resource(resource);
+	struct iso_resource_auto *r = to_iso_resource_auto(resource);
 
 	guard(spinlock_irq)(&client->lock);
 
-	r->todo = ISO_RES_DEALLOC;
-	schedule_iso_resource(r, 0);
+	r->todo = ISO_RES_AUTO_DEALLOC;
+	schedule_iso_resource_auto(r, 0);
 }
 
 static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_resource *request)
 {
 	struct iso_resource_event *e1, *e2;
-	struct iso_resource *r;
+	struct iso_resource_auto *r;
 	int ret;
 
 	r = kmalloc_obj(*r);
@@ -1443,9 +1442,9 @@ static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_
 	if (ret < 0)
 		goto fail;
 
-	INIT_DELAYED_WORK(&r->work, iso_resource_work);
+	INIT_DELAYED_WORK(&r->work, iso_resource_auto_work);
 	r->client	= client;
-	r->todo		= ISO_RES_ALLOC;
+	r->todo		= ISO_RES_AUTO_ALLOC;
 	r->e_alloc	= e1;
 	r->e_dealloc	= e2;
 
@@ -1454,11 +1453,11 @@ static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_
 	e2->iso_resource.closure = request->closure;
 	e2->iso_resource.type    = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
 
-	r->resource.release = release_iso_resource;
+	r->resource.release = release_iso_resource_auto;
 	ret = add_client_resource(client, &r->resource, GFP_KERNEL);
 	if (ret < 0)
 		goto fail;
-	schedule_iso_resource(r, 0);
+	schedule_iso_resource_auto(r, 0);
 
 	request->handle = r->resource.handle;
 
@@ -1481,7 +1480,7 @@ static int ioctl_deallocate_iso_resource(struct client *client,
 					 union ioctl_arg *arg)
 {
 	return release_client_resource(client,
-			arg->deallocate.handle, release_iso_resource, NULL);
+			arg->deallocate.handle, release_iso_resource_auto, NULL);
 }
 
 #define UNAVAILABLE_HANDLE	-1
-- 
2.53.0

[PATCH 5/7] firewire: core: code cleanup to remove old implementations for once operation

[Takashi S. <o-t...@sa...>]

The helper functions for iso_resource allocation and work item still
include codes for once operation.

This commit refactors them to remove the old implementations.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 37 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 478e8f6400f0..f81a8aa4bcbc 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -139,8 +139,11 @@ struct iso_resource {
 	struct client *client;
 	/* Schedule work and access todo only with client->lock held. */
 	struct delayed_work work;
-	enum {ISO_RES_ALLOC, ISO_RES_REALLOC, ISO_RES_DEALLOC,
-	      ISO_RES_ALLOC_ONCE, ISO_RES_DEALLOC_ONCE,} todo;
+	enum {
+		ISO_RES_ALLOC,
+		ISO_RES_REALLOC,
+		ISO_RES_DEALLOC,
+	} todo;
 	struct iso_resource_params params;
 	struct iso_resource_event *e_alloc, *e_dealloc;
 };
@@ -1342,9 +1345,7 @@ static void iso_resource_work(struct work_struct *work)
 			skip = todo == ISO_RES_REALLOC &&
 			       r->params.generation == generation;
 		}
-		free = todo == ISO_RES_DEALLOC ||
-		       todo == ISO_RES_ALLOC_ONCE ||
-		       todo == ISO_RES_DEALLOC_ONCE;
+		free = todo == ISO_RES_DEALLOC;
 		r->params.generation = generation;
 	}
 
@@ -1356,8 +1357,7 @@ static void iso_resource_work(struct work_struct *work)
 	fw_iso_resource_manage(client->device->card, generation,
 			r->params.channels, &channel, &bandwidth,
 			todo == ISO_RES_ALLOC ||
-			todo == ISO_RES_REALLOC ||
-			todo == ISO_RES_ALLOC_ONCE);
+			todo == ISO_RES_REALLOC);
 	/*
 	 * Is this generation outdated already?  As long as this resource sticks
 	 * in the xarray, it will be scheduled again for a newer generation or at
@@ -1390,7 +1390,7 @@ static void iso_resource_work(struct work_struct *work)
 	if (todo == ISO_RES_REALLOC && success)
 		goto out;
 
-	if (todo == ISO_RES_ALLOC || todo == ISO_RES_ALLOC_ONCE) {
+	if (todo == ISO_RES_ALLOC) {
 		e = r->e_alloc;
 		r->e_alloc = NULL;
 	} else {
@@ -1425,8 +1425,7 @@ static void release_iso_resource(struct client *client,
 	schedule_iso_resource(r, 0);
 }
 
-static int init_iso_resource(struct client *client,
-		struct fw_cdev_allocate_iso_resource *request, int todo)
+static int init_iso_resource(struct client *client, struct fw_cdev_allocate_iso_resource *request)
 {
 	struct iso_resource_event *e1, *e2;
 	struct iso_resource *r;
@@ -1446,7 +1445,7 @@ static int init_iso_resource(struct client *client,
 
 	INIT_DELAYED_WORK(&r->work, iso_resource_work);
 	r->client	= client;
-	r->todo		= todo;
+	r->todo		= ISO_RES_ALLOC;
 	r->e_alloc	= e1;
 	r->e_dealloc	= e2;
 
@@ -1455,15 +1454,10 @@ static int init_iso_resource(struct client *client,
 	e2->iso_resource.closure = request->closure;
 	e2->iso_resource.type    = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
 
-	if (todo == ISO_RES_ALLOC) {
-		r->resource.release = release_iso_resource;
-		ret = add_client_resource(client, &r->resource, GFP_KERNEL);
-		if (ret < 0)
-			goto fail;
-	} else {
-		r->resource.release = NULL;
-		r->resource.handle = -1;
-	}
+	r->resource.release = release_iso_resource;
+	ret = add_client_resource(client, &r->resource, GFP_KERNEL);
+	if (ret < 0)
+		goto fail;
 	schedule_iso_resource(r, 0);
 
 	request->handle = r->resource.handle;
@@ -1480,8 +1474,7 @@ static int init_iso_resource(struct client *client,
 static int ioctl_allocate_iso_resource(struct client *client,
 				       union ioctl_arg *arg)
 {
-	return init_iso_resource(client,
-			&arg->allocate_iso_resource, ISO_RES_ALLOC);
+	return init_iso_resource(client, &arg->allocate_iso_resource);
 }
 
 static int ioctl_deallocate_iso_resource(struct client *client,
-- 
2.53.0

[PATCH 4/7] firewire: core: split functions for iso_resource once operation

[Takashi S. <o-t...@sa...>]

Unlike FW_CDEV_IOC_ALLOCATE_ISO_RESOURCE operation, the operations of
FW_CDEV_IOC_[DE]ALLOCATE_ISO_RESOURCE_ONCE require no client resource,
thus they keeps no handle value.

This commit adds the series of functions to separate these operations,
according to divide-and-conquer methodology.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 83 ++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 4 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index effa03739679..478e8f6400f0 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -145,6 +145,18 @@ struct iso_resource {
 	struct iso_resource_event *e_alloc, *e_dealloc;
 };
 
+struct iso_resource_once {
+	struct client *client;
+	// Schedule work and access todo only with client->lock held.
+	struct delayed_work work;
+	enum {
+		ISO_RES_ONCE_ALLOC,
+		ISO_RES_ONCE_DEALLOC,
+	} todo;
+	struct iso_resource_params params;
+	struct iso_resource_event *event;
+};
+
 static struct address_handler_resource *to_address_handler_resource(struct client_resource *resource)
 {
 	return container_of(resource, struct address_handler_resource, resource);
@@ -1479,18 +1491,81 @@ static int ioctl_deallocate_iso_resource(struct client *client,
 			arg->deallocate.handle, release_iso_resource, NULL);
 }
 
+#define UNAVAILABLE_HANDLE	-1
+
+static void iso_resource_once_work(struct work_struct *work)
+{
+	struct iso_resource_once *r = from_work(r, work, 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;
+
+	r->params.generation = generation;
+	bandwidth = r->params.bandwidth;
+
+	fw_iso_resource_manage(client->device->card, generation, r->params.channels, &channel,
+			       &bandwidth, r->todo == ISO_RES_ONCE_ALLOC);
+
+	e->iso_resource.handle = UNAVAILABLE_HANDLE;
+	e->iso_resource.channel = channel;
+	e->iso_resource.bandwidth = bandwidth;
+
+	queue_event(client, &e->event, &e->iso_resource, sizeof(e->iso_resource), NULL, 0);
+
+	cancel_delayed_work(&r->work);
+	kfree(r);
+
+	client_put(client);
+}
+
+static int init_iso_resource_once(struct client *client,
+				  struct fw_cdev_allocate_iso_resource *request, int todo)
+{
+	struct iso_resource_event *e __free(kfree) = kmalloc_obj(*e);
+	struct iso_resource_once *r __free(kfree) = kmalloc_obj(*r);
+	int err;
+
+	if (!r || !e)
+		return -ENOMEM;
+
+	err = fill_iso_resource_params(&r->params, request);
+	if (err < 0)
+		return err;
+
+	INIT_DELAYED_WORK(&r->work, iso_resource_once_work);
+	r->client = client;
+	r->todo	= todo;
+
+	if (todo == ISO_RES_ONCE_ALLOC)
+		e->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED;
+	else
+		e->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED;
+	e->iso_resource.closure = request->closure;
+	r->event = no_free_ptr(e);
+
+	// Keep the client until work item finishing.
+	client_get(r->client);
+
+	queue_delayed_work(fw_workqueue, &no_free_ptr(r)->work, 0);
+
+	request->handle = UNAVAILABLE_HANDLE;
+
+	return 0;
+}
+
 static int ioctl_allocate_iso_resource_once(struct client *client,
 					    union ioctl_arg *arg)
 {
-	return init_iso_resource(client,
-			&arg->allocate_iso_resource, ISO_RES_ALLOC_ONCE);
+	return init_iso_resource_once(client, &arg->allocate_iso_resource, ISO_RES_ONCE_ALLOC);
 }
 
 static int ioctl_deallocate_iso_resource_once(struct client *client,
 					      union ioctl_arg *arg)
 {
-	return init_iso_resource(client,
-			&arg->allocate_iso_resource, ISO_RES_DEALLOC_ONCE);
+	return init_iso_resource_once(client, &arg->allocate_iso_resource, ISO_RES_ONCE_DEALLOC);
 }
 
 /*
-- 
2.53.0

[PATCH 3/7] firewire: core: code refactoring for helper function to fill iso_resource parameters

[Takashi S. <o-t...@sa...>]

This change is a preparation for future changes. The added helper function
will be reused in the changes to fill iso_resource parameters according to
the users' request.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 45 ++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 8391c7efab2c..effa03739679 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -128,6 +128,12 @@ struct descriptor_resource {
 	u32 data[];
 };
 
+struct iso_resource_params {
+	int generation;
+	u64 channels;
+	s32 bandwidth;
+};
+
 struct iso_resource {
 	struct client_resource resource;
 	struct client *client;
@@ -135,9 +141,7 @@ struct iso_resource {
 	struct delayed_work work;
 	enum {ISO_RES_ALLOC, ISO_RES_REALLOC, ISO_RES_DEALLOC,
 	      ISO_RES_ALLOC_ONCE, ISO_RES_DEALLOC_ONCE,} todo;
-	int generation;
-	u64 channels;
-	s32 bandwidth;
+	struct iso_resource_params params;
 	struct iso_resource_event *e_alloc, *e_dealloc;
 };
 
@@ -1290,6 +1294,20 @@ static int ioctl_get_cycle_timer(struct client *client, union ioctl_arg *arg)
 	return 0;
 }
 
+static int fill_iso_resource_params(struct iso_resource_params *params,
+				    struct fw_cdev_allocate_iso_resource *request)
+{
+	if ((request->channels == 0 && request->bandwidth == 0) ||
+	    request->bandwidth > BANDWIDTH_AVAILABLE_INITIAL)
+		return -EINVAL;
+
+	params->generation = -1;
+	params->channels = request->channels;
+	params->bandwidth = request->bandwidth;
+
+	return 0;
+}
+
 static void iso_resource_work(struct work_struct *work)
 {
 	struct iso_resource_event *e;
@@ -1310,21 +1328,21 @@ static void iso_resource_work(struct work_struct *work)
 		} else {
 			// We could be called twice within the same generation.
 			skip = todo == ISO_RES_REALLOC &&
-			       r->generation == generation;
+			       r->params.generation == generation;
 		}
 		free = todo == ISO_RES_DEALLOC ||
 		       todo == ISO_RES_ALLOC_ONCE ||
 		       todo == ISO_RES_DEALLOC_ONCE;
-		r->generation = generation;
+		r->params.generation = generation;
 	}
 
 	if (skip)
 		goto out;
 
-	bandwidth = r->bandwidth;
+	bandwidth = r->params.bandwidth;
 
 	fw_iso_resource_manage(client->device->card, generation,
-			r->channels, &channel, &bandwidth,
+			r->params.channels, &channel, &bandwidth,
 			todo == ISO_RES_ALLOC ||
 			todo == ISO_RES_REALLOC ||
 			todo == ISO_RES_ALLOC_ONCE);
@@ -1355,7 +1373,7 @@ static void iso_resource_work(struct work_struct *work)
 	}
 
 	if (todo == ISO_RES_ALLOC && channel >= 0)
-		r->channels = 1ULL << channel;
+		r->params.channels = 1ULL << channel;
 
 	if (todo == ISO_RES_REALLOC && success)
 		goto out;
@@ -1402,10 +1420,6 @@ static int init_iso_resource(struct client *client,
 	struct iso_resource *r;
 	int ret;
 
-	if ((request->channels == 0 && request->bandwidth == 0) ||
-	    request->bandwidth > BANDWIDTH_AVAILABLE_INITIAL)
-		return -EINVAL;
-
 	r = kmalloc_obj(*r);
 	e1 = kmalloc_obj(*e1);
 	e2 = kmalloc_obj(*e2);
@@ -1414,12 +1428,13 @@ static int init_iso_resource(struct client *client,
 		goto fail;
 	}
 
+	ret = fill_iso_resource_params(&r->params, request);
+	if (ret < 0)
+		goto fail;
+
 	INIT_DELAYED_WORK(&r->work, iso_resource_work);
 	r->client	= client;
 	r->todo		= todo;
-	r->generation	= -1;
-	r->channels	= request->channels;
-	r->bandwidth	= request->bandwidth;
 	r->e_alloc	= e1;
 	r->e_dealloc	= e2;
 
-- 
2.53.0

[PATCH 2/7] firewire: core: code refactoring to queue work item for iso_resource

[Takashi S. <o-t...@sa...>]

The add_client_resource() function checks the type of client resource
every time to be called. If the type is for iso_resource, it schedules
work item.

However, the iso_resource client resource is only added by the call of
init_iso_resource(). There is no need to check the type every time adding
any client resource.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 144625c34be2..8391c7efab2c 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -526,8 +526,6 @@ static int add_client_resource(struct client *client, struct client_resource *re
 
 		resource->handle = index;
 		client_get(client);
-		if (is_iso_resource(resource))
-			schedule_iso_resource(to_iso_resource(resource), 0);
 	}
 
 	return 0;
@@ -1438,8 +1436,9 @@ static int init_iso_resource(struct client *client,
 	} else {
 		r->resource.release = NULL;
 		r->resource.handle = -1;
-		schedule_iso_resource(r, 0);
 	}
+	schedule_iso_resource(r, 0);
+
 	request->handle = r->resource.handle;
 
 	return 0;
-- 
2.53.0

[PATCH 0/7] firewire: core: separate iso_resource paths

[Takashi S. <o-t...@sa...>]

Hi,

Dingisoul has reported that a case where the reference count of a
client structure is leaked when handling iso_resource in cdev layer[1].
Fixing the bug immediately s difficult due to the complexity of
per-client resource lifetime.

As a first step toward addressing this issue, this patchset refactors the
existing code for isochronous resource operation. Userspace application
can allocate and deallocate isochronous resources on IEEE 1394 bus in two
ways:
 * FW_CDEV_IOC_[DE]ALLOCATE_ISO_RESOURCE
 * FW_CDEV_IOC_[DE]ALLOCATE_ISO_RESOURCE_ONCE

With the former, the application delegates the maintenance of the
allocated isochronous resources to kernel and obtain a handle for the
client resource. With the latter, the application should maintain
isochronous resources every time receiving bus reset event, without
relying on a handle.

Currently, both  operations are handled by the same code, although they
differ in terms of client resource management.

This patchset separates these two paths. As a result, it becomes clear
that the reported issue only affects client resource allocated via the
former method. While the actual bug fix is deferred, this refactoring
lays the groundwork for it.

[1] https://sourceforge.net/p/linux1394/mailman/linux1394-devel/thread/20260404110936.GA282614%40sakamocchi.jp/#msg59317811

Takashi Sakamoto (7):
  firewire: core: code refactoring for early return at client resource
    allocation
  firewire: core: code refactoring to queue work item for iso_resource
  firewire: core: code refactoring for helper function to fill
    iso_resource parameters
  firewire: core: split functions for iso_resource once operation
  firewire: core: code cleanup to remove old implementations for once
    operation
  firewire: core: append _auto suffix for non-once iso resource
    operations
  firewire: core: code cleanup for iso resource auto creation

 drivers/firewire/core-cdev.c | 285 +++++++++++++++++++++--------------
 1 file changed, 176 insertions(+), 109 deletions(-)


base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
-- 
2.53.0

[PATCH 1/7] firewire: core: code refactoring for early return at client resource allocation

[Takashi S. <o-t...@sa...>]

The add_client_resource() function returns zero at success or negative
value at error. The critical section is already protected by
scoped_guard() macro. In this case, the programming pattern of early
return improves code readability.

Signed-off-by: Takashi Sakamoto <o-t...@sa...>
---
 drivers/firewire/core-cdev.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index f791db4c8dff..144625c34be2 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -507,31 +507,30 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 static int add_client_resource(struct client *client, struct client_resource *resource,
 			       gfp_t gfp_mask)
 {
-	int ret;
-
 	scoped_guard(spinlock_irqsave, &client->lock) {
 		u32 index;
+		int ret;
+
+		if (client->in_shutdown)
+			return  -ECANCELED;
 
-		if (client->in_shutdown) {
-			ret = -ECANCELED;
+		if (gfpflags_allow_blocking(gfp_mask)) {
+			ret = xa_alloc(&client->resource_xa, &index, resource, xa_limit_32b,
+				       GFP_NOWAIT);
 		} else {
-			if (gfpflags_allow_blocking(gfp_mask)) {
-				ret = xa_alloc(&client->resource_xa, &index, resource, xa_limit_32b,
-					       GFP_NOWAIT);
-			} else {
-				ret = xa_alloc_bh(&client->resource_xa, &index, resource,
-						  xa_limit_32b, GFP_NOWAIT);
-			}
-		}
-		if (ret >= 0) {
-			resource->handle = index;
-			client_get(client);
-			if (is_iso_resource(resource))
-				schedule_iso_resource(to_iso_resource(resource), 0);
+			ret = xa_alloc_bh(&client->resource_xa, &index, resource,
+					  xa_limit_32b, GFP_NOWAIT);
 		}
+		if (ret < 0)
+			return ret;
+
+		resource->handle = index;
+		client_get(client);
+		if (is_iso_resource(resource))
+			schedule_iso_resource(to_iso_resource(resource), 0);
 	}
 
-	return ret < 0 ? ret : 0;
+	return 0;
 }
 
 static int release_client_resource(struct client *client, u32 handle,
-- 
2.53.0

Re: [PATCH v1 0/2] firewire: Simplify storing pointers in device id struct

[Takashi S. <o-t...@sa...>]

Hi,

On Thu, Apr 23, 2026 at 06:53:12PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> Hello Takashi,
> 
> On Thu, Apr 23, 2026 at 11:19:59PM +0900, Takashi Sakamoto wrote:
> > It is unclear who is responsible for maintaining the 'ieee1394_device_id'
> > structure in include/linux/mod_devicetable.h, but if it falls under my
> > responsibility (which seems likely),
> 
> It matches my understanding that it's indeed you who is responsible for
> ieee1394_device_id.
> 
> > I would prefer to postpone applying these patches, or at least exclude
> > them from this merge window.
> 
> I don't expect them to go in for v7.1-rc1. My idea was to send this kind
> of patch during the merge window to allow to get it into next soon after
> the merge window closed to allow some cooking in next.
>  
> > After reading the discussions around the UAPI, I am not fully convinced
> > that your patches appear to provide clear benefits to existing
> > IEEE 1394 bus users or their software. From my perspective, the motivation
> > appears to be primarily related to the CHERI extension work.
> 
> Yes and no. My motivation to work on these is triggered by CHERI indeed.
> But I already worked on this kind of update even before I knew about
> CHERI to get rid of the casts. (Back then it was about i2c, see
> https://lore.kernel.org/all/202...@pe.../.)
> 
> For me this series is a sweet spot, because it allows to reduce the
> CHERI patch stack but at the same time reduces the amount of (IMHO ugly)
> casts involved in handling pointers in .driver_data. With my mainline
> maintainer hat on, I consider the latter alone a good enough reason to
> apply patches like this. One issue it addresses en passant is that a
> part of the casts back to a pointer drop the const attribute, see the
> output of
> 
> 	git grep '\*).*driver_data' | grep -v '\<const\>'
> 
> . I didn't check for false positives in this list, but I guess there are
> not many. This doesn't affect firewire though, there the casts are all
> fine.
> 
> As my patch series doesn't introduce changes to the compiled drivers,
> the effect on bus users or their software is admittedly quite limited of
> course :-), but I'd hope that you see a benefit in the 2nd patch even if
> CHERI isn't (and probably shouldn't be) a motivation for you.
> 
> The reason I mentioned CHERI was mainly to be transparent about my
> motivation, but this triggered more discussion than I liked, distracting
> from the advantages of the changes for non-CHERI archs.
> 
> > As you know, this subsystem is quite marginal in the Linux kernel
> > codebase. Given that, it might be worth considering whether this
> > subsystem should be excluded from the build target when capability
> > pointers are enabled (e.g. via Kconfig, if available), since it does not
> > appear to work outside the ILP32 or LP64 data models. It may be worth
> > carefully considering where effort is best spent. I can understand the
> > merits of CHERI extensions, but changes related to this subsystem would
> > likely be acceptable only after the kernel core functions have been
> > updated.
> 
> I wasn't aware about this limitation. struct ieee1394_device_id just
> happend to be near the top of include/linux/mod_devicetable.h. (There is
> only pci_device_id before, but I wanted to start with a smaller part of
> this quest.)
>  
> > That said, this is just my current view. I would welcome any feedback or
> > objections. Besides, it is still one of my tasks to figure out how to
> > adapt the UAPI structures and the firewire core implementations to
> > non-ILP32/LP64 data models.
> 
> With you talking much about UAPI I wonder if you're aware that for all
> current Linux architectures my patch set doesn't introduce any changes
> in the binary interface because all have sizeof(unsigned long) ==
> sizeof(union { unsigned long; const void *; }). And the two adaptions
> (my introduction of the union for driver_data + your UAPI structure
> adaptions) should not restrict each other, right?
> 
> So I'd be glad if you forgot about CHERI and just judge the patchset for
> the things it achieves for current mainline. Of course I hope you agree
> it's a nice cleanup eventually and apply it.

Setting aside CHERI-related considerations, what concrete benefits do
these patches provide to the current kernel? Are those benefits clearly
explained in the cover letter and the patch comments?

>From the cover letter, I understand that:

> A considerable amount of drivers for the first category uses the
> unsigned long variable to store a pointer. This involves casting both
> for assignment and usage.

The conversion between 'unsigned long' and 'void *' are not inherently
invalid within ILP32/LP64 data models. As I see it, the main significant
effect of these patches is to add a 'const' qualifier to pointer values
(and improve type safety, presumably in your view).

If that is the case, why do the patch comments spend so much lines
describing the size of unsigned long and pointer types, and CHERI
extension? These points do not seem directly relevant to the practical
benefits described above.

It would be better to repost the patches with clearer and more focused
comments so that subsystem maintainers are more likely to apply them
without the background discussion about the CHERI extension.


Thanks

Takashi Sakamoto

Re: [PATCH v1 0/2] firewire: Simplify storing pointers in device id struct

[Takashi S. <o-t...@sa...>]

Hi,

It is unclear who is responsible for maintaining the 'ieee1394_device_id'
structure in include/linux/mod_devicetable.h, but if it falls under my
responsibility (which seems likely), I would prefer to postpone applying
these patches, or at least exclude them from this merge window.

After reading the discussions around the UAPI, I am not fully convinced
that your patches appear to provide clear benefits to existing
IEEE 1394 bus users or their software. From my perspective, the motivation
appears to be primarily related to the CHERI extension work.

As you know, this subsystem is quite marginal in the Linux kernel
codebase. Given that, it might be worth considering whether this
subsystem should be excluded from the build target when capability
pointers are enabled (e.g. via Kconfig, if available), since it does not
appear to work outside the ILP32 or LP64 data models. It may be worth
carefully considering where effort is best spent. I can understand the
merits of CHERI extensions, but changes related to this subsystem would
likely be acceptable only after the kernel core functions have been
updated.

That said, this is just my current view. I would welcome any feedback or
objections. Besides, it is still one of my tasks to figure out how to
adapt the UAPI structures and the firewire core implementations to
non-ILP32/LP64 data models.


Thanks

Takashi Sakamoto

On Sun, Apr 19, 2026 at 08:42:12AM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> Hello,
> 
> <linux/mod_devicetable.h> contains several device_id structs for various
> device types.
> 
> Most of them have one of:
> 
>  - kernel_ulong_t driver_data (sometimes called "driver_info", sometimes 
>    the type is plain unsigned long)
>  - const void *data (sometimes called "driver_data" or "context", sometimes not const)
> 
> A considerable amount of drivers for the first category uses the
> unsigned long variable to store a pointer. This involves casting both
> for assignment and usage.
> 
> An additional complication exists for the CHERI hardware extension
> where sizeof(void *) > sizeof(unsigned long). So with that an unsigned
> long variable cannot be used to store a pointer.
> 
> To address both issues this series replaces the unsigned long variable
> by an anonymous union containing both an unsigned long and a pointer.
> 
> For all non-CHERI architectures this isn't an ABI change because all
> have sizeof(void *) == sizeof(unsigned long).
> 
> The first patch changes the definition of struct ieee1394_device_id. The
> second drops some casts in sound drivers. (There are no other firewire
> drivers that could benefit.) I adapted all sound drivers in a single
> patch, tell me if I should split per driver.
> 
> For merging I suggest to take the whole series via the ALSA tree in the
> next merge window, as there are no modified files that are specific to
> firewire only and the second patch depends on the first.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (The Capable Hub) (2):
>   firewire: Simplify storing pointers in device id struct
>   ALSA: firewire: Make use of ieee1394's .driver_data_ptr
> 
>  include/linux/mod_devicetable.h |  5 ++++-
>  sound/firewire/dice/dice.c      | 34 ++++++++++++++++-----------------
>  sound/firewire/fireface/ff.c    | 12 ++++++------
>  sound/firewire/motu/motu.c      |  6 +++---
>  sound/firewire/oxfw/oxfw.c      |  4 ++--
>  5 files changed, 32 insertions(+), 29 deletions(-)
> 
> 
> base-commit: 028ef9c96e96197026887c0f092424679298aae8
> -- 
> 2.47.3
>