lkml.org 
[lkml]   [2005]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC] [PATCH 1/3] ioat: DMA subsystem


Mostly ok, but some minor nits.



Andrew Grover wrote:
> index 0000000..f2cc2d7
> --- /dev/null
> +++ b/drivers/dma/cb_list.h
> @@ -0,0 +1,12 @@
> +/* Extra macros that build on <linux/list.h> */
> +#ifndef CB_LIST_H
> +#define CB_LIST_H
> +
> +#include <linux/list.h>
> +
> +/* Provide some safty to list_add, which I find easy to swap the arguments to */
> +
> +#define list_add_entry(pos, head, member) list_add(&pos->member, head)
> +#define list_add_entry_tail(pos, head, member) list_add_tail(&pos->member, head)
> +
> +#endif /* CB_LIST_H */

Maybe this just adds fuel to the fire, given your code comment, but I
tend to think most people are used to

object_foo(object, other args...)

where the object in question is "the list". That would imply a

list_foo(head, others args...)

pattern.

As a side note, I have the same problem as you, WRT swapping the
list_add arguments. I've always thought that was the one big drawback
to Linus's otherwise elegant list implementation.

general nits:

1) docbook-able function headers, with useful documentation, would be
nice. Using libata as an example, even if I don't provide any useful
function description, I at least document the locking details/context
for each function.

2) more inline code commenting would be nice.



> + if (chan->device->device_alloc_chan_resources(chan) >= 0) {
> + chan->client = client;
> + list_add_entry_tail(chan, &client->channels, client_node);
> + return chan;
> + }


device_alloc_chan_resources is a very long name. :)


> +static void
> +dma_client_chan_free(struct dma_chan *chan)
> +{
> + BUG_ON(!chan);
> +
> + chan->device->device_free_chan_resources(chan);
> + chan->client = NULL;
> +}

ditto


> +static void
> +dma_chans_rebalance(void)

explanation of this function would be nice. remember to answer "how?"
and "why?", not "what?".

