lkml.org 
[lkml]   [2007]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/6] firewire: isochronous and asynchronous I/O
    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 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/

    \
     
     \ /
      Last update: 2007-05-03 02:13    [W:0.024 / U:2.608 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site