[PATCH 0/8] firewire: core/ohci: code refactoring for pages dedicated to DMA

[PATCH 0/8] firewire: core/ohci: code refactoring for pages dedicated to DMA

[Takashi Sakamoto]

Hi,

This patchset is to refactor current codes handling pages dedicated to
DMA.

There are two cases to acquires pages dedicated to DMA; isochronous
contexts and AR contexts. The reason of page acquisition is the need to map
them into VMA. In the former case, they are mapped into userspace VMA. In
the latter case, they are mapped into kernel VMA with continuous address
to access to packet content across the page boundaries, especially between
the end and the beginning of pages. The allocated pages are
discontiguous, and their cache coherency is managed by DMA streaming
APIs.

1394 OHCI has no restriction about the size and alignment of memory
registered for DMA, while the registered DMA address should be within
32 bit. The DMA mapped addresses can be discontiguous between descriptor,
thus the above subsystem design is due to the convenience of system side.

I think the recent convention of driver programming relies on DMA-coherent
buffers, while this code refactoring keeps the uses of DMA streaming
APIs as is. Also, the acquisition per page is kept as is, even if there
are some ways to allocate memories wider than the page size. They would be
future works since it requires to change how to handle the packet content
across the pages.


Takashi Sakamoto (8):
  firewire: core: move private function declaration from public header
    to internal header
  firewire: core: use mutex instead of spinlock for client isochronous
    context
  firewire: core: code refactoring with cleanup function for isoc pages
  firewire: core: use common kernel API to allocate and release a batch
    of pages
  firewire: core: stop using page private to store DMA mapping address
  firewire: ohci: use MAX macro to guarantee minimum count of pages for
    AR contexts
  firewire: ohci: split page allocation from dma mapping
  firewire: ohci: stop using page private to store DMA mapping address

 drivers/firewire/core-cdev.c |  41 ++++++------
 drivers/firewire/core-iso.c  |  86 +++++++++++++-----------
 drivers/firewire/core.h      |   1 +
 drivers/firewire/ohci.c      | 124 ++++++++++++++++++++---------------
 include/linux/firewire.h     |   3 +-
 5 files changed, 141 insertions(+), 114 deletions(-)


base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
-- 
2.51.0

[PATCH 1/8] firewire: core: move private function declaration from public header to internal header

[Takashi Sakamoto]

The fw_iso_buffer_lookup function is used by core module only, thus no
need to describe its prototype in kernel internal header.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core.h  | 1 +
 include/linux/firewire.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 41fb39d9a4e6..26868f007131 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -166,6 +166,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event);
 int fw_iso_buffer_alloc(struct fw_iso_buffer *buffer, int page_count);
 int fw_iso_buffer_map_dma(struct fw_iso_buffer *buffer, struct fw_card *card,
 			  enum dma_data_direction direction);
