A proposal to check the device in generic way

A proposal to check the device in generic way

[Shevchenko, Andriy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2028 bytes --]

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:

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

-- 
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.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: A proposal to check the device in generic way

[Guennadi Liakhovetski]

Hi Andriy

(adding Russell to CC)

On Mon, 11 Mar 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:
> 
> struct filter_params {
>  struct dev *dev;
>  void *param;
> };

I don't think you always know which DMA device you want to use with this 
DMA client - sometimes there are several DMA engine devices, that can be 
used with your DMA client, or even if it's only one, you don't necessarily 
have a pointer to it in your DMA client.

Thanks
Guennadi

> 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

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: A proposal to check the device in generic way

[Shevchenko, Andriy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2411 bytes --]

On Mon, 2013-03-11 at 13:26 +0100, Guennadi Liakhovetski wrote: 
> > 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:
> > 
> > struct filter_params {
> >  struct dev *dev;
> >  void *param;
> > };
> 
> I don't think you always know which DMA device you want to use with this 
> DMA client - sometimes there are several DMA engine devices, that can be 
> used with your DMA client, or even if it's only one, you don't necessarily 
> have a pointer to it in your DMA client.

Ah, right. But let's assume we will use filter_fn in conjunction with
xlate_fn. In that case we will have explicit understanding for which
device we registered specific xlate_fn. So, xlate_fn could provide
struct device pointer to be compared with, though I prefer to have more
suitable mechanism instead of filter_fn.

-- 
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.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: A proposal to check the device in generic way

[Russell King - ARM Linux]

On Mon, Mar 11, 2013 at 01:26:01PM +0100, Guennadi Liakhovetski wrote:
> Hi Andriy
> 
> (adding Russell to CC)
> 
> On Mon, 11 Mar 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:
> > 
> > struct filter_params {
> >  struct dev *dev;
> >  void *param;
> > };
> 
> I don't think you always know which DMA device you want to use with this 
> DMA client - sometimes there are several DMA engine devices, that can be 
> used with your DMA client, or even if it's only one, you don't necessarily 
> have a pointer to it in your DMA client.

Correct - take a look at what gets passed - for example, sometimes the
filter parameter is just a string.  Sometimes, even in the static
platform code, we may not have an address for the DMA device structure.

So, how would you supply the struct device for the DMA?  You can't.

But, think about it a moment.  What we're actually really interested in
is: do we know the structure of the driver's channel specific data?
What determines that?  The device?  Or the driver?  It's the driver.

So for safe and correct type checking, we need to make sure that the
dma_chan is really associated with _this_ driver before we dereference
anything in the driver-private data structures surrounding dma_chan.

Hence, the struct device_driver is the right thing to use.  I would
however agree that it would be nice if the device model provided us a
way to do that without having to delve that deeply ourselves.

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