> +{
> + struct dma_client *client;
> + struct dma_chan *chan;
> +
> + list_for_each_entry(client, &dma_client_list, global_node) {

locking of dma_client_list?

> + while (client->chans_desired > client->chan_count) {
> + chan = dma_client_chan_alloc(client);
> + if (!chan)
> + break;
> +
> + client->chan_count++;
> + client->event_callback(client, chan, DMA_RESOURCE_ADDED);
> + }
> +
> + while (client->chans_desired < client->chan_count) {
> + chan = list_entry(client->channels.next, struct dma_chan, client_node);
> + list_del(&chan->client_node);
> + client->chan_count--;
> + client->event_callback(client, chan, DMA_RESOURCE_REMOVED);
> + dma_client_chan_free(chan);

In general, this DMA_RESOURCE_REMOVED operation feels like a "yanking
the carpet out from under my feet" operation, something we should avoid
for object-lifetime reasons.

However in this case, AFAICS dmaengine.c completely controls object
lifetime, so I do not see a real problem. I'm just nervous. :)


> + }
> + }
> +}
> +
> +struct dma_client *
> +dma_async_client_register(dma_event_callback event_callback)
> +{
> + struct dma_client *client;
> +
> + BUG_ON(!event_callback);
> +
> + client = kmalloc(sizeof(*client), GFP_KERNEL);
> + if (!client)
> + return NULL;
> +
> + INIT_LIST_HEAD(&client->channels);
> +
> + client->chans_desired = 0;
> + client->chan_count = 0;
> + client->event_callback = event_callback;
> +
> + list_add_entry_tail(client, &dma_client_list, global_node);
> +
> + return client;

Possible SMP bug here?

So far, in my code read, I was presuming that the caller was doing some
sort of locking on dma_client_list and dma_device_list. (Hint: need
locking docs for each function)

But if you are using GFP_KERNEL, it certainly appears that two callers
could race with each other when touching dma_client_list.



> +dma_cookie_t
> +dma_async_memcpy_buf_to_buf(
> + struct dma_chan *chan,
> + void *dest,
> + void *src,
> + size_t len)
> +{
> + chan->bytes_transferred += len;
> + chan->memcpy_count++;
> +
> + return chan->device->device_memcpy_buf_to_buf(chan, dest, src, len);
> +}
> +
> +dma_cookie_t
> +dma_async_memcpy_buf_to_pg(
> + struct dma_chan *chan,
> + struct page *page,
> + unsigned int offset,
> + void *kdata,
> + size_t len)
> +{
> + chan->bytes_transferred += len;
> + chan->memcpy_count++;
> +
> + return chan->device->device_memcpy_buf_to_pg(chan, page, offset, kdata, len);
> +}
> +
> +dma_cookie_t
> +dma_async_memcpy_pg_to_pg(
> + struct dma_chan *chan,
> + struct page *dest_pg,
> + unsigned int dest_off,
> + struct page *src_pg,
> + unsigned int src_off,
> + size_t len)
> +{
> + chan->bytes_transferred += len;
> + chan->memcpy_count++;
> +
> + return chan->device->device_memcpy_pg_to_pg(chan, dest_pg, dest_off,
> + src_pg, src_off, len);
> +}
> +
> +void
> +dma_async_memcpy_issue_pending(struct dma_chan *chan)
> +{
> + return chan->device->device_memcpy_issue_pending(chan);
> +}
> +
> +enum dma_status_t
> +dma_async_memcpy_complete(struct dma_chan *chan, dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used)
> +{
> + return chan->device->device_memcpy_complete(chan, cookie, last, used);
> +}

Making these 'static inline' might be a good idea?


> +int
> +dma_async_device_register(struct dma_device *device)
> +{
> + static int id;
> + int chancnt = 0;
> + struct dma_chan* chan;
> +
> + if (!device)
> + return -ENODEV;
> +
> + list_add_entry_tail(device, &dma_device_list, global_node);
> +
> + dma_chans_rebalance();
> +
> + device->dev_id = id++;
> +
> + /* represent channels in sysfs. Probably want devs too */
> + list_for_each_entry(chan, &device->channels, device_node) {
> + chan->chan_id = chancnt++;
> + chan->class_dev.class = &dma_devclass;
> + chan->class_dev.dev = NULL;
> + snprintf(chan->class_dev.class_id, BUS_ID_SIZE, "dma%dchan%d",
> + device->dev_id, chan->chan_id);
> +
> + chan->min_copy_size = DMA_DEFAULT_MIN_COPY_SIZE;
> + class_device_register(&chan->class_dev);
> + }
> +
> + return 0;
> +}
> +
> +void
> +dma_async_device_unregister(struct dma_device* device)
> +{
> + struct dma_chan *chan;
> +
> + BUG_ON(!device);
> +
> + list_for_each_entry(chan, &device->channels, device_node) {
> + if (chan->client) {
> + list_del(&chan->client_node);
> + chan->client->chan_count--;
> + chan->client->event_callback(chan->client, chan, DMA_RESOURCE_REMOVED);
> + dma_client_chan_free(chan);
> + }
> + class_device_unregister(&chan->class_dev);
> + }
> +
> + list_del(&device->global_node);
> +
> + dma_chans_rebalance();
> +}
> +
> +static struct workqueue_struct *dma_wait_wq;
> +static LIST_HEAD(dma_poll_list);
> +
> +enum dma_status_t
> +dma_async_wait_for_completion(struct dma_chan *chan, dma_cookie_t cookie)
> +{
> + while (dma_async_memcpy_complete(chan, cookie, NULL, NULL) == DMA_IN_PROGRESS)
> + schedule();

1) Is it worth adding a loop above the 'while', which does

retries = 5
while (operation == in progress &&
retries-- > 0)
udelay(1)
2) at that point, perhaps replace schedule() with schedule_timeout(1).
WARNING: this might introduce too much latency, and be a bad idea.