+size_t fw_iso_buffer_lookup(struct fw_iso_buffer *buffer, dma_addr_t completed);
 
 static inline void fw_iso_context_init_work(struct fw_iso_context *ctx, work_func_t func)
 {
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 6143b7d28eac..aa84421b58ac 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -533,7 +533,6 @@ struct fw_iso_buffer {
 int fw_iso_buffer_init(struct fw_iso_buffer *buffer, struct fw_card *card,
 		       int page_count, enum dma_data_direction direction);
 void fw_iso_buffer_destroy(struct fw_iso_buffer *buffer, struct fw_card *card);
-size_t fw_iso_buffer_lookup(struct fw_iso_buffer *buffer, dma_addr_t completed);
 
 struct fw_iso_context;
 typedef void (*fw_iso_callback_t)(struct fw_iso_context *context,
-- 
2.51.0

[PATCH 2/8] firewire: core: use mutex instead of spinlock for client isochronous context

[Takashi Sakamoto]

There is a restriction that the userspace client associated to a file
descriptor can hold one isochronous context. The client-level spinning
lock is used to guarantee it, however the lock is also used for
multi-purposes. Additionally, there is no need to use this type of lock,
and the mutex is available, instead.

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

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 49dc1612c691..2b8a878c8aae 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -63,6 +63,7 @@ struct client {
 	u64 bus_reset_closure;
 
 	struct fw_iso_context *iso_context;
+	struct mutex iso_context_mutex;
 	u64 iso_closure;
 	struct fw_iso_buffer buffer;
 	unsigned long vm_start;
@@ -306,6 +307,7 @@ static int fw_device_op_open(struct inode *inode, struct file *file)
 	INIT_LIST_HEAD(&client->phy_receiver_link);
 	INIT_LIST_HEAD(&client->link);
 	kref_init(&client->kref);
+	mutex_init(&client->iso_context_mutex);
 
 	file->private_data = client;
 
@@ -1088,26 +1090,27 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
 		context->drop_overflow_headers = true;
 
 	// We only support one context at this time.
-	guard(spinlock_irq)(&client->lock);
-
-	if (client->iso_context != NULL) {
-		fw_iso_context_destroy(context);
-
-		return -EBUSY;
-	}
-	if (!client->buffer_is_mapped) {
-		ret = fw_iso_buffer_map_dma(&client->buffer,
-					    client->device->card,
-					    iso_dma_direction(context));
-		if (ret < 0) {
+	scoped_guard(mutex, &client->iso_context_mutex) {
+		if (client->iso_context != NULL) {
 			fw_iso_context_destroy(context);
 
-			return ret;
+			return -EBUSY;
 		}
-		client->buffer_is_mapped = true;
+		// The DMA mapping operation is available if the buffer is already allocated by
+		// mmap(2) system call. If not, it is delegated to the system call.
+		if (!client->buffer_is_mapped) {
+			ret = fw_iso_buffer_map_dma(&client->buffer, client->device->card,
+						    iso_dma_direction(context));
+			if (ret < 0) {
+				fw_iso_context_destroy(context);
+
+				return ret;
+			}
+			client->buffer_is_mapped = true;
+		}
+		client->iso_closure = a->closure;
+		client->iso_context = context;
 	}
-	client->iso_closure = a->closure;
-	client->iso_context = context;
 
 	a->handle = 0;
 
@@ -1826,7 +1829,9 @@ static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma)
 	if (ret < 0)
 		return ret;
 
-	scoped_guard(spinlock_irq, &client->lock) {
+	scoped_guard(mutex, &client->iso_context_mutex) {
+		// The direction of DMA can be determined if the isochronous context is already
+		// allocated. If not, the DMA mapping operation is postponed after the allocation.
 		if (client->iso_context) {
 			ret = fw_iso_buffer_map_dma(&client->buffer, client->device->card,
 						    iso_dma_direction(client->iso_context));
@@ -1879,6 +1884,7 @@ static int fw_device_op_release(struct inode *inode, struct file *file)
 
 	if (client->iso_context)
 		fw_iso_context_destroy(client->iso_context);
+	mutex_destroy(&client->iso_context_mutex);
 
 	if (client->buffer.pages)
 		fw_iso_buffer_destroy(&client->buffer, client->device->card);
-- 
2.51.0

[PATCH 3/8] firewire: core: code refactoring with cleanup function for isoc pages

[Takashi Sakamoto]

This commit refactors the implementation to allocate pages for isochronous
DMA contexts with cleanup function.

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

diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
index a67493862c85..e678ffba5de2 100644
--- a/drivers/firewire/core-iso.c
+++ b/drivers/firewire/core-iso.c
@@ -30,26 +30,29 @@
 
 int fw_iso_buffer_alloc(struct fw_iso_buffer *buffer, int page_count)
 {
+	struct page **page_array __free(kfree) = kcalloc(page_count, sizeof(page_array[0]), GFP_KERNEL);
 	int i;
 
-	buffer->page_count = 0;
-	buffer->page_count_mapped = 0;
-	buffer->pages = kmalloc_array(page_count, sizeof(buffer->pages[0]),
-				      GFP_KERNEL);
-	if (buffer->pages == NULL)
+	if (!page_array)
 		return -ENOMEM;
 
-	for (i = 0; i < page_count; i++) {
-		buffer->pages[i] = alloc_page(GFP_KERNEL | GFP_DMA32 | __GFP_ZERO);
-		if (buffer->pages[i] == NULL)
+	for (i = 0; i < page_count; ++i) {
+		struct page *page = alloc_page(GFP_KERNEL | GFP_DMA32 | __GFP_ZERO);
+
+		if (!page)
 			break;
+		page_array[i] = page;
 	}
-	buffer->page_count = i;
+
 	if (i < page_count) {
-		fw_iso_buffer_destroy(buffer, NULL);
+		while (i-- > 0)
+			__free_page(page_array[i]);
 		return -ENOMEM;
 	}
 
+	buffer->page_count = page_count;
+	buffer->pages = no_free_ptr(page_array);
+
 	return 0;
 }
 
@@ -104,11 +107,14 @@ void fw_iso_buffer_destroy(struct fw_iso_buffer *buffer,
 		dma_unmap_page(card->device, address,
 			       PAGE_SIZE, buffer->direction);
 	}
-	for (i = 0; i < buffer->page_count; i++)
-		__free_page(buffer->pages[i]);
 
-	kfree(buffer->pages);
-	buffer->pages = NULL;
+	if (buffer->pages) {
+		for (int i = 0; i < buffer->page_count; ++i)
+			__free_page(buffer->pages[i]);
+		kfree(buffer->pages);
+		buffer->pages = NULL;
+	}
+
 	buffer->page_count = 0;
 	buffer->page_count_mapped = 0;
 }
-- 
2.51.0

[PATCH 4/8] firewire: core: use common kernel API to allocate and release a batch of pages

[Takashi Sakamoto]

The pair of alloc_pages_bulk() and release_pages() are convenient to
allocate and release a batch of pages.

This commit utilizes the pair to maintain pages for isochronous DMA
context.

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

diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
index e678ffba5de2..f2e35ac7a476 100644
--- a/drivers/firewire/core-iso.c
+++ b/drivers/firewire/core-iso.c
@@ -31,22 +31,18 @@
 int fw_iso_buffer_alloc(struct fw_iso_buffer *buffer, int page_count)
 {
 	struct page **page_array __free(kfree) = kcalloc(page_count, sizeof(page_array[0]), GFP_KERNEL);
-	int i;
 
 	if (!page_array)
 		return -ENOMEM;
 
-	for (i = 0; i < page_count; ++i) {
-		struct page *page = alloc_page(GFP_KERNEL | GFP_DMA32 | __GFP_ZERO);
-
-		if (!page)
-			break;
-		page_array[i] = page;
-	}
-
-	if (i < page_count) {
-		while (i-- > 0)
-			__free_page(page_array[i]);
+	// Retrieve noncontiguous pages. The descriptors for 1394 OHCI isochronous DMA contexts
+	// have a set of address and length per each, while the reason to use pages is the
+	// convenience to map them into virtual address space of user process.
+	unsigned long nr_populated = alloc_pages_bulk(GFP_KERNEL | GFP_DMA32 | __GFP_ZERO,
+						      page_count, page_array);
+	if (nr_populated != page_count) {
+		// Assuming the above call fills page_array sequentially from the beginning.
+		release_pages(page_array, nr_populated);
 		return -ENOMEM;
 	}
 
@@ -64,7 +60,10 @@ int fw_iso_buffer_map_dma(struct fw_iso_buffer *buffer, struct fw_card *card,
 
 	buffer->direction = direction;
 
+	// Retrieve DMA mapping addresses for the pages. They are not contiguous. Maintain the cache
+	// coherency for the pages by hand.
 	for (i = 0; i < buffer->page_count; i++) {
+		// The dma_map_phys() with a physical address per page is available here, instead.
 		address = dma_map_page(card->device, buffer->pages[i],
 				       0, PAGE_SIZE, direction);
 		if (dma_mapping_error(card->device, address))
@@ -109,8 +108,7 @@ void fw_iso_buffer_destroy(struct fw_iso_buffer *buffer,
 	}
 
 	if (buffer->pages) {
-		for (int i = 0; i < buffer->page_count; ++i)
-			__free_page(buffer->pages[i]);
+		release_pages(buffer->pages, buffer->page_count);
 		kfree(buffer->pages);
 		buffer->pages = NULL;
 	}
-- 
2.51.0

[PATCH 5/8] firewire: core: stop using page private to store DMA mapping address

[Takashi Sakamoto]

There is a long discussion about the use of private field in page
structure between Linux kernel developers.

This commit stop using page private to store DMA mapping address for
isochronous context, to prepare for mm future change.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-cdev.c |  5 +---
 drivers/firewire/core-iso.c  | 48 +++++++++++++++++++-----------------
 drivers/firewire/ohci.c      | 24 +++++++++---------
 include/linux/firewire.h     |  2 +-
 4 files changed, 39 insertions(+), 40 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 2b8a878c8aae..bb4d0f938f5b 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -67,7 +67,6 @@ struct client {
 	u64 iso_closure;
 	struct fw_iso_buffer buffer;
 	unsigned long vm_start;
-	bool buffer_is_mapped;
 
 	struct list_head phy_receiver_link;
 	u64 phy_receiver_closure;
@@ -1098,7 +1097,7 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
 		}
 		// The DMA mapping operation is available if the buffer is already allocated by
 		// mmap(2) system call. If not, it is delegated to the system call.
-		if (!client->buffer_is_mapped) {
+		if (client->buffer.pages && !client->buffer.dma_addrs) {
 			ret = fw_iso_buffer_map_dma(&client->buffer, client->device->card,
 						    iso_dma_direction(context));
 			if (ret < 0) {
@@ -1106,7 +1105,6 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
 
 				return ret;
 			}
-			client->buffer_is_mapped = true;
 		}
 		client->iso_closure = a->closure;
 		client->iso_context = context;
@@ -1837,7 +1835,6 @@ static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma)
 						    iso_dma_direction(client->iso_context));
 			if (ret < 0)
 				goto fail;
-			client->buffer_is_mapped = true;
 		}
 	}
 
diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
index f2e35ac7a476..3f36243ec0c1 100644
--- a/drivers/firewire/core-iso.c
+++ b/drivers/firewire/core-iso.c
@@ -55,25 +55,32 @@ int fw_iso_buffer_alloc(struct fw_iso_buffer *buffer, int page_count)
 int fw_iso_buffer_map_dma(struct fw_iso_buffer *buffer, struct fw_card *card,
 			  enum dma_data_direction direction)
 {
-	dma_addr_t address;
+	dma_addr_t *dma_addrs __free(kfree) = kcalloc(buffer->page_count, sizeof(dma_addrs[0]),
+						      GFP_KERNEL);
 	int i;
 
-	buffer->direction = direction;
+	if (!dma_addrs)
+		return -ENOMEM;
 
 	// Retrieve DMA mapping addresses for the pages. They are not contiguous. Maintain the cache
 	// coherency for the pages by hand.
 	for (i = 0; i < buffer->page_count; i++) {
 		// The dma_map_phys() with a physical address per page is available here, instead.
-		address = dma_map_page(card->device, buffer->pages[i],
-				       0, PAGE_SIZE, direction);
-		if (dma_mapping_error(card->device, address))
+		dma_addr_t dma_addr = dma_map_page(card->device, buffer->pages[i], 0, PAGE_SIZE,
+						   direction);
+		if (dma_mapping_error(card->device, dma_addr))
 			break;
 
-		set_page_private(buffer->pages[i], address);
+		dma_addrs[i] = dma_addr;
 	}
-	buffer->page_count_mapped = i;
-	if (i < buffer->page_count)
+	if (i < buffer->page_count) {
+		while (i-- > 0)
+			dma_unmap_page(card->device, dma_addrs[i], PAGE_SIZE, buffer->direction);
 		return -ENOMEM;
+	}
+
+	buffer->direction = direction;
+	buffer->dma_addrs = no_free_ptr(dma_addrs);
 
 	return 0;
 }
@@ -98,13 +105,13 @@ EXPORT_SYMBOL(fw_iso_buffer_init);
 void fw_iso_buffer_destroy(struct fw_iso_buffer *buffer,
 			   struct fw_card *card)
 {
-	int i;
-	dma_addr_t address;
-
-	for (i = 0; i < buffer->page_count_mapped; i++) {
-		address = page_private(buffer->pages[i]);
-		dma_unmap_page(card->device, address,
-			       PAGE_SIZE, buffer->direction);
+	if (buffer->dma_addrs) {
+		for (int i = 0; i < buffer->page_count; ++i) {
+			dma_addr_t dma_addr = buffer->dma_addrs[i];
+			dma_unmap_page(card->device, dma_addr, PAGE_SIZE, buffer->direction);
+		}
+		kfree(buffer->dma_addrs);
+		buffer->dma_addrs = NULL;
 	}
 
 	if (buffer->pages) {
@@ -114,20 +121,15 @@ void fw_iso_buffer_destroy(struct fw_iso_buffer *buffer,
 	}
 
 	buffer->page_count = 0;
-	buffer->page_count_mapped = 0;
 }
 EXPORT_SYMBOL(fw_iso_buffer_destroy);
 
 /* Convert DMA address to offset into virtually contiguous buffer. */
 size_t fw_iso_buffer_lookup(struct fw_iso_buffer *buffer, dma_addr_t completed)
 {
-	size_t i;
-	dma_addr_t address;
-	ssize_t offset;
-
-	for (i = 0; i < buffer->page_count; i++) {
-		address = page_private(buffer->pages[i]);
-		offset = (ssize_t)completed - (ssize_t)address;
+	for (int i = 0; i < buffer->page_count; i++) {
+		dma_addr_t dma_addr = buffer->dma_addrs[i];
+		ssize_t offset = (ssize_t)completed - (ssize_t)dma_addr;
 		if (offset > 0 && offset <= PAGE_SIZE)
 			return (i << PAGE_SHIFT) + offset;
 	}
diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index e3e78dc42530..68a336577d36 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -3184,7 +3184,7 @@ static int queue_iso_transmit(struct iso_context *ctx,
 	struct descriptor *d, *last, *pd;
 	struct fw_iso_packet *p;
 	__le32 *header;
-	dma_addr_t d_bus, page_bus;
+	dma_addr_t d_bus;
 	u32 z, header_z, payload_z, irq;
 	u32 payload_index, payload_end_index, next_page_index;
 	int page, end_page, i, length, offset;
@@ -3254,11 +3254,11 @@ static int queue_iso_transmit(struct iso_context *ctx,
 			min(next_page_index, payload_end_index) - payload_index;
 		pd[i].req_count    = cpu_to_le16(length);
 
-		page_bus = page_private(buffer->pages[page]);
-		pd[i].data_address = cpu_to_le32(page_bus + offset);
+		dma_addr_t dma_addr = buffer->dma_addrs[i];
+		pd[i].data_address = cpu_to_le32(dma_addr + offset);
 
 		dma_sync_single_range_for_device(ctx->context.ohci->card.device,
-						 page_bus, offset, length,
+						 dma_addr, offset, length,
 						 DMA_TO_DEVICE);
 
 		payload_index += length;
@@ -3287,7 +3287,7 @@ static int queue_iso_packet_per_buffer(struct iso_context *ctx,
 {
 	struct device *device = ctx->context.ohci->card.device;
 	struct descriptor *d, *pd;
-	dma_addr_t d_bus, page_bus;
+	dma_addr_t d_bus;
 	u32 z, header_z, rest;
 	int i, j, length;
 	int page, offset, packet_count, header_size, payload_per_buffer;
@@ -3337,10 +3337,10 @@ static int queue_iso_packet_per_buffer(struct iso_context *ctx,
 			pd->res_count = pd->req_count;
 			pd->transfer_status = 0;
 
-			page_bus = page_private(buffer->pages[page]);
-			pd->data_address = cpu_to_le32(page_bus + offset);
+			dma_addr_t dma_addr = buffer->dma_addrs[page];
+			pd->data_address = cpu_to_le32(dma_addr + offset);
 
-			dma_sync_single_range_for_device(device, page_bus,
+			dma_sync_single_range_for_device(device, dma_addr,
 							 offset, length,
 							 DMA_FROM_DEVICE);
 
@@ -3367,7 +3367,7 @@ static int queue_iso_buffer_fill(struct iso_context *ctx,
 				 unsigned long payload)
 {
 	struct descriptor *d;
-	dma_addr_t d_bus, page_bus;
+	dma_addr_t d_bus;
 	int page, offset, rest, z, i, length;
 
 	page   = payload >> PAGE_SHIFT;
@@ -3400,11 +3400,11 @@ static int queue_iso_buffer_fill(struct iso_context *ctx,
 		d->res_count = d->req_count;
 		d->transfer_status = 0;
 
-		page_bus = page_private(buffer->pages[page]);
-		d->data_address = cpu_to_le32(page_bus + offset);
+		dma_addr_t dma_addr = buffer->dma_addrs[page];
+		d->data_address = cpu_to_le32(dma_addr + offset);
 
 		dma_sync_single_range_for_device(ctx->context.ohci->card.device,
-						 page_bus, offset, length,
+						 dma_addr, offset, length,
 						 DMA_FROM_DEVICE);
 
 		rest -= length;
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index aa84421b58ac..09c8484f7430 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -526,8 +526,8 @@ struct fw_iso_packet {
 struct fw_iso_buffer {
 	enum dma_data_direction direction;
 	struct page **pages;
+	dma_addr_t *dma_addrs;
 	int page_count;
-	int page_count_mapped;
 };
 
 int fw_iso_buffer_init(struct fw_iso_buffer *buffer, struct fw_card *card,
-- 
2.51.0

[PATCH 6/8] firewire: ohci: use MAX macro to guarantee minimum count of pages for AR contexts

[Takashi Sakamoto]

The computation of page size for AR DMA context can be simplified by MAX
macro.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/ohci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 68a336577d36..f6da4cd7d123 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -86,7 +86,7 @@ struct descriptor {
 #define AR_BUFFER_SIZE	(32*1024)
 #define AR_BUFFERS_MIN	DIV_ROUND_UP(AR_BUFFER_SIZE, PAGE_SIZE)
 /* we need at least two pages for proper list management */
-#define AR_BUFFERS	(AR_BUFFERS_MIN >= 2 ? AR_BUFFERS_MIN : 2)
+#define AR_BUFFERS	MAX(2, AR_BUFFERS_MIN)
 
 #define MAX_ASYNC_PAYLOAD	4096
 #define MAX_AR_PACKET_SIZE	(16 + MAX_ASYNC_PAYLOAD + 4)
-- 
2.51.0

[PATCH 7/8] firewire: ohci: split page allocation from dma mapping

[Takashi Sakamoto]

1394 OHCI PCI driver had long been the only user of dma_alloc_pages().
Although tee subsystem recently started using it, they are still a few
users of the infrequently-used function.

In the discussion for dma-mapping function, Jason Gunthorpe shows his
opinion about the design of public API for the function. According to it,
the users provide physical address to the function, then receive DMA
mapping address, regardless of the location of mapping target.

With the above aspects, this commit eliminates the use of
dma_alloc_pages() from this subsystem.

Link: https://lore.kernel.org/lkml/20250905174324.GI616306@nvidia.com/
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/ohci.c | 77 +++++++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 26 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index f6da4cd7d123..e34965acb925 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -539,18 +539,22 @@ static void ar_context_link_page(struct ar_context *ctx, unsigned int index)
 static void ar_context_release(struct ar_context *ctx)
 {
 	struct device *dev = ctx->ohci->card.device;
-	unsigned int i;
 
 	if (!ctx->buffer)
 		return;
 
-	vunmap(ctx->buffer);
+	for (int i = 0; i < AR_BUFFERS; ++i) {
+		dma_addr_t dma_addr = page_private(ctx->pages[i]);
 
-	for (i = 0; i < AR_BUFFERS; i++) {
-		if (ctx->pages[i])
-			dma_free_pages(dev, PAGE_SIZE, ctx->pages[i],
-				       ar_buffer_bus(ctx, i), DMA_FROM_DEVICE);
+		dma_unmap_page(dev, dma_addr, PAGE_SIZE, DMA_FROM_DEVICE);
+		set_page_private(ctx->pages[i], 0);
 	}
+
+	vunmap(ctx->buffer);
+	ctx->buffer = NULL;
+
+	release_pages(ctx->pages, AR_BUFFERS);
+	memset(ctx->pages, 0, sizeof(ctx->pages));
 }
 
 static void ar_context_abort(struct ar_context *ctx, const char *error_msg)
@@ -845,31 +849,57 @@ static int ar_context_init(struct ar_context *ctx, struct fw_ohci *ohci,
 {
 	struct device *dev = ohci->card.device;
 	unsigned int i;
-	dma_addr_t dma_addr;
 	struct page *pages[AR_BUFFERS + AR_WRAPAROUND_PAGES];
+	void *vaddr;
 	struct descriptor *d;
 
 	ctx->regs        = regs;
 	ctx->ohci        = ohci;
 	INIT_WORK(&ctx->work, ohci_ar_context_work);
 
-	for (i = 0; i < AR_BUFFERS; i++) {
-		ctx->pages[i] = dma_alloc_pages(dev, PAGE_SIZE, &dma_addr,
-						DMA_FROM_DEVICE, GFP_KERNEL);
-		if (!ctx->pages[i])
-			goto out_of_memory;
-		set_page_private(ctx->pages[i], dma_addr);
-		dma_sync_single_for_device(dev, dma_addr, PAGE_SIZE,
-					   DMA_FROM_DEVICE);
+	// Retrieve noncontiguous pages. The descriptors for 1394 OHCI AR DMA contexts have a set
+	// of address and length per each. The reason to use pages is to construct contiguous
+	// address range in kernel virtual address space.
+	unsigned long nr_populated = alloc_pages_bulk(GFP_KERNEL | GFP_DMA32, AR_BUFFERS, pages);
+
+	if (nr_populated != AR_BUFFERS) {
+		release_pages(pages, nr_populated);
+		return -ENOMEM;
 	}
 
-	for (i = 0; i < AR_BUFFERS; i++)
-		pages[i]              = ctx->pages[i];
+	// Map the pages into contiguous kernel virtual addresses so that the packet data
+	// across the pages can be referred as being contiguous, especially across the last
+	// and first pages.
 	for (i = 0; i < AR_WRAPAROUND_PAGES; i++)
-		pages[AR_BUFFERS + i] = ctx->pages[i];
-	ctx->buffer = vmap(pages, ARRAY_SIZE(pages), VM_MAP, PAGE_KERNEL);
-	if (!ctx->buffer)
-		goto out_of_memory;
+		pages[AR_BUFFERS + i] = pages[i];
+	vaddr = vmap(pages, ARRAY_SIZE(pages), VM_MAP, PAGE_KERNEL);
+	if (!vaddr) {
+		release_pages(pages, nr_populated);
+		return -ENOMEM;
+	}
+
+	// Retrieve DMA mapping addresses for the pages. They are not contiguous. Maintain the cache
+	// coherency for the pages by hand.
+	for (i = 0; i < AR_BUFFERS; i++) {
+		// The dma_map_phys() with a physical address per page is available here, instead.
+		dma_addr_t dma_addr = dma_map_page(dev, pages[i], 0, PAGE_SIZE, DMA_FROM_DEVICE);
+		if (dma_mapping_error(dev, dma_addr))
+			break;
+		set_page_private(pages[i], dma_addr);
+		dma_sync_single_for_device(dev, dma_addr, PAGE_SIZE, DMA_FROM_DEVICE);
+	}
+	if (i < AR_BUFFERS) {
+		while (i-- > 0) {
+			dma_addr_t dma_addr = page_private(pages[i]);
+			dma_unmap_page(dev, dma_addr, PAGE_SIZE, DMA_FROM_DEVICE);
+		}
+		vunmap(vaddr);
+		release_pages(pages, nr_populated);
+		return -ENOMEM;
+	}
+
+	ctx->buffer = vaddr;
+	memcpy(ctx->pages, pages, sizeof(ctx->pages));
 
 	ctx->descriptors     = ohci->misc_buffer     + descriptors_offset;
 	ctx->descriptors_bus = ohci->misc_buffer_bus + descriptors_offset;
@@ -886,11 +916,6 @@ static int ar_context_init(struct ar_context *ctx, struct fw_ohci *ohci,
 	}
 
 	return 0;
-
-out_of_memory:
-	ar_context_release(ctx);
-
-	return -ENOMEM;
 }
 
 static void ar_context_run(struct ar_context *ctx)
-- 
2.51.0

[PATCH 8/8] firewire: ohci: stop using page private to store DMA mapping address

[Takashi Sakamoto]

There is a long discussion about the use of private field in page
structure between Linux kernel developers.

This commit stop using page private to store DMA mapping address for
AR context, to prepare for mm future change.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/ohci.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index e34965acb925..a43fe680d508 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -96,6 +96,7 @@ struct ar_context {
 	struct fw_ohci *ohci;
 	struct page *pages[AR_BUFFERS];
 	void *buffer;
+	dma_addr_t dma_addrs[AR_BUFFERS];
 	struct descriptor *descriptors;
 	dma_addr_t descriptors_bus;
 	void *pointer;
@@ -513,11 +514,6 @@ static int ohci_update_phy_reg(struct fw_card *card, int addr,
 	return update_phy_reg(ohci, addr, clear_bits, set_bits);
 }
 
-static inline dma_addr_t ar_buffer_bus(struct ar_context *ctx, unsigned int i)
-{
-	return page_private(ctx->pages[i]);
-}
-
 static void ar_context_link_page(struct ar_context *ctx, unsigned int index)
 {
 	struct descriptor *d;
@@ -544,11 +540,11 @@ static void ar_context_release(struct ar_context *ctx)
 		return;
 
 	for (int i = 0; i < AR_BUFFERS; ++i) {
-		dma_addr_t dma_addr = page_private(ctx->pages[i]);
-
-		dma_unmap_page(dev, dma_addr, PAGE_SIZE, DMA_FROM_DEVICE);
-		set_page_private(ctx->pages[i], 0);
+		dma_addr_t dma_addr = ctx->dma_addrs[i];
+		if (dma_addr)
+			dma_unmap_page(dev, dma_addr, PAGE_SIZE, DMA_FROM_DEVICE);
 	}
+	memset(ctx->dma_addrs, 0, sizeof(ctx->dma_addrs));
 
 	vunmap(ctx->buffer);
 	ctx->buffer = NULL;
@@ -647,14 +643,12 @@ static void ar_sync_buffers_for_cpu(struct ar_context *ctx,
 
 	i = ar_first_buffer_index(ctx);
 	while (i != end_buffer_index) {
-		dma_sync_single_for_cpu(ctx->ohci->card.device,
-					ar_buffer_bus(ctx, i),
-					PAGE_SIZE, DMA_FROM_DEVICE);
+		dma_sync_single_for_cpu(ctx->ohci->card.device, ctx->dma_addrs[i], PAGE_SIZE,
+					DMA_FROM_DEVICE);
 		i = ar_next_buffer_index(i);
 	}
 	if (end_buffer_offset > 0)
-		dma_sync_single_for_cpu(ctx->ohci->card.device,
-					ar_buffer_bus(ctx, i),
+		dma_sync_single_for_cpu(ctx->ohci->card.device, ctx->dma_addrs[i],
 					end_buffer_offset, DMA_FROM_DEVICE);
 }
 
@@ -795,9 +789,8 @@ static void ar_recycle_buffers(struct ar_context *ctx, unsigned int end_buffer)
 
 	i = ar_first_buffer_index(ctx);
 	while (i != end_buffer) {
-		dma_sync_single_for_device(ctx->ohci->card.device,
-					   ar_buffer_bus(ctx, i),
-					   PAGE_SIZE, DMA_FROM_DEVICE);
+		dma_sync_single_for_device(ctx->ohci->card.device, ctx->dma_addrs[i], PAGE_SIZE,
+					   DMA_FROM_DEVICE);
 		ar_context_link_page(ctx, i);
 		i = ar_next_buffer_index(i);
 	}
@@ -850,6 +843,7 @@ static int ar_context_init(struct ar_context *ctx, struct fw_ohci *ohci,
 	struct device *dev = ohci->card.device;
 	unsigned int i;
 	struct page *pages[AR_BUFFERS + AR_WRAPAROUND_PAGES];
+	dma_addr_t dma_addrs[AR_BUFFERS];
 	void *vaddr;
 	struct descriptor *d;
 
@@ -885,19 +879,18 @@ static int ar_context_init(struct ar_context *ctx, struct fw_ohci *ohci,
 		dma_addr_t dma_addr = dma_map_page(dev, pages[i], 0, PAGE_SIZE, DMA_FROM_DEVICE);
 		if (dma_mapping_error(dev, dma_addr))
 			break;
-		set_page_private(pages[i], dma_addr);
+		dma_addrs[i] = dma_addr;
 		dma_sync_single_for_device(dev, dma_addr, PAGE_SIZE, DMA_FROM_DEVICE);
 	}
 	if (i < AR_BUFFERS) {
-		while (i-- > 0) {
-			dma_addr_t dma_addr = page_private(pages[i]);
-			dma_unmap_page(dev, dma_addr, PAGE_SIZE, DMA_FROM_DEVICE);
-		}
+		while (i-- > 0)
+			dma_unmap_page(dev, dma_addrs[i], PAGE_SIZE, DMA_FROM_DEVICE);
 		vunmap(vaddr);
 		release_pages(pages, nr_populated);
 		return -ENOMEM;
 	}
 
+	memcpy(ctx->dma_addrs, dma_addrs, sizeof(ctx->dma_addrs));
 	ctx->buffer = vaddr;
 	memcpy(ctx->pages, pages, sizeof(ctx->pages));
 
@@ -910,7 +903,7 @@ static int ar_context_init(struct ar_context *ctx, struct fw_ohci *ohci,
 		d->control        = cpu_to_le16(DESCRIPTOR_INPUT_MORE |
 						DESCRIPTOR_STATUS |
 						DESCRIPTOR_BRANCH_ALWAYS);
-		d->data_address   = cpu_to_le32(ar_buffer_bus(ctx, i));
+		d->data_address   = cpu_to_le32(ctx->dma_addrs[i]);
 		d->branch_address = cpu_to_le32(ctx->descriptors_bus +
 			ar_next_buffer_index(i) * sizeof(struct descriptor));
 	}
-- 
2.51.0

Re: [PATCH 0/8] firewire: core/ohci: code refactoring for pages dedicated to DMA

[Takashi Sakamoto]

On Sat, Jan 10, 2026 at 10:39:03AM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> This patchset is to refactor current codes handling pages dedicated to
> DMA.
> 
> There are two cases to acquires pages dedicated to DMA; isochronous
> contexts and AR contexts. The reason of page acquisition is the need to map
> them into VMA. In the former case, they are mapped into userspace VMA. In
> the latter case, they are mapped into kernel VMA with continuous address
> to access to packet content across the page boundaries, especially between
> the end and the beginning of pages. The allocated pages are
> discontiguous, and their cache coherency is managed by DMA streaming
> APIs.
> 
> 1394 OHCI has no restriction about the size and alignment of memory
> registered for DMA, while the registered DMA address should be within
> 32 bit. The DMA mapped addresses can be discontiguous between descriptor,
> thus the above subsystem design is due to the convenience of system side.
> 
> I think the recent convention of driver programming relies on DMA-coherent
> buffers, while this code refactoring keeps the uses of DMA streaming
> APIs as is. Also, the acquisition per page is kept as is, even if there
> are some ways to allocate memories wider than the page size. They would be
> future works since it requires to change how to handle the packet content
> across the pages.
> 
> 
> Takashi Sakamoto (8):
>   firewire: core: move private function declaration from public header
>     to internal header
>   firewire: core: use mutex instead of spinlock for client isochronous
>     context
>   firewire: core: code refactoring with cleanup function for isoc pages
>   firewire: core: use common kernel API to allocate and release a batch
>     of pages
>   firewire: core: stop using page private to store DMA mapping address
>   firewire: ohci: use MAX macro to guarantee minimum count of pages for
>     AR contexts
>   firewire: ohci: split page allocation from dma mapping
>   firewire: ohci: stop using page private to store DMA mapping address
> 
>  drivers/firewire/core-cdev.c |  41 ++++++------
>  drivers/firewire/core-iso.c  |  86 +++++++++++++-----------
>  drivers/firewire/core.h      |   1 +
>  drivers/firewire/ohci.c      | 124 ++++++++++++++++++++---------------
>  include/linux/firewire.h     |   3 +-
>  5 files changed, 141 insertions(+), 114 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto