[PATCH 0/5] firewire: core: use XArray to maintain client resources

[PATCH 0/5] firewire: core: use XArray to maintain client resources

[Takashi Sakamoto]

Hi,

In core function, the instances of several types of client resources
are maintained by IDR, and the index value of each resource is passed to
user space application as handle. As of kernel v6.0, IDR has been
superseded by XArray and deprecated.

This series of changes is to obsolete the usage of IDR with XArray.

Takashi Sakamoto (5):
  firewire: core: minor code refactoring to release client resource
  firewire: core: add helper functions to convert to parent resource
    structure
  firewire: core: add helper function to detect data of iso resource
    structure
  firewire: core: code refactoring to use idr_for_each_entry() macro
    instead of idr_for_each() function
  firewire: core: use xarray instead of idr to maintain client resource

 drivers/firewire/core-cdev.c | 170 +++++++++++++++++++----------------
 drivers/firewire/core.h      |   1 -
 2 files changed, 94 insertions(+), 77 deletions(-)

-- 
2.43.0

[PATCH 1/5] firewire: core: minor code refactoring to release client resource

[Takashi Sakamoto]

Current implementation checks and validates the result to find resource
entry two times. It is redundant.

This commit refactors the redundancy.

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

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index c211bb19c94e..81fdb2be9063 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -512,15 +512,14 @@ static int release_client_resource(struct client *client, u32 handle,
 
 	scoped_guard(spinlock_irq, &client->lock) {
 		if (client->in_shutdown)
-			resource = NULL;
-		else
-			resource = idr_find(&client->resource_idr, handle);
-		if (resource && resource->release == release)
-			idr_remove(&client->resource_idr, handle);
-	}
+			return -EINVAL;
 
-	if (!(resource && resource->release == release))
-		return -EINVAL;
+		resource = idr_find(&client->resource_idr, handle);
+		if (!resource || resource->release != release)
+			return -EINVAL;
+
+		idr_remove(&client->resource_idr, handle);
+	}
 
 	if (return_resource)
 		*return_resource = resource;
-- 
2.43.0

[PATCH 2/5] firewire: core: add helper functions to convert to parent resource structure

[Takashi Sakamoto]

All of local resource structure commonly have data of client_resource type
in its first member. This design sometimes requires usage of
container_of to retrieve parent structure by the first member.

This commit adds some helper functions for this purpose.

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

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 81fdb2be9063..e72f91cc3711 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -139,6 +139,26 @@ struct iso_resource {
 	struct iso_resource_event *e_alloc, *e_dealloc;
 };
 
+static struct address_handler_resource *to_address_handler_resource(struct client_resource *resource)
+{
+	return container_of(resource, struct address_handler_resource, resource);
+}
+
+static struct inbound_transaction_resource *to_inbound_transaction_resource(struct client_resource *resource)
+{
+	return container_of(resource, struct inbound_transaction_resource, resource);
+}
+
+static struct descriptor_resource *to_descriptor_resource(struct client_resource *resource)
+{
+	return container_of(resource, struct descriptor_resource, resource);
+}
+
+static struct iso_resource *to_iso_resource(struct client_resource *resource)
+{
+	return container_of(resource, struct iso_resource, resource);
+}
+
 static void release_iso_resource(struct client *, struct client_resource *);
 
 static void schedule_iso_resource(struct iso_resource *r, unsigned long delay)