> + return DMA_SUCCESS;
> +}
> +
> +#if 0
> +static void
> +dma_poll(void *data)
> +{
> + struct dma_completion *comp = data;
> +
> + comp->status = dma_memcpy_complete(comp->chan, comp->cookie);
> + while (comp->status == DMA_IN_PROGRESS) {
> + comp->chan->device->device_arm_interrupt(comp->chan);
> + wait_for_completion(&__get_cpu_var(kick_dma_poll));
> + comp->status = dma_memcpy_complete(comp->chan, comp->cookie);
> + }
> + complete(&comp->comp);
> +}
> +
> +enum dma_status_t
> +dma_wait_for_completion(struct dma_chan *chan, dma_cookie_t cookie)
> +{
> + enum dma_status_t status;
> + DECLARE_DMA_COMPLETION(comp, chan, cookie);
> + DECLARE_WORK(dma_wait_work, dma_poll, &comp);
> +
> + BUG_ON(in_interrupt());
> +
> + status = dma_memcpy_complete(chan, cookie);
> + if (status != DMA_IN_PROGRESS)
> + return status;
> +
> + queue_work(dma_wait_wq, &dma_wait_work);
> + wait_for_completion(&comp.comp);
> + return comp.status;
> +}
> +#endif

is this for future use? never to be used?


> +static int __init dma_bus_init(void)
> +{
> + int cpu;
> +
> + dma_wait_wq = create_workqueue("dmapoll");

dma_wait_wq is never used, due to #if 0


> + for_each_online_cpu(cpu) {
> + init_completion(&per_cpu(kick_dma_poll, cpu));
> + }
> + return class_register(&dma_devclass);
> +}
> +
> +subsys_initcall(dma_bus_init);
> +
> +EXPORT_SYMBOL(dma_async_client_register);
> +EXPORT_SYMBOL(dma_async_client_unregister);
> +EXPORT_SYMBOL(dma_async_client_chan_request);
> +EXPORT_SYMBOL(dma_async_memcpy_buf_to_buf);
> +EXPORT_SYMBOL(dma_async_memcpy_buf_to_pg);
> +EXPORT_SYMBOL(dma_async_memcpy_pg_to_pg);
> +EXPORT_SYMBOL(dma_async_memcpy_complete);
> +EXPORT_SYMBOL(dma_async_memcpy_issue_pending);
> +EXPORT_SYMBOL(dma_async_device_register);
> +EXPORT_SYMBOL(dma_async_device_unregister);
> +EXPORT_SYMBOL(dma_async_wait_for_completion);
> +EXPORT_PER_CPU_SYMBOL(kick_dma_poll);
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> new file mode 100644
> index 0000000..7b4f58b
> --- /dev/null
> +++ b/include/linux/dmaengine.h
> @@ -0,0 +1,268 @@
> +/*******************************************************************************
> +
> +
> + Copyright(c) 2004 - 2005 Intel Corporation. All rights reserved.
> +
> + This program is free software; you can redistribute it and/or modify it
> + under the terms of the GNU General Public License as published by the Free
> + Software Foundation; either version 2 of the License, or (at your option)
> + any later version.
> +
> + This program is distributed in the hope that it will be useful, but WITHOUT
> + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + more details.
> +
> + You should have received a copy of the GNU General Public License along with
> + this program; if not, write to the Free Software Foundation, Inc., 59
> + Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> +
> + The full GNU General Public License is included in this distribution in the
> + file called LICENSE.
> +
> +*******************************************************************************/
> +
> +
> +#ifndef DMAENGINE_H
> +#define DMAENGINE_H
> +
> +#include <linux/device.h>
> +#include <linux/uio.h>
> +#include <linux/skbuff.h>
> +
> +DECLARE_PER_CPU(struct completion, kick_dma_poll);
> +
> +#define DMA_DEFAULT_MIN_COPY_SIZE 16
> +
> +/**
> + * enum dma_event_t - resource PNP/power managment events
> + * @DMA_RESOURCE_SUSPEND: DMA device going into low power state
> + * @DMA_RESOURCE_RESUME: DMA device returning to full power
> + * @DMA_RESOURCE_ADDED: DMA device added to the system
> + * @DMA_RESOURCE_REMOVED: DMA device removed from the system
> + */
> +enum dma_event_t {
> + DMA_RESOURCE_SUSPEND,
> + DMA_RESOURCE_RESUME,
> + DMA_RESOURCE_ADDED,
> + DMA_RESOURCE_REMOVED,
> +};
> +
> +/**
> + * typedef dma_cookie_t
> + *
> + * if dma_cookie_t is >0 it's a DMA request cookie, <0 it's an error code
> + */
> +typedef s32 dma_cookie_t;

