linux1394-devel Mailing List for IEEE 1394 for Linux

On Wed, May 20, 2026 at 10:08:37PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> Some paths could still be refactored to remove redundant paths after
> the previous cdev layer refactoring patchset[1].
> 
> 
> [1] https://lore.kernel.org/lkml/202...@sa.../
> 
> 
> Takashi Sakamoto (3):
>   firewire: core: minor code refactoring for case-dependent parameters
>     of iso resources management
>   firewire: core: rename member name for channel mask of isoc resource
>   firewire: core: cancel using delayed work for iso_resource_once
>     management
> 
>  drivers/firewire/core-cdev.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)

Applied for for-next branch.


Regards

Takashi Sakamoto

There is no need to use deferrable type of work for iso_resource_once
management because the work is queued to run immediately.

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

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 56c21cabc20c..e49d8a58be09 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -150,8 +150,7 @@ struct iso_resource_auto {
 
 struct iso_resource_once {
 	struct client *client;
-	// Schedule work and access todo only with client->lock held.
-	struct delayed_work work;
+	struct work_struct work;
 	enum {
 		ISO_RES_ONCE_ALLOC,
 		ISO_RES_ONCE_DEALLOC,
@@ -1486,7 +1485,7 @@ static int ioctl_deallocate_iso_resource(struct client *client,
 
 static void iso_resource_once_work(struct work_struct *work)
 {
-	struct iso_resource_once *r = from_work(r, work, work.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;
@@ -1505,7 +1504,7 @@ static void iso_resource_once_work(struct work_struct *work)
 
 	queue_event(client, &e->event, &e->iso_resource, sizeof(e->iso_resource), NULL, 0);
 
-	cancel_delayed_work(&r->work);
+	cancel_work(&r->work);
 	kfree(r);
 
 	client_put(client);
@@ -1525,7 +1524,7 @@ static int init_iso_resource_once(struct client *client,
 	if (err < 0)
 		return err;
 
-	INIT_DELAYED_WORK(&r->work, iso_resource_once_work);
+	INIT_WORK(&r->work, iso_resource_once_work);
 	r->client = client;
 	r->todo	= todo;
 
@@ -1539,7 +1538,7 @@ static int init_iso_resource_once(struct client *client,
 	// Keep the client until work item finishing.
 	client_get(r->client);
 
-	queue_delayed_work(fw_workqueue, &no_free_ptr(r)->work, 0);
+	queue_work(fw_workqueue, &no_free_ptr(r)->work);
 
 	request->handle = UNAVAILABLE_HANDLE;
 
-- 
2.53.0

The iso_resource_params structure has a member for channel mask, while
the name of field is easy to misinterpret.

Append _mask to the member name.

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

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index c669c9e42d34..56c21cabc20c 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -129,7 +129,7 @@ struct descriptor_resource {
 };
 
 struct iso_resource_params {
-	u64 channels;
+	u64 channels_mask;
 	s32 bandwidth;
 };
 
@@ -1316,7 +1316,7 @@ static int fill_iso_resource_params(struct iso_resource_params *params,
 	    request->bandwidth > BANDWIDTH_AVAILABLE_INITIAL)
 		return -EINVAL;
 
-	params->channels = request->channels;
+	params->channels_mask = request->channels;
 	params->bandwidth = request->bandwidth;
 
 	return 0;
@@ -1360,7 +1360,7 @@ static void iso_resource_auto_work(struct work_struct *work)
 
 	bandwidth = r->params.bandwidth;
 
-	fw_iso_resource_manage(client->device->card, current_generation, r->params.channels,
+	fw_iso_resource_manage(client->device->card, current_generation, r->params.channels_mask,
 			       &channel, &bandwidth, todo != ISO_RES_AUTO_DEALLOC);
 
 	if (todo == ISO_RES_AUTO_DEALLOC) {
@@ -1402,7 +1402,7 @@ static void iso_resource_auto_work(struct work_struct *work)
 				r->todo = ISO_RES_AUTO_REALLOC;
 
 			if (channel >= 0)
-				r->params.channels = 1ULL << channel;
+				r->params.channels_mask = BIT_ULL(channel);
 
 			e = r->e_alloc;
 			r->e_alloc = NULL;
@@ -1496,7 +1496,7 @@ static void iso_resource_once_work(struct work_struct *work)
 
 	bandwidth = r->params.bandwidth;
 
-	fw_iso_resource_manage(client->device->card, generation, r->params.channels, &channel,
+	fw_iso_resource_manage(client->device->card, generation, r->params.channels_mask, &channel,
 			       &bandwidth, r->todo == ISO_RES_ONCE_ALLOC);
 
 	e->iso_resource.handle = UNAVAILABLE_HANDLE;
-- 
2.53.0

Hi,

Some paths could still be refactored to remove redundant paths after
the previous cdev layer refactoring patchset[1].


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


Takashi Sakamoto (3):
  firewire: core: minor code refactoring for case-dependent parameters
    of iso resources management
  firewire: core: rename member name for channel mask of isoc resource
  firewire: core: cancel using delayed work for iso_resource_once
    management

 drivers/firewire/core-cdev.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)


base-commit: 8208d94f149a53311ac7687c051cb3a6d58063f7
-- 
2.53.0

The generation parameter is specific to the auto case of iso resources
management, while it is in the common parameter structure.

Move the generation member to the structure specific to auto case.

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

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index c166e7617d2a..c669c9e42d34 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -129,7 +129,6 @@ struct descriptor_resource {
 };
 
 struct iso_resource_params {
-	int generation;
 	u64 channels;
 	s32 bandwidth;
 };
@@ -144,6 +143,7 @@ struct iso_resource_auto {
 		ISO_RES_AUTO_REALLOC,
 		ISO_RES_AUTO_DEALLOC,
 	} todo;
+	int generation;
 	struct iso_resource_params params;
 	struct iso_resource_event *e_alloc, *e_dealloc;
 };
@@ -1316,7 +1316,6 @@ static int fill_iso_resource_params(struct iso_resource_params *params,
 	    request->bandwidth > BANDWIDTH_AVAILABLE_INITIAL)
 		return -EINVAL;
 
-	params->generation = -1;
 	params->channels = request->channels;
 	params->bandwidth = request->bandwidth;
 
@@ -1336,8 +1335,8 @@ static void iso_resource_auto_work(struct work_struct *work)
 	scoped_guard(spinlock_irq, &client->lock) {
 		reset_jiffies = client->device->card->reset_jiffies;
 		current_generation = client->device->generation;
-		resource_generation = r->params.generation;
-		r->params.generation = current_generation;
+		resource_generation = r->generation;
+		r->generation = current_generation;
 		todo = r->todo;
 	}
 
@@ -1495,7 +1494,6 @@ static void iso_resource_once_work(struct work_struct *work)
 	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,
-- 
2.53.0

Hi,

On Mon, May 11, 2026 at 12:45:01PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> Hello,
> 
> v1 of this series can be found at
> https://lore.kernel.org/all/cov...@ba...
> .
> 
> The changes introduced here are the same as before, but the commit log
> of the first patch is (hopefully) improved to better point out the
> advantage of the approach for mainline. The second patch demonstrates
> the explicit casts can be dropped after the first patch.
> 
> The patch series intends to not change the runtime behaviour, however
> the 2nd patch introduces a few changes to the generated code. Looking at
> these for an arm64 build they only affected register allocation (so
> where x0 was used before it's x1 after the patch). I'm not proficient in
> x86 assembly enough to understand the changes there, but I guess they
> also don't affect the runtime behaviour.
> 
> My motivation for this patch set is to reduce the patch stack for Linux
> CHERI support. This affects firewire because with CHERI you cannot store
> a pointer in an unsigned long variable. But I hope these changes qualify
> as cleanup worth to be applied even without considering CHERI.
> 
> For merging I suggest to take the whole series via the ALSA tree during
> 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(-)

These two patches are applied to for-next branch.

We can't predict the future, but setting aside the comment about the CHERI
extension, the actual benefit of these changes in the current Linux
kernel is simply to avoid casting between long and pointer and enable
constant pointer for the existing code.


Thanks

Takashi Sakamoto

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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