@@ -151,8 +171,7 @@ static void schedule_iso_resource(struct iso_resource *r, unsigned long delay)
 static void schedule_if_iso_resource(struct client_resource *resource)
 {
 	if (resource->release == release_iso_resource)
-		schedule_iso_resource(container_of(resource,
-					struct iso_resource, resource), 0);
+		schedule_iso_resource(to_iso_resource(resource), 0);
 }
 
 /*
@@ -682,8 +701,7 @@ static int ioctl_send_request(struct client *client, union ioctl_arg *arg)
 static void release_request(struct client *client,
 			    struct client_resource *resource)
 {
-	struct inbound_transaction_resource *r = container_of(resource,
-			struct inbound_transaction_resource, resource);
+	struct inbound_transaction_resource *r = to_inbound_transaction_resource(resource);
 
 	if (r->is_fcp)
 		fw_request_put(r->request);
@@ -793,8 +811,7 @@ static void handle_request(struct fw_card *card, struct fw_request *request,
 static void release_address_handler(struct client *client,
 				    struct client_resource *resource)
 {
-	struct address_handler_resource *r =
-	    container_of(resource, struct address_handler_resource, resource);
+	struct address_handler_resource *r = to_address_handler_resource(resource);
 
 	fw_core_remove_address_handler(&r->handler);
 	kfree(r);
@@ -858,8 +875,7 @@ static int ioctl_send_response(struct client *client, union ioctl_arg *arg)
 				    release_request, &resource) < 0)
 		return -EINVAL;
 
-	r = container_of(resource, struct inbound_transaction_resource,
-			 resource);
+	r = to_inbound_transaction_resource(resource);
 	if (r->is_fcp) {
 		fw_request_put(r->request);
 		goto out;
@@ -893,8 +909,7 @@ static int ioctl_initiate_bus_reset(struct client *client, union ioctl_arg *arg)
 static void release_descriptor(struct client *client,
 			       struct client_resource *resource)
 {
-	struct descriptor_resource *r =
-		container_of(resource, struct descriptor_resource, resource);
+	struct descriptor_resource *r = to_descriptor_resource(resource);
 
 	fw_core_remove_descriptor(&r->descriptor);
 	kfree(r);
@@ -1387,8 +1402,7 @@ static void iso_resource_work(struct work_struct *work)
 static void release_iso_resource(struct client *client,
 				 struct client_resource *resource)
 {
-	struct iso_resource *r =
-		container_of(resource, struct iso_resource, resource);
+	struct iso_resource *r = to_iso_resource(resource);
 
 	guard(spinlock_irq)(&client->lock);
 
-- 
2.43.0

[PATCH 3/5] firewire: core: add helper function to detect data of iso resource structure

[Takashi Sakamoto]

It depends on the function assigned to release member to identify
resource structure.

This commit adds a helper function to identify iso_resource structure.

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

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index e72f91cc3711..6fe2a2ea9869 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -161,6 +161,11 @@ static struct iso_resource *to_iso_resource(struct client_resource *resource)
 
 static void release_iso_resource(struct client *, struct client_resource *);
 
+static int is_iso_resource(const struct client_resource *resource)
+{
+	return resource->release == release_iso_resource;
+}
+
 static void schedule_iso_resource(struct iso_resource *r, unsigned long delay)
 {
 	client_get(r->client);
@@ -170,7 +175,7 @@ static void schedule_iso_resource(struct iso_resource *r, unsigned long delay)
 
 static void schedule_if_iso_resource(struct client_resource *resource)
 {
-	if (resource->release == release_iso_resource)
+	if (is_iso_resource(resource))
 		schedule_iso_resource(to_iso_resource(resource), 0);
 }
 
-- 
2.43.0

[PATCH 4/5] firewire: core: code refactoring to use idr_for_each_entry() macro instead of idr_for_each() function

[Takashi Sakamoto]

This commit is a preparation to use xa_for_each() macro. Current
implementation uses idr_for_each() function and has a disadvantage to
replace with the macro. The IDR framework has idr_for_each_entry() macro
for the similar purpose. This commit replace the function with the
macro with minor code refactoring.

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

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 6fe2a2ea9869..83d25327c1d3 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -166,6 +166,14 @@ static int is_iso_resource(const struct client_resource *resource)
 	return resource->release == release_iso_resource;
 }
 
+static void release_transaction(struct client *client,
+				struct client_resource *resource);
+
+static int is_outbound_transaction_resource(const struct client_resource *resource)
+{
+	return resource->release == release_transaction;
+}
+
 static void schedule_iso_resource(struct iso_resource *r, unsigned long delay)
 {
 	client_get(r->client);
@@ -173,12 +181,6 @@ static void schedule_iso_resource(struct iso_resource *r, unsigned long delay)
 		client_put(r->client);
 }
 
-static void schedule_if_iso_resource(struct client_resource *resource)
-{
-	if (is_iso_resource(resource))
-		schedule_iso_resource(to_iso_resource(resource), 0);
-}
-
 /*
  * dequeue_event() just kfree()'s the event, so the event has to be
  * the first field in a struct XYZ_event.
@@ -401,16 +403,11 @@ static void for_each_client(struct fw_device *device,
 		callback(c);
 }
 
-static int schedule_reallocations(int id, void *p, void *data)
-{
-	schedule_if_iso_resource(p);
-
-	return 0;
-}
-
 static void queue_bus_reset_event(struct client *client)
 {
 	struct bus_reset_event *e;
+	struct client_resource *resource;
+	int id;
 
 	e = kzalloc(sizeof(*e), GFP_KERNEL);
 	if (e == NULL)
@@ -423,7 +420,10 @@ static void queue_bus_reset_event(struct client *client)
 
 	guard(spinlock_irq)(&client->lock);
 
-	idr_for_each(&client->resource_idr, schedule_reallocations, client);
+	idr_for_each_entry(&client->resource_idr, resource, id) {
+		if (is_iso_resource(resource))
+			schedule_iso_resource(to_iso_resource(resource), 0);
+	}
 }
 
 void fw_device_cdev_update(struct fw_device *device)
@@ -518,7 +518,8 @@ static int add_client_resource(struct client *client,
 		if (ret >= 0) {
 			resource->handle = ret;
 			client_get(client);
-			schedule_if_iso_resource(resource);
+			if (is_iso_resource(resource))
+				schedule_iso_resource(to_iso_resource(resource), 0);
 		}
 	}
 
@@ -1835,35 +1836,27 @@ static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma)
 	return ret;
 }
 
-static int is_outbound_transaction_resource(int id, void *p, void *data)
+static bool has_outbound_transactions(struct client *client)
 {
-	struct client_resource *resource = p;
-
-	return resource->release == release_transaction;
-}
+	struct client_resource *resource;
+	int id;
 
-static int has_outbound_transactions(struct client *client)
-{
 	guard(spinlock_irq)(&client->lock);
 
-	return idr_for_each(&client->resource_idr, is_outbound_transaction_resource, NULL);
-}
-
-static int shutdown_resource(int id, void *p, void *data)
-{
-	struct client_resource *resource = p;
-	struct client *client = data;
-
-	resource->release(client, resource);
-	client_put(client);
+	idr_for_each_entry(&client->resource_idr, resource, id) {
+		if (is_outbound_transaction_resource(resource))
+			return true;
+	}
 
-	return 0;
+	return false;
 }
 
 static int fw_device_op_release(struct inode *inode, struct file *file)
 {
 	struct client *client = file->private_data;
 	struct event *event, *next_event;
+	struct client_resource *resource;
+	int id;
 
 	scoped_guard(spinlock_irq, &client->device->card->lock)
 		list_del(&client->phy_receiver_link);
@@ -1883,7 +1876,10 @@ static int fw_device_op_release(struct inode *inode, struct file *file)
 
 	wait_event(client->tx_flush_wait, !has_outbound_transactions(client));
 
-	idr_for_each(&client->resource_idr, shutdown_resource, client);
+	idr_for_each_entry(&client->resource_idr, resource, id) {
+		resource->release(client, resource);
+		client_put(client);
+	}
 	idr_destroy(&client->resource_idr);
 
 	list_for_each_entry_safe(event, next_event, &client->event_list, link)
-- 
2.43.0

[PATCH 5/5] firewire: core: use xarray instead of idr to maintain client resource

[Takashi Sakamoto]

In core function, the instances of some client resource structures are
maintained by IDR. As of kernel v6.0, IDR has been superseded by XArray
and deprecated.

This commit replaces the usage of IDR with XArray to maintain the
resource instances. The instance of XArray is allocated per client with
XA_FLAGS_ALLOC1 so that the index of allocated entry is greater than zero
and returns to user space client as handle of the resource.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-cdev.c | 66 +++++++++++++++++++-----------------
 drivers/firewire/core.h      |  1 -
 2 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 83d25327c1d3..3ea220d96c31 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -14,7 +14,6 @@
 #include <linux/errno.h>
 #include <linux/firewire.h>
 #include <linux/firewire-cdev.h>
-#include <linux/idr.h>
 #include <linux/irqflags.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
@@ -54,7 +53,7 @@ struct client {
 
 	spinlock_t lock;
 	bool in_shutdown;
-	struct idr resource_idr;
+	struct xarray resource_xa;
 	struct list_head event_list;
 	wait_queue_head_t wait;
 	wait_queue_head_t tx_flush_wait;
@@ -297,7 +296,7 @@ static int fw_device_op_open(struct inode *inode, struct file *file)
 
 	client->device = device;
 	spin_lock_init(&client->lock);
-	idr_init(&client->resource_idr);
+	xa_init_flags(&client->resource_xa, XA_FLAGS_ALLOC1 | XA_FLAGS_LOCK_BH);
 	INIT_LIST_HEAD(&client->event_list);
 	init_waitqueue_head(&client->wait);
 	init_waitqueue_head(&client->tx_flush_wait);
@@ -407,7 +406,7 @@ static void queue_bus_reset_event(struct client *client)
 {
 	struct bus_reset_event *e;
 	struct client_resource *resource;
-	int id;
+	unsigned long index;
 
 	e = kzalloc(sizeof(*e), GFP_KERNEL);
 	if (e == NULL)
@@ -420,7 +419,7 @@ static void queue_bus_reset_event(struct client *client)
 
 	guard(spinlock_irq)(&client->lock);
 
-	idr_for_each_entry(&client->resource_idr, resource, id) {
+	xa_for_each(&client->resource_xa, index, resource) {
 		if (is_iso_resource(resource))
 			schedule_iso_resource(to_iso_resource(resource), 0);
 	}
@@ -501,31 +500,33 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 	return ret ? -EFAULT : 0;
 }
 
-static int add_client_resource(struct client *client,
-			       struct client_resource *resource, gfp_t gfp_mask)
+static int add_client_resource(struct client *client, struct client_resource *resource,
+			       gfp_t gfp_mask)
 {
-	bool preload = gfpflags_allow_blocking(gfp_mask);
 	int ret;
 
-	if (preload)
-		idr_preload(gfp_mask);
-
 	scoped_guard(spinlock_irqsave, &client->lock) {
-		if (client->in_shutdown)
+		u32 index;
+
+		if (client->in_shutdown) {
 			ret = -ECANCELED;
-		else
-			ret = idr_alloc(&client->resource_idr, resource, 0, 0, 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 = ret;
+			resource->handle = index;
 			client_get(client);
 			if (is_iso_resource(resource))
 				schedule_iso_resource(to_iso_resource(resource), 0);
 		}
 	}
 
-	if (preload)
-		idr_preload_end();
-
 	return ret < 0 ? ret : 0;
 }
 
@@ -533,17 +534,18 @@ static int release_client_resource(struct client *client, u32 handle,
 				   client_resource_release_fn_t release,
 				   struct client_resource **return_resource)
 {
+	unsigned long index = handle;
 	struct client_resource *resource;
 
 	scoped_guard(spinlock_irq, &client->lock) {
 		if (client->in_shutdown)
 			return -EINVAL;
 
-		resource = idr_find(&client->resource_idr, handle);
+		resource = xa_load(&client->resource_xa, index);
 		if (!resource || resource->release != release)
 			return -EINVAL;
 
-		idr_remove(&client->resource_idr, handle);
+		xa_erase(&client->resource_xa, handle);
 	}
 
 	if (return_resource)
@@ -566,9 +568,10 @@ static void complete_transaction(struct fw_card *card, int rcode, u32 request_ts
 {
 	struct outbound_transaction_event *e = data;
 	struct client *client = e->client;
+	unsigned long index = e->r.resource.handle;
 
 	scoped_guard(spinlock_irqsave, &client->lock) {
-		idr_remove(&client->resource_idr, e->r.resource.handle);
+		xa_erase(&client->resource_xa, index);
 		if (client->in_shutdown)
 			wake_up(&client->tx_flush_wait);
 	}
@@ -619,7 +622,7 @@ static void complete_transaction(struct fw_card *card, int rcode, u32 request_ts
 		break;
 	}
 
-	/* Drop the idr's reference */
+	// Drop the xarray's reference.
 	client_put(client);
 }
 
@@ -1317,6 +1320,7 @@ static void iso_resource_work(struct work_struct *work)
 	struct iso_resource *r =
 			container_of(work, struct iso_resource, work.work);
 	struct client *client = r->client;
+	unsigned long index = r->resource.handle;
 	int generation, channel, bandwidth, todo;
 	bool skip, free, success;
 
@@ -1351,7 +1355,7 @@ static void iso_resource_work(struct work_struct *work)
 			todo == ISO_RES_ALLOC_ONCE);
 	/*
 	 * Is this generation outdated already?  As long as this resource sticks
-	 * in the idr, it will be scheduled again for a newer generation or at
+	 * in the xarray, it will be scheduled again for a newer generation or at
 	 * shutdown.
 	 */
 	if (channel == -EAGAIN &&
@@ -1366,10 +1370,10 @@ static void iso_resource_work(struct work_struct *work)
 		if (r->todo == ISO_RES_ALLOC)
 			r->todo = ISO_RES_REALLOC;
 		// Allocation or reallocation failure?  Pull this resource out of the
-		// idr and prepare for deletion, unless the client is shutting down.
+		// xarray and prepare for deletion, unless the client is shutting down.
 		if (r->todo == ISO_RES_REALLOC && !success &&
 		    !client->in_shutdown &&
-		    idr_remove(&client->resource_idr, r->resource.handle)) {
+		    xa_erase(&client->resource_xa, index)) {
 			client_put(client);
 			free = true;
 		}
@@ -1839,11 +1843,11 @@ static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma)
 static bool has_outbound_transactions(struct client *client)
 {
 	struct client_resource *resource;
-	int id;
+	unsigned long index;
 
 	guard(spinlock_irq)(&client->lock);
 
-	idr_for_each_entry(&client->resource_idr, resource, id) {
+	xa_for_each(&client->resource_xa, index, resource) {
 		if (is_outbound_transaction_resource(resource))
 			return true;
 	}
@@ -1856,7 +1860,7 @@ static int fw_device_op_release(struct inode *inode, struct file *file)
 	struct client *client = file->private_data;
 	struct event *event, *next_event;
 	struct client_resource *resource;
-	int id;
+	unsigned long index;
 
 	scoped_guard(spinlock_irq, &client->device->card->lock)
 		list_del(&client->phy_receiver_link);
@@ -1870,17 +1874,17 @@ static int fw_device_op_release(struct inode *inode, struct file *file)
 	if (client->buffer.pages)
 		fw_iso_buffer_destroy(&client->buffer, client->device->card);
 
-	/* Freeze client->resource_idr and client->event_list */
+	// Freeze client->resource_xa and client->event_list.
 	scoped_guard(spinlock_irq, &client->lock)
 		client->in_shutdown = true;
 
 	wait_event(client->tx_flush_wait, !has_outbound_transactions(client));
 
-	idr_for_each_entry(&client->resource_idr, resource, id) {
+	xa_for_each(&client->resource_xa, index, resource) {
 		resource->release(client, resource);
 		client_put(client);
 	}
-	idr_destroy(&client->resource_idr);
+	xa_destroy(&client->resource_xa);
 
 	list_for_each_entry_safe(event, next_event, &client->event_list, link)
 		kfree(event);
diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 8cace026090c..57d101c01e36 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -7,7 +7,6 @@
 #include <linux/dma-mapping.h>
 #include <linux/fs.h>
 #include <linux/list.h>
-#include <linux/idr.h>
 #include <linux/xarray.h>
 #include <linux/mm_types.h>
 #include <linux/rwsem.h>
-- 
2.43.0

Re: [PATCH 0/5] firewire: core: use XArray to maintain client resources

[Takashi Sakamoto]

On Tue, Aug 13, 2024 at 08:52:05AM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> In core function, the instances of several types of client resources
> are maintained by IDR, and the index value of each resource is passed to
> user space application as handle. As of kernel v6.0, IDR has been
> superseded by XArray and deprecated.
> 
> This series of changes is to obsolete the usage of IDR with XArray.
> 
> Takashi Sakamoto (5):
>   firewire: core: minor code refactoring to release client resource
>   firewire: core: add helper functions to convert to parent resource
>     structure
>   firewire: core: add helper function to detect data of iso resource
>     structure
>   firewire: core: code refactoring to use idr_for_each_entry() macro
>     instead of idr_for_each() function
>   firewire: core: use xarray instead of idr to maintain client resource
> 
>  drivers/firewire/core-cdev.c | 170 +++++++++++++++++++----------------
>  drivers/firewire/core.h      |   1 -
>  2 files changed, 94 insertions(+), 77 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto