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

[Christoph Hellwig]

> +	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(&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.



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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/