Re: [PATCH] dma: Remove comment about embedding dma_slave_config into custom structs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Feb 16, 2014 at 02:21:22PM +0100, Lars-Peter Clausen wrote:
> The documentation for the dma_slave_config struct recommends that if a DMA
> controller has special configuration options, which can not be configured
> through the dma_slave_config struct, the driver should create its own custom
> config struct and embed the dma_slave_config struct in it and pass the custom
> config struct to dmaengine_slave_config(). This overloads the generic
> dmaengine_slave_config() API with custom semantics and any caller of the
> dmaengine_slave_config() that is not aware of these special semantics will cause
> undefined behavior. This means that it is impossible for generic code to make
> use of dmaengine_slave_config(). Such a restriction contradicts the very idea of
> having a generic API.
> 
> E.g. consider the following case of a DMA controller that has an option to
> reverse the field polarity of the DMA transfer with the following implementation
> for setting the configuration:
> 
> 	struct my_slave_config {
> 		struct dma_slave_config config;
> 		unsigned int field_polarity;
> 	};
> 
> 	static int my_dma_controller_slave_config(struct dma_chan *chan,
> 		struct dma_slave_config *config)
> 	{
> 		struct my_slave_config *my_cfg = container_of(config,
> 				struct my_slave_config, config);
> 
> 		...
> 		my_dma_set_field_polarity(chan, my_cfg->field_polarity);
> 		...
> 	}
> 
> Now a generic user of the dmaengine API might want to configure a DMA channel
> for this DMA controller that it obtained using the following code:
> 
> 	struct dma_slave_config config;
> 
> 	config.src_addr = ...;
> 	...
> 	dmaengine_slave_config(chan, &config);
> 
> The call to dmaengine_slave_config() will eventually call into
> my_dma_controller_slave_config() which will cast from dma_slave_config to
> my_slave_config and then tries to access the field_polarity member. Since the
> dma_slave_config struct that was passed in was never embedded into a
> my_slave_config struct this attempt will just read random stack garbage and use
> that to configure the DMA controller. This is bad. Instead, if a DMA controller
> really needs to have custom configuration options, the driver should create a
> custom API for it. This makes it very clear that there is a direct dependency
> of a user of such an API and the implementer. E.g.:
> 
> 	int my_dma_set_field_polarity(struct dma_chan *chan,
> 		unsigned int field_polarity) {
> 		if (chan->device->dev->driver != &my_dma_controller_driver.driver)
> 			return -EINVAL;
> 		...
> 	}
> 	EXPORT_SYMBOL_GPL(my_dma_set_field_polarity);
> 
> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>

Applied, thanks

-- 
~Vinod

> ---
>  include/linux/dmaengine.h | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index c5c92d5..8300fb8 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -341,15 +341,11 @@ enum dma_slave_buswidth {
>   * and this struct will then be passed in as an argument to the
>   * DMA engine device_control() function.
>   *
> - * The rationale for adding configuration information to this struct
> - * is as follows: if it is likely that most DMA slave controllers in
> - * the world will support the configuration option, then make it
> - * generic. If not: if it is fixed so that it be sent in static from
> - * the platform data, then prefer to do that. Else, if it is neither
> - * fixed at runtime, nor generic enough (such as bus mastership on
> - * some CPU family and whatnot) then create a custom slave config
> - * struct and pass that, then make this config a member of that
> - * struct, if applicable.
> + * The rationale for adding configuration information to this struct is as
> + * follows: if it is likely that more than one DMA slave controllers in
> + * the world will support the configuration option, then make it generic.
> + * If not: if it is fixed so that it be sent in static from the platform
> + * data, then prefer to do that.
>   */
>  struct dma_slave_config {
>  	enum dma_transfer_direction direction;
> -- 
> 1.8.0
> 

-- 
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux