Messages in this thread | | | Date | Wed, 2 May 2007 20:29:04 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH 2/6] firewire: isochronous and asynchronous I/O |
| |
> + 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.
> +#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.
> + retval = fw_core_add_address_handler(&topology_map, > + &topology_map_region); > + BUG_ON(retval < 0); > + > + retval = fw_core_add_address_handler(®isters, > + ®isters_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.
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |