Messages in this thread | | | Date | Fri, 14 Nov 2008 22:08:42 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH 03/13] dmaengine: up-level reference counting to the module level |
| |
On Fri, 14 Nov 2008 14:34:32 -0700 Dan Williams <dan.j.williams@intel.com> wrote:
> Simply, if a client wants any dmaengine channel then prevent all dmaengine > modules from being removed. Once the clients are done re-enable module > removal. > > Why?, beyond reducing complication: > 1/ Tracking reference counts per-transaction in an efficient manner, as > is currently done, requires a complicated scheme to avoid cache-line > bouncing effects. > 2/ Per-transaction ref-counting gives the false impression that a > dma-driver can be gracefully removed ahead of its user (net, md, or > dma-slave) > 3/ None of the in-tree dma-drivers talk to hot pluggable hardware, but > if such an engine were built one day we still would not need to notify > clients of remove events. The driver can simply return NULL to a > ->prep() request, something that is much easier for a client to handle. > > ... > > +static struct module *dma_chan_to_owner(struct dma_chan *chan) > +{ > + return chan->device->dev->driver->owner; > +}
Has this all been tested with CONFIG_MODULES=n?
It looks like we have a lot of unneeded code if CONFIG_MODULES=n. However that might not be a case which is worth bothering about.
> +/** > + * balance_ref_count - catch up the channel reference count > + */ > +static void balance_ref_count(struct dma_chan *chan)
Forgot to kerneldocument the argument.
> +{ > + struct module *owner = dma_chan_to_owner(chan); > + > + while (chan->client_count < dmaengine_ref_count) { > + __module_get(owner); > + chan->client_count++; > + } > +}
The locking for ->client_count is undocumented.
> +/** > + * dma_chan_get - try to grab a dma channel's parent driver module > + * @chan - channel to grab > + */ > +static int dma_chan_get(struct dma_chan *chan) > +{ > + int err = -ENODEV; > + struct module *owner = dma_chan_to_owner(chan); > + > + if (chan->client_count) { > + __module_get(owner); > + err = 0; > + } else if (try_module_get(owner)) > + err = 0;
I wonder if try_module_get() could be used in both cases (migt not make sense to do so though).
> + if (err == 0) > + chan->client_count++;
Locking for this?
> + /* allocate upon first client reference */ > + if (chan->client_count == 1 && err == 0) { > + int desc = chan->device->device_alloc_chan_resources(chan, NULL); > + > + if (desc < 0) { > + chan->client_count = 0; > + module_put(owner); > + err = -ENOMEM;
Shouldn't we just propagate the ->device_alloc_chan_resources() return value?
> + } else > + balance_ref_count(chan); > + } > + > + return err; > +} > + > +static void dma_chan_put(struct dma_chan *chan) > +{ > + if (!chan->client_count) > + return; /* this channel failed alloc_chan_resources */
Or we had a bug ;)
> + chan->client_count--;
Undocumented locking..
> > ... >
| |