Re: A proposal to check the device in generic way

[Arnd Bergmann]

On Monday 11 March 2013, Shevchenko, Andriy wrote:
> Hello.
> 
> Currently in linux-next we have the following things:
> 
> $ git grep -n 'chan->device->dev->driver' drivers/dma/
> 
> drivers/dma/amba-pl08x.c:1594:  if (chan->device->dev->driver !=
> &pl08x_amba_driver.drv)
> drivers/dma/dmaengine.c:190:    return chan->device->dev->driver->owner;
> drivers/dma/edma.c:609: if (chan->device->dev->driver ==
> &edma_driver.driver) {
> drivers/dma/omap-dma.c:654:     if (chan->device->dev->driver ==
> &omap_dma_driver.driver) {
> drivers/dma/pl330.c:2374:       if (chan->device->dev->driver !=
> &pl330_driver.drv)
> drivers/dma/sa11x0-dma.c:1080:  if (chan->device->dev->driver ==
> &sa11x0_dma_driver.driver) 
> 
> I think it's a non-generic way to check which driver provides a channel
> into filter function. First of all, I don't get why that comparison goes
> as deep as driver structure. Isn't clearer to check chan->device->dev
> against the struct dev passed in the custom parameter structure? Like:

I agree that checking the driver is wrong and that we should check the
device instead, if we have a filter function called from ->xlate(), which
in turn gets called by dma_request_slave_channel().

Some of the filter functions above are passed to device drivers and
are not used in combination with the DT binding, which implies that
there is nothing to check the device against, so it would not work.

If you look for instance at the pl330 driver, there
are two filter functions: pl330_dt_filter correctly compares the
device pointer, while pl330_filter only gets an integer argument
and cannot compare that to the device.

> struct filter_params {
>  struct dev *dev;
>  void *param;
> };
> 
> bool filter_fn(struct dma_chan *chan, void *fparams)
> {
>   struct filter_params *p = fparams;
>   if (chan->device->dev != p->dev)
>    return false;
>  ...
> }
> 
> In case my idea has a right to live, what about to move such check inside
> DMA engine code?

> Opinions, comments?
> 
> Earlier I tried to discuss this with Arnd here: http://www.spinics.net/lists/arm-kernel/msg220716.html

I think the right solution is to provide an interface in the dmaengine
common code that lets the ->xlate() callback get right at the channel
it wants, rather than call dma_request_channel(). It is rather silly d
a dmaengine driver that has all the information about the dma channel
it wants to return, we have to call back into a common function that
then searches all the channel known to the system.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/