More natural to use [signed] long? i.e. a machine int. Or _must_ this
match hardware somewhere?


> +/*#define dma_submit_error(cookie) ((cookie) < 0 ? 1 : 0)*/
> +
> +/**
> + * enum dma_status_t - DMA transaction status
> + * @DMA_SUCCESS: transaction completed successfully
> + * @DMA_IN_PROGRESS: transaction not yet processed
> + * @DMA_ERROR: transaction failed
> + */
> +enum dma_status_t {
> + DMA_SUCCESS,
> + DMA_IN_PROGRESS,
> + DMA_ERROR,
> +};
> +
> +/**
> + * struct dma_chan - devices supply DMA channels, clients use them
> + * @client: ptr to the client user of this chan, will be NULL when unused
> + * @device: ptr to the dma device who supplies this channel, always !NULL
> + * @client_node: used to add this to the client chan list
> + * @device_node: used to add this to the device chan list
> + */
> +struct dma_chan
> +{
> + struct dma_client *client;
> + struct dma_device *device;
> + dma_cookie_t cookie;
> +
> + /* sysfs */
> + int chan_id;
> + struct class_device class_dev;
> +
> + /* stats */
> + unsigned long memcpy_count;
> + unsigned long bytes_transferred;
> + unsigned int min_copy_size;

very very minor nit, but it bugs me at least: the stats variables
strike me as overly long and verbose.


> + struct list_head client_node;
> + struct list_head device_node;
> +
> + cpumask_t cpumask;
> +};
> +
> +/*
> + * typedef dma_event_callback - function pointer to a DMA event callback
> + */
> +typedef void (*dma_event_callback) (struct dma_client *client, struct dma_chan *chan, enum dma_event_t event);
> +
> +/**
> + * struct dma_client - info on the entity making use of DMA services
> + * @event_callback: func ptr to call when something happens
> + * @chan_count: number of chans allocated
> + * @chans_desired: number of chans requested. Can be +- chan_count
> + * @port: upstream DMA port from the client's PCI device
> + * @channels: the list of DMA channels allocated
> + * @global_node: list_head for global dma_client_list
> + */
> +struct dma_client {
> + dma_event_callback event_callback;
> + unsigned int chan_count;
> + unsigned int chans_desired;
> +
> + /* TODO keep some stats */
> + struct list_head channels;
> + struct list_head global_node;
> +};
> +
> +/**
> + * struct dma_device - info on the entity supplying DMA services
> + * @chancnt: how many DMA channels are supported
> + * @channels: the list of struct dma_chan
> + * @global_node: list_head for global dma_device_list
> + * Other func ptrs: used to make use of this device's capabilities
> + */
> +struct dma_device {
> +
> + unsigned int chancnt;
> + struct list_head channels;
> + struct list_head global_node;
> +
> + int dev_id;
> + /*struct class_device class_dev;*/
> +
> + int (*device_alloc_chan_resources)(struct dma_chan *chan);
> + void (*device_free_chan_resources)(struct dma_chan *chan);
> + dma_cookie_t (*device_memcpy_buf_to_buf)(struct dma_chan *chan, void *dest,
> + void *src, size_t len);
> + dma_cookie_t (*device_memcpy_buf_to_pg)(struct dma_chan *chan, struct page *page,
> + unsigned int offset, void *kdata, size_t len);
> + dma_cookie_t (*device_memcpy_pg_to_pg)(struct dma_chan *chan, struct page *dest_pg,
> + unsigned int dest_off, struct page *src_pg, unsigned int src_off,
> + size_t len);
> + void (*device_arm_interrupt)(struct dma_chan *chan);
> + enum dma_status_t (*device_memcpy_complete)(struct dma_chan *chan, dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used);
> + void (*device_memcpy_issue_pending)(struct dma_chan *chan);

names feel a bit long


-
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: 2005-11-23 23:46    [from the cache]
©2003-2011 Jasper Spaans