Re: [PATCH 2/6] firewire: isochronous and asynchronous I/O

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Christoph Hellwig wrote:

+	for (i = 0; i < buffer->page_count; i++) {
+		buffer->pages[i] = alloc_page(GFP_KERNEL | GFP_DMA32 | __GFP_ZERO);
+		if (buffer->pages[i] == NULL)
+			goto out_pages;
+
+		address = dma_map_page(card->device, buffer->pages[i],
+				       0, PAGE_SIZE, direction);
+		if (dma_mapping_error(address)) {
+			__free_page(buffer->pages[i]);
+			goto out_pages;
+		}


Are you sure using streaming dma mapping is safe here?  I don't see
actual user in this patch, but doing the proper ownership protocol
for them is quite difficult if you reuse them, and allocating them
in kernelspace usually means you want to keep reusing them.
What other options are there? The only user in the stack is the userspace interface, which lets you mmap the pages in the iso_buffer from an application. The pages need to stay around and stay mapped as long as that buffer is mmapped by userspace. The buffer can be several megabytes and I don't want to set up a kernel side virtual mapping for it. The pages are only used for either outgoing or incoming data, never both, so device/driver ownership isn't too difficult to handle. 


+#include <linux/kthread.h>


You don't actually seem to use this one ..


+#include <asm/uaccess.h>


.. or this one ..


+#include <asm/semaphore.h>


.. or this one.


Ah, right, I'll get rid of those.


+	retval = fw_core_add_address_handler(&topology_map,
+					     &topology_map_region);
+	BUG_ON(retval < 0);
+
+	retval = fw_core_add_address_handler(&registers,
+					     &registers_region);
+	BUG_ON(retval < 0);
+
+	/* Add the vendor textual descriptor. */
+	retval = fw_core_add_descriptor(&vendor_id_descriptor);
+	BUG_ON(retval < 0);
+	retval = fw_core_add_descriptor(&model_id_descriptor);
+	BUG_ON(retval < 0);


These kinds of bug checks look wrong.  Either the operations
can't fail in which case they should not return an error value
or you should handle them properly.
The fw_core_add_descriptor() checks that the descriptor block it's passed is internally consistent and is used for blocks passed in from userspace too. In these two cases, the blocks are static const arrays in the driver and if fw_core_add_descriptor returns < 0 it's a bug in the driver. 


Both the previous and this patch contain quite a lot of GFP_ATOMIC
allocation which are a sign of not having a very good layering.
Looking through the GFP_ATOMIC allocations, I see a couple that could be rolled back to GFP_KERNEL. But I don't know that it means bad layering, I'm just typically using a GFP_ATOMIC kmalloc than, preallocating some fixed number of, say, packets or nodes or whatever. For example, the old SBP-2 (storage) driver uses a free-list of packets and grabs one from that list when the SCSI stacks asks it to send a request. If that list is empty, it fails and lets the SCSI stack retry the command. I'm using a GFP_ATOMIC kmalloc instead in that case, and I believe it's a better approach than implementing ad-hoc allocation data structures. 

Thanks for the reviews, I'll look through your other emails.
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux