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

Kristian Høgsberg
2007-05-03T00:10:15Z

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