| Message ID | 1384803581-23871-1-git-send-email-swarren@wwwdotorg.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, 2013-11-18 at 12:39 -0700, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> Thanks for an updated version. Still couple of comments below. After addressing them Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com> for the acpi-dma.c part. > dma_request_slave_channel() simply returns NULL whenever DMA channel > lookup fails. Lookup could fail for two distinct reasons: > > a) No DMA specification exists for the channel name. > This includes situations where no DMA specifications exist at all, or > other general lookup problems. > > b) A DMA specification does exist, yet the driver for that channel is not > yet registered. > > Case (b) should trigger deferred probe in client drivers. However, since > they have no way to differentiate the two situations, it cannot. > > Implement new function dma_request_slave_channel_or_err(), which performs > identically to dma_request_slave_channel(), except that it returns an > error-pointer rather than NULL, which allows callers to detect when > deferred probe should occur. > > Eventually, all drivers should be converted to this new API, the old API > removed, and the new API renamed to the more desirable name. This patch > doesn't convert the existing API and all drivers in one go, since some > drivers call dma_request_slave_channel() then dma_request_channel() if > that fails. That would require modifying dma_request_channel() in the > same way, which would then require modifying close to 100 drivers at once, > rather than just the 15-20 or so that use dma_request_slave_channel(), > which might be tenable in a single patch. > > acpi_dma_request_slave_chan_by_index() doesn't actually implement > deferred probe. Perhaps it should? Yes, it should. You may mentioned that it will be updated later accordingly. > > Cc: treding@nvidia.com > Cc: pdeschrijver@nvidia.com > Cc: linux-tegra@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: Dan Williams <dan.j.williams@intel.com> > To: Vinod Koul <vinod.koul@intel.com> > To: Andriy Shevchenko <andriy.shevchenko@intel.com> > Cc: dmaengine@vger.kernel.org > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > v2: > * include <linux/err.h> in <linux/dmaengine.h> > * Return -ENODATA if slave_id or chan_id out-of-range. > * Return an error-pointer not NULL if we can't find a matching DMA > controller or translate the channel. > > This patch is part of a series with strong internal depdendencies. I'm > looking for an ack so that I can take the entire series through the Tegra > and arm-soc trees. The series will be part of a stable branch that can be > merged into other subsystems if needed to avoid/resolve dependencies. > Note that I'm just reposting this one patch from the series for V2. > > Alternatively, if this patch can be applied directly on top of 3.13-rc1 > (once it appears) alone in a stable topic branch, I can merge that into > my Tegra tree and use it as a base. > --- > drivers/dma/acpi-dma.c | 14 +++++++------- > drivers/dma/dmaengine.c | 44 ++++++++++++++++++++++++++++++++++++++++---- > drivers/dma/of-dma.c | 12 +++++++----- > include/linux/dmaengine.h | 8 ++++++++ > include/linux/of_dma.h | 9 ++++++--- > 5 files changed, 68 insertions(+), 19 deletions(-) > > diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c > index e69b03c0fa50..e3d9568f31a3 100644 > --- a/drivers/dma/acpi-dma.c > +++ b/drivers/dma/acpi-dma.c > @@ -334,7 +334,7 @@ static int acpi_dma_parse_fixed_dma(struct acpi_resource *res, void *data) > * @dev: struct device to get DMA request from > * @index: index of FixedDMA descriptor for @dev > * > - * Returns pointer to appropriate dma channel on success or NULL on error. > + * Returns pointer to appropriate dma channel on success or an error pointer. > */ > struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev, > size_t index) > @@ -349,10 +349,10 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev, > > /* Check if the device was enumerated by ACPI */ > if (!dev || !ACPI_HANDLE(dev)) > - return NULL; > + return ERR_PTR(-ENODEV); > > if (acpi_bus_get_device(ACPI_HANDLE(dev), &adev)) > - return NULL; > + return ERR_PTR(-ENODEV); > > memset(&pdata, 0, sizeof(pdata)); > pdata.index = index; > @@ -367,7 +367,7 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev, > acpi_dev_free_resource_list(&resource_list); > > if (dma_spec->slave_id < 0 || dma_spec->chan_id < 0) > - return NULL; > + return ERR_PTR(-ENODATA); > > mutex_lock(&acpi_dma_lock); > > @@ -390,7 +390,7 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev, > } > > mutex_unlock(&acpi_dma_lock); > - return chan; > + return chan ? chan : ERR_PTR(-ENOENT); > } > EXPORT_SYMBOL_GPL(acpi_dma_request_slave_chan_by_index); > > @@ -403,7 +403,7 @@ EXPORT_SYMBOL_GPL(acpi_dma_request_slave_chan_by_index); > * translate the names "tx" and "rx" here based on the most common case where > * the first FixedDMA descriptor is TX and second is RX. > * > - * Returns pointer to appropriate dma channel on success or NULL on error. > + * Returns pointer to appropriate dma channel on success or an error pointer. > */ > struct dma_chan *acpi_dma_request_slave_chan_by_name(struct device *dev, > const char *name) > @@ -415,7 +415,7 @@ struct dma_chan *acpi_dma_request_slave_chan_by_name(struct device *dev, > else if (!strcmp(name, "rx")) > index = 1; > else > - return NULL; > + return ERR_PTR(-ENODEV); I still think -EINVAL suits a bit better here. > > return acpi_dma_request_slave_chan_by_index(dev, index); > } > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index ea806bdc12ef..5e7f8af2f0ec 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -540,6 +540,8 @@ EXPORT_SYMBOL_GPL(dma_get_slave_channel); > * @mask: capabilities that the channel must satisfy > * @fn: optional callback to disposition available channels > * @fn_param: opaque parameter to pass to dma_filter_fn > + * > + * Returns pointer to appropriate dma channel on success or NULL. > */ > struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask, > dma_filter_fn fn, void *fn_param) > @@ -588,24 +590,58 @@ struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask, > EXPORT_SYMBOL_GPL(__dma_request_channel); > > /** > - * dma_request_slave_channel - try to allocate an exclusive slave channel > + * __dma_request_slave_channel - try to allocate an exclusive slave > + * channel > * @dev: pointer to client device structure > * @name: slave channel name > + * > + * Returns pointer to appropriate dma channel on success or an error pointer. > */ > -struct dma_chan *dma_request_slave_channel(struct device *dev, const char *name) > +static struct dma_chan *__dma_request_slave_channel(struct device *dev, > + const char *name, bool defer) > { > /* If device-tree is present get slave info from here */ > if (dev->of_node) > - return of_dma_request_slave_channel(dev->of_node, name); > + return of_dma_request_slave_channel(dev->of_node, name, defer); > > /* If device was enumerated by ACPI get slave info from here */ > if (ACPI_HANDLE(dev)) > return acpi_dma_request_slave_chan_by_name(dev, name); > > - return NULL; > + return ERR_PTR(-ENODEV); > +} > + > +/** > + * dma_request_slave_channel - try to allocate an exclusive slave channel > + * @dev: pointer to client device structure > + * @name: slave channel name > + * > + * Returns pointer to appropriate dma channel on success or NULL. > + */ > +struct dma_chan *dma_request_slave_channel(struct device *dev, > + const char *name) > +{ > + struct dma_chan *ch = __dma_request_slave_channel(dev, name, false); > + if (IS_ERR(ch)) > + return NULL; > + return ch; > } > EXPORT_SYMBOL_GPL(dma_request_slave_channel); > > +/** > + * dma_request_slave_channel_or_err - try to allocate an exclusive slave channel > + * @dev: pointer to client device structure > + * @name: slave channel name > + * > + * Returns pointer to appropriate dma channel on success or an error pointer. > + */ > +struct dma_chan *dma_request_slave_channel_or_err(struct device *dev, > + const char *name) > +{ > + return __dma_request_slave_channel(dev, name, true); > +} > +EXPORT_SYMBOL_GPL(dma_request_slave_channel_or_err); > + > void dma_release_channel(struct dma_chan *chan) > { > mutex_lock(&dma_list_mutex); > diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c > index 0b88dd3d05f4..928141f6f21b 100644 > --- a/drivers/dma/of-dma.c > +++ b/drivers/dma/of-dma.c > @@ -143,10 +143,10 @@ static int of_dma_match_channel(struct device_node *np, const char *name, > * @np: device node to get DMA request from > * @name: name of desired channel > * > - * Returns pointer to appropriate dma channel on success or NULL on error. > + * Returns pointer to appropriate dma channel on success or an error pointer. > */ > struct dma_chan *of_dma_request_slave_channel(struct device_node *np, > - const char *name) > + const char *name, bool defer) > { > struct of_phandle_args dma_spec; > struct of_dma *ofdma; > @@ -155,14 +155,14 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, > > if (!np || !name) { > pr_err("%s: not enough information provided\n", __func__); > - return NULL; > + return ERR_PTR(-ENODEV); > } > > count = of_property_count_strings(np, "dma-names"); > if (count < 0) { > pr_err("%s: dma-names property of node '%s' missing or empty\n", > __func__, np->full_name); > - return NULL; > + return ERR_PTR(-ENODEV); > } > > for (i = 0; i < count; i++) { > @@ -181,11 +181,13 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, > > of_node_put(dma_spec.np); > > + if (!ofdma && defer) > + return ERR_PTR(-EPROBE_DEFER); > if (chan) > return chan; > } > > - return NULL; > + return ERR_PTR(-ENODEV); > } > > /** > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 41cf0c399288..f156c145fad2 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -22,6 +22,7 @@ > #define LINUX_DMAENGINE_H > > #include <linux/device.h> > +#include <linux/err.h> > #include <linux/uio.h> > #include <linux/bug.h> > #include <linux/scatterlist.h> > @@ -1041,6 +1042,8 @@ void dma_issue_pending_all(void); > struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask, > dma_filter_fn fn, void *fn_param); > struct dma_chan *dma_request_slave_channel(struct device *dev, const char *name); > +struct dma_chan *dma_request_slave_channel_or_err(struct device *dev, > + const char *name); > void dma_release_channel(struct dma_chan *chan); > #else > static inline struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type) > @@ -1068,6 +1071,11 @@ static inline struct dma_chan *dma_request_slave_channel(struct device *dev, > { > return NULL; > } > +static inline struct dma_chan *dma_request_slave_channel_or_err( > + struct device *dev, const char *name) > +{ > + return ERR_PTR(-ENODEV); > +} > static inline void dma_release_channel(struct dma_chan *chan) > { > } > diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h > index ae36298ba076..0504461574c6 100644 > --- a/include/linux/of_dma.h > +++ b/include/linux/of_dma.h > @@ -38,7 +38,8 @@ extern int of_dma_controller_register(struct device_node *np, > void *data); > extern void of_dma_controller_free(struct device_node *np); > extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np, > - const char *name); > + const char *name, > + bool defer); > extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec, > struct of_dma *ofdma); > #else > @@ -54,8 +55,10 @@ static inline void of_dma_controller_free(struct device_node *np) > { > } > > -static inline struct dma_chan *of_dma_request_slave_channel(struct device_node *np, > - const char *name) > +static inline struct dma_chan *of_dma_request_slave_channel( > + struct device_node *np, > + const char *name, > + bool defer) > { > return NULL; > } -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On 11/19/2013 04:49 AM, Shevchenko, Andriy wrote: > On Mon, 2013-11-18 at 12:39 -0700, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> > > Thanks for an updated version. > Still couple of comments below. > > After addressing them > > Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com> > > for the acpi-dma.c part. Thanks. Sorry I missed the s/ENODEV/EINVAL/ in acpi_dma_request_slave_chan_by_name(). I know having review comments overlooked or ignored can be annoying. It's all fixed now:-) -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c index e69b03c0fa50..e3d9568f31a3 100644 --- a/drivers/dma/acpi-dma.c +++ b/drivers/dma/acpi-dma.c @@ -334,7 +334,7 @@ static int acpi_dma_parse_fixed_dma(struct acpi_resource *res, void *data) * @dev: struct device to get DMA request from * @index: index of FixedDMA descriptor for @dev * - * Returns pointer to appropriate dma channel on success or NULL on error. + * Returns pointer to appropriate dma channel on success or an error pointer. */ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev, size_t index) @@ -349,10 +349,10 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev, /* Check if the device was enumerated by ACPI */ if (!dev || !ACPI_HANDLE(dev)) - return NULL; + return ERR_PTR(-ENODEV); if (acpi_bus_get_device(ACPI_HANDLE(dev), &adev)) - return NULL; + return ERR_PTR(-ENODEV); memset(&pdata, 0, sizeof(pdata)); pdata.index = index; @@ -367,7 +367,7 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev, acpi_dev_free_resource_list(&resource_list); if (dma_spec->slave_id < 0 || dma_spec->chan_id < 0) - return NULL; + return ERR_PTR(-ENODATA); mutex_lock(&acpi_dma_lock); @@ -390,7 +390,7 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev, } mutex_unlock(&acpi_dma_lock); - return chan; + return chan ? chan : ERR_PTR(-ENOENT); } EXPORT_SYMBOL_GPL(acpi_dma_request_slave_chan_by_index); @@ -403,7 +403,7 @@ EXPORT_SYMBOL_GPL(acpi_dma_request_slave_chan_by_index); * translate the names "tx" and "rx" here based on the most common case where * the first FixedDMA descriptor is TX and second is RX. * - * Returns pointer to appropriate dma channel on success or NULL on error. + * Returns pointer to appropriate dma channel on success or an error pointer. */ struct dma_chan *acpi_dma_request_slave_chan_by_name(struct device *dev, const char *name) @@ -415,7 +415,7 @@ struct dma_chan *acpi_dma_request_slave_chan_by_name(struct device *dev, else if (!strcmp(name, "rx")) index = 1; else - return NULL; + return ERR_PTR(-ENODEV); return acpi_dma_request_slave_chan_by_index(dev, index); } diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index ea806bdc12ef..5e7f8af2f0ec 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -540,6 +540,8 @@ EXPORT_SYMBOL_GPL(dma_get_slave_channel); * @mask: capabilities that the channel must satisfy * @fn: optional callback to disposition available channels * @fn_param: opaque parameter to pass to dma_filter_fn + * + * Returns pointer to appropriate dma channel on success or NULL. */ struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param) @@ -588,24 +590,58 @@ struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask, EXPORT_SYMBOL_GPL(__dma_request_channel); /** - * dma_request_slave_channel - try to allocate an exclusive slave channel + * __dma_request_slave_channel - try to allocate an exclusive slave + * channel * @dev: pointer to client device structure * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or an error pointer. */ -struct dma_chan *dma_request_slave_channel(struct device *dev, const char *name) +static struct dma_chan *__dma_request_slave_channel(struct device *dev, + const char *name, bool defer) { /* If device-tree is present get slave info from here */ if (dev->of_node) - return of_dma_request_slave_channel(dev->of_node, name); + return of_dma_request_slave_channel(dev->of_node, name, defer); /* If device was enumerated by ACPI get slave info from here */ if (ACPI_HANDLE(dev)) return acpi_dma_request_slave_chan_by_name(dev, name); - return NULL; + return ERR_PTR(-ENODEV); +} + +/** + * dma_request_slave_channel - try to allocate an exclusive slave channel + * @dev: pointer to client device structure + * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or NULL. + */ +struct dma_chan *dma_request_slave_channel(struct device *dev, + const char *name) +{ + struct dma_chan *ch = __dma_request_slave_channel(dev, name, false); + if (IS_ERR(ch)) + return NULL; + return ch; } EXPORT_SYMBOL_GPL(dma_request_slave_channel); +/** + * dma_request_slave_channel_or_err - try to allocate an exclusive slave channel + * @dev: pointer to client device structure + * @name: slave channel name + * + * Returns pointer to appropriate dma channel on success or an error pointer. + */ +struct dma_chan *dma_request_slave_channel_or_err(struct device *dev, + const char *name) +{ + return __dma_request_slave_channel(dev, name, true); +} +EXPORT_SYMBOL_GPL(dma_request_slave_channel_or_err); + void dma_release_channel(struct dma_chan *chan) { mutex_lock(&dma_list_mutex); diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 0b88dd3d05f4..928141f6f21b 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -143,10 +143,10 @@ static int of_dma_match_channel(struct device_node *np, const char *name, * @np: device node to get DMA request from * @name: name of desired channel * - * Returns pointer to appropriate dma channel on success or NULL on error. + * Returns pointer to appropriate dma channel on success or an error pointer. */ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, - const char *name) + const char *name, bool defer) { struct of_phandle_args dma_spec; struct of_dma *ofdma; @@ -155,14 +155,14 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, if (!np || !name) { pr_err("%s: not enough information provided\n", __func__); - return NULL; + return ERR_PTR(-ENODEV); } count = of_property_count_strings(np, "dma-names"); if (count < 0) { pr_err("%s: dma-names property of node '%s' missing or empty\n", __func__, np->full_name); - return NULL; + return ERR_PTR(-ENODEV); } for (i = 0; i < count; i++) { @@ -181,11 +181,13 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, of_node_put(dma_spec.np); + if (!ofdma && defer) + return ERR_PTR(-EPROBE_DEFER); if (chan) return chan; } - return NULL; + return ERR_PTR(-ENODEV); } /** diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 41cf0c399288..f156c145fad2 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -22,6 +22,7 @@ #define LINUX_DMAENGINE_H #include <linux/device.h> +#include <linux/err.h> #include <linux/uio.h> #include <linux/bug.h> #include <linux/scatterlist.h> @@ -1041,6 +1042,8 @@ void dma_issue_pending_all(void); struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param); struct dma_chan *dma_request_slave_channel(struct device *dev, const char *name); +struct dma_chan *dma_request_slave_channel_or_err(struct device *dev, + const char *name); void dma_release_channel(struct dma_chan *chan); #else static inline struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type) @@ -1068,6 +1071,11 @@ static inline struct dma_chan *dma_request_slave_channel(struct device *dev, { return NULL; } +static inline struct dma_chan *dma_request_slave_channel_or_err( + struct device *dev, const char *name) +{ + return ERR_PTR(-ENODEV); +} static inline void dma_release_channel(struct dma_chan *chan) { } diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h index ae36298ba076..0504461574c6 100644 --- a/include/linux/of_dma.h +++ b/include/linux/of_dma.h @@ -38,7 +38,8 @@ extern int of_dma_controller_register(struct device_node *np, void *data); extern void of_dma_controller_free(struct device_node *np); extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np, - const char *name); + const char *name, + bool defer); extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec, struct of_dma *ofdma); #else @@ -54,8 +55,10 @@ static inline void of_dma_controller_free(struct device_node *np) { } -static inline struct dma_chan *of_dma_request_slave_channel(struct device_node *np, - const char *name) +static inline struct dma_chan *of_dma_request_slave_channel( + struct device_node *np, + const char *name, + bool defer) { return NULL; }