[4/7] dmaengine: sprd: Add device validation to support multiple controllers

Commit Message

(Exiting) Baolin Wang April 15, 2019, 12:14 p.m. UTC

From: Eric Long <[e-mail]>

Since we can support multiple DMA engine controllers, we should add
device validation in filter function to check if the correct controller
to be requested.

Signed-off-by: Eric Long <[e-mail]>
Signed-off-by: Baolin Wang <[e-mail]>
---
 drivers/dma/sprd-dma.c |    5 +++++
 1 file changed, 5 insertions(+)

Comments

Vinod Koul April 29, 2019, 11:57 a.m. UTC | #1

On 15-04-19, 20:14, Baolin Wang wrote:
> From: Eric Long <[e-mail]>
> 
> Since we can support multiple DMA engine controllers, we should add
> device validation in filter function to check if the correct controller
> to be requested.
> 
> Signed-off-by: Eric Long <[e-mail]>
> Signed-off-by: Baolin Wang <[e-mail]>
> ---
>  drivers/dma/sprd-dma.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 0f92e60..9f99d4b 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
>  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
>  {
>  	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> +	struct of_phandle_args *dma_spec =
> +		container_of(param, struct of_phandle_args, args[0]);
>  	u32 slave_id = *(u32 *)param;
>  
> +	if (chan->device->dev->of_node != dma_spec->np)

Are you not using of_dma_find_controller() that does this, so this would
be useless!

> +		return false;
> +
>  	schan->dev_id = slave_id;
>  	return true;
>  }
> -- 
> 1.7.9.5

(Exiting) Baolin Wang April 29, 2019, 12:20 p.m. UTC | #2

On Mon, 29 Apr 2019 at 19:57, Vinod Koul <[e-mail]> wrote:
>
> On 15-04-19, 20:14, Baolin Wang wrote:
> > From: Eric Long <[e-mail]>
> >
> > Since we can support multiple DMA engine controllers, we should add
> > device validation in filter function to check if the correct controller
> > to be requested.
> >
> > Signed-off-by: Eric Long <[e-mail]>
> > Signed-off-by: Baolin Wang <[e-mail]>
> > ---
> >  drivers/dma/sprd-dma.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > index 0f92e60..9f99d4b 100644
> > --- a/drivers/dma/sprd-dma.c
> > +++ b/drivers/dma/sprd-dma.c
> > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> >  {
> >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > +     struct of_phandle_args *dma_spec =
> > +             container_of(param, struct of_phandle_args, args[0]);
> >       u32 slave_id = *(u32 *)param;
> >
> > +     if (chan->device->dev->of_node != dma_spec->np)
>
> Are you not using of_dma_find_controller() that does this, so this would
> be useless!

Yes, we can use of_dma_find_controller(), but that will be a little
complicated than current solution. Since we need introduce one
structure to save the node to validate in the filter function like
below, which seems make things complicated. But if you still like to
use of_dma_find_controller(), I can change to use it in next version.
Thank.

struct sprd_dma_filter_param {
     struct device_node *np;
};

static struct dma_chan* sprd_dma_xlate(struct of_phandle_args
*dma_spec, struct of_dma *of_dma)
{
    param.np = dma_spec->node;

    return dma_request_channel(xxx);
}

of_dma_controller_register(np, sprd_dma_xlate, sdev);

Vinod Koul April 29, 2019, 2:05 p.m. UTC | #3

On 29-04-19, 20:20, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 19:57, Vinod Koul <[e-mail]> wrote:
> >
> > On 15-04-19, 20:14, Baolin Wang wrote:
> > > From: Eric Long <[e-mail]>
> > >
> > > Since we can support multiple DMA engine controllers, we should add
> > > device validation in filter function to check if the correct controller
> > > to be requested.
> > >
> > > Signed-off-by: Eric Long <[e-mail]>
> > > Signed-off-by: Baolin Wang <[e-mail]>
> > > ---
> > >  drivers/dma/sprd-dma.c |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > index 0f92e60..9f99d4b 100644
> > > --- a/drivers/dma/sprd-dma.c
> > > +++ b/drivers/dma/sprd-dma.c
> > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > >  {
> > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > +     struct of_phandle_args *dma_spec =
> > > +             container_of(param, struct of_phandle_args, args[0]);
> > >       u32 slave_id = *(u32 *)param;
> > >
> > > +     if (chan->device->dev->of_node != dma_spec->np)
> >
> > Are you not using of_dma_find_controller() that does this, so this would
> > be useless!
> 
> Yes, we can use of_dma_find_controller(), but that will be a little
> complicated than current solution. Since we need introduce one
> structure to save the node to validate in the filter function like
> below, which seems make things complicated. But if you still like to
> use of_dma_find_controller(), I can change to use it in next version.

Sorry I should have clarified more..

of_dma_find_controller() is called by xlate, so you already run this
check, so why use this :)

(Exiting) Baolin Wang April 30, 2019, 5:30 a.m. UTC | #4

On Mon, 29 Apr 2019 at 22:05, Vinod Koul <[e-mail]> wrote:
>
> On 29-04-19, 20:20, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <[e-mail]> wrote:
> > >
> > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > From: Eric Long <[e-mail]>
> > > >
> > > > Since we can support multiple DMA engine controllers, we should add
> > > > device validation in filter function to check if the correct controller
> > > > to be requested.
> > > >
> > > > Signed-off-by: Eric Long <[e-mail]>
> > > > Signed-off-by: Baolin Wang <[e-mail]>
> > > > ---
> > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > index 0f92e60..9f99d4b 100644
> > > > --- a/drivers/dma/sprd-dma.c
> > > > +++ b/drivers/dma/sprd-dma.c
> > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > >  {
> > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > +     struct of_phandle_args *dma_spec =
> > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > >       u32 slave_id = *(u32 *)param;
> > > >
> > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > >
> > > Are you not using of_dma_find_controller() that does this, so this would
> > > be useless!
> >
> > Yes, we can use of_dma_find_controller(), but that will be a little
> > complicated than current solution. Since we need introduce one
> > structure to save the node to validate in the filter function like
> > below, which seems make things complicated. But if you still like to
> > use of_dma_find_controller(), I can change to use it in next version.
>
> Sorry I should have clarified more..
>
> of_dma_find_controller() is called by xlate, so you already run this
> check, so why use this :)

The of_dma_find_controller() can save the requested device node into
dma_spec, and in the of_dma_simple_xlate() function, it will call
dma_request_channel() to request one channel, but it did not validate
the device node to find the corresponding dma device in
dma_request_channel(). So we should in our filter function to validate
the device node with the device node specified by the dma_spec. Hope I
make things clear.

Vinod Koul April 30, 2019, 8:29 a.m. UTC | #5

On 30-04-19, 13:30, Baolin Wang wrote:
> On Mon, 29 Apr 2019 at 22:05, Vinod Koul <[e-mail]> wrote:
> >
> > On 29-04-19, 20:20, Baolin Wang wrote:
> > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <[e-mail]> wrote:
> > > >
> > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > From: Eric Long <[e-mail]>
> > > > >
> > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > device validation in filter function to check if the correct controller
> > > > > to be requested.
> > > > >
> > > > > Signed-off-by: Eric Long <[e-mail]>
> > > > > Signed-off-by: Baolin Wang <[e-mail]>
> > > > > ---
> > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > index 0f92e60..9f99d4b 100644
> > > > > --- a/drivers/dma/sprd-dma.c
> > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > >  {
> > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > +     struct of_phandle_args *dma_spec =
> > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > >       u32 slave_id = *(u32 *)param;
> > > > >
> > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > >
> > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > be useless!
> > >
> > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > complicated than current solution. Since we need introduce one
> > > structure to save the node to validate in the filter function like
> > > below, which seems make things complicated. But if you still like to
> > > use of_dma_find_controller(), I can change to use it in next version.
> >
> > Sorry I should have clarified more..
> >
> > of_dma_find_controller() is called by xlate, so you already run this
> > check, so why use this :)
> 
> The of_dma_find_controller() can save the requested device node into
> dma_spec, and in the of_dma_simple_xlate() function, it will call
> dma_request_channel() to request one channel, but it did not validate
> the device node to find the corresponding dma device in
> dma_request_channel(). So we should in our filter function to validate
> the device node with the device node specified by the dma_spec. Hope I
> make things clear.

But dma_request_channel() calls of_dma_request_slave_channel() which
invokes of_dma_find_controller() why is it broken for you if that is the
case..

(Exiting) Baolin Wang April 30, 2019, 8:34 a.m. UTC | #6

On Tue, 30 Apr 2019 at 16:30, Vinod Koul <[e-mail]> wrote:
>
> On 30-04-19, 13:30, Baolin Wang wrote:
> > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <[e-mail]> wrote:
> > >
> > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <[e-mail]> wrote:
> > > > >
> > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > From: Eric Long <[e-mail]>
> > > > > >
> > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > device validation in filter function to check if the correct controller
> > > > > > to be requested.
> > > > > >
> > > > > > Signed-off-by: Eric Long <[e-mail]>
> > > > > > Signed-off-by: Baolin Wang <[e-mail]>
> > > > > > ---
> > > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > index 0f92e60..9f99d4b 100644
> > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > >  {
> > > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > +     struct of_phandle_args *dma_spec =
> > > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > > >       u32 slave_id = *(u32 *)param;
> > > > > >
> > > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > > >
> > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > be useless!
> > > >
> > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > complicated than current solution. Since we need introduce one
> > > > structure to save the node to validate in the filter function like
> > > > below, which seems make things complicated. But if you still like to
> > > > use of_dma_find_controller(), I can change to use it in next version.
> > >
> > > Sorry I should have clarified more..
> > >
> > > of_dma_find_controller() is called by xlate, so you already run this
> > > check, so why use this :)
> >
> > The of_dma_find_controller() can save the requested device node into
> > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > dma_request_channel() to request one channel, but it did not validate
> > the device node to find the corresponding dma device in
> > dma_request_channel(). So we should in our filter function to validate
> > the device node with the device node specified by the dma_spec. Hope I
> > make things clear.
>
> But dma_request_channel() calls of_dma_request_slave_channel() which
> invokes of_dma_find_controller() why is it broken for you if that is the
> case..

No,the calling process should be:
dma_request_slave_channel()
--->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
----> dma_request_channel().

(Exiting) Baolin Wang April 30, 2019, 8:53 a.m. UTC | #7

Hi Vinod,

On Tue, 30 Apr 2019 at 16:34, Baolin Wang <[e-mail]> wrote:
>
> On Tue, 30 Apr 2019 at 16:30, Vinod Koul <[e-mail]> wrote:
> >
> > On 30-04-19, 13:30, Baolin Wang wrote:
> > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <[e-mail]> wrote:
> > > >
> > > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <[e-mail]> wrote:
> > > > > >
> > > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > > From: Eric Long <[e-mail]>
> > > > > > >
> > > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > > device validation in filter function to check if the correct controller
> > > > > > > to be requested.
> > > > > > >
> > > > > > > Signed-off-by: Eric Long <[e-mail]>
> > > > > > > Signed-off-by: Baolin Wang <[e-mail]>
> > > > > > > ---
> > > > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > > > >  1 file changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > > index 0f92e60..9f99d4b 100644
> > > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > > >  {
> > > > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > > +     struct of_phandle_args *dma_spec =
> > > > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > > > >       u32 slave_id = *(u32 *)param;
> > > > > > >
> > > > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > > > >
> > > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > > be useless!
> > > > >
> > > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > > complicated than current solution. Since we need introduce one
> > > > > structure to save the node to validate in the filter function like
> > > > > below, which seems make things complicated. But if you still like to
> > > > > use of_dma_find_controller(), I can change to use it in next version.
> > > >
> > > > Sorry I should have clarified more..
> > > >
> > > > of_dma_find_controller() is called by xlate, so you already run this
> > > > check, so why use this :)
> > >
> > > The of_dma_find_controller() can save the requested device node into
> > > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > > dma_request_channel() to request one channel, but it did not validate
> > > the device node to find the corresponding dma device in
> > > dma_request_channel(). So we should in our filter function to validate
> > > the device node with the device node specified by the dma_spec. Hope I
> > > make things clear.
> >
> > But dma_request_channel() calls of_dma_request_slave_channel() which
> > invokes of_dma_find_controller() why is it broken for you if that is the
> > case..
>
> No,the calling process should be:
> dma_request_slave_channel()
> --->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
> ----> dma_request_channel().
>

You can check other drivers, they also will save the device node to
validate in their filter function.
For example the imx-sdma driver:
https://elixir.bootlin.com/linux/v5.1-rc6/source/drivers/dma/imx-sdma.c#L1931

Vinod Koul May 2, 2019, 6:01 a.m. UTC | #8

On 30-04-19, 16:53, Baolin Wang wrote:
> Hi Vinod,
> 
> On Tue, 30 Apr 2019 at 16:34, Baolin Wang <[e-mail]> wrote:
> >
> > On Tue, 30 Apr 2019 at 16:30, Vinod Koul <[e-mail]> wrote:
> > >
> > > On 30-04-19, 13:30, Baolin Wang wrote:
> > > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <[e-mail]> wrote:
> > > > >
> > > > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <[e-mail]> wrote:
> > > > > > >
> > > > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > > > From: Eric Long <[e-mail]>
> > > > > > > >
> > > > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > > > device validation in filter function to check if the correct controller
> > > > > > > > to be requested.
> > > > > > > >
> > > > > > > > Signed-off-by: Eric Long <[e-mail]>
> > > > > > > > Signed-off-by: Baolin Wang <[e-mail]>
> > > > > > > > ---
> > > > > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > > > > >  1 file changed, 5 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > > > index 0f92e60..9f99d4b 100644
> > > > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > > > >  {
> > > > > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > > > +     struct of_phandle_args *dma_spec =
> > > > > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > > > > >       u32 slave_id = *(u32 *)param;
> > > > > > > >
> > > > > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > > > > >
> > > > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > > > be useless!
> > > > > >
> > > > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > > > complicated than current solution. Since we need introduce one
> > > > > > structure to save the node to validate in the filter function like
> > > > > > below, which seems make things complicated. But if you still like to
> > > > > > use of_dma_find_controller(), I can change to use it in next version.
> > > > >
> > > > > Sorry I should have clarified more..
> > > > >
> > > > > of_dma_find_controller() is called by xlate, so you already run this
> > > > > check, so why use this :)
> > > >
> > > > The of_dma_find_controller() can save the requested device node into
> > > > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > > > dma_request_channel() to request one channel, but it did not validate
> > > > the device node to find the corresponding dma device in
> > > > dma_request_channel(). So we should in our filter function to validate
> > > > the device node with the device node specified by the dma_spec. Hope I
> > > > make things clear.
> > >
> > > But dma_request_channel() calls of_dma_request_slave_channel() which
> > > invokes of_dma_find_controller() why is it broken for you if that is the
> > > case..
> >
> > No,the calling process should be:
> > dma_request_slave_channel()
> > --->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
> > ----> dma_request_channel().

The thing is that this is a generic issue, so fix should be in the core
and not in the driver. Agree in you case of_dma_find_controller() is not
invoked, so we should fix that in core

> 
> You can check other drivers, they also will save the device node to
> validate in their filter function.
> For example the imx-sdma driver:
> https://elixir.bootlin.com/linux/v5.1-rc6/source/drivers/dma/imx-sdma.c#L1931

Exactly, more the reason this should be in core :)

(Exiting) Baolin Wang May 6, 2019, 4:48 a.m. UTC | #9

Hi Vinod,

On Thu, 2 May 2019 at 14:01, Vinod Koul <[e-mail]> wrote:
>
> On 30-04-19, 16:53, Baolin Wang wrote:
> > Hi Vinod,
> >
> > On Tue, 30 Apr 2019 at 16:34, Baolin Wang <[e-mail]> wrote:
> > >
> > > On Tue, 30 Apr 2019 at 16:30, Vinod Koul <[e-mail]> wrote:
> > > >
> > > > On 30-04-19, 13:30, Baolin Wang wrote:
> > > > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <[e-mail]> wrote:
> > > > > >
> > > > > > On 29-04-19, 20:20, Baolin Wang wrote:
> > > > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > On 15-04-19, 20:14, Baolin Wang wrote:
> > > > > > > > > From: Eric Long <[e-mail]>
> > > > > > > > >
> > > > > > > > > Since we can support multiple DMA engine controllers, we should add
> > > > > > > > > device validation in filter function to check if the correct controller
> > > > > > > > > to be requested.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Eric Long <[e-mail]>
> > > > > > > > > Signed-off-by: Baolin Wang <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >  drivers/dma/sprd-dma.c |    5 +++++
> > > > > > > > >  1 file changed, 5 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> > > > > > > > > index 0f92e60..9f99d4b 100644
> > > > > > > > > --- a/drivers/dma/sprd-dma.c
> > > > > > > > > +++ b/drivers/dma/sprd-dma.c
> > > > > > > > > @@ -1020,8 +1020,13 @@ static void sprd_dma_free_desc(struct virt_dma_desc *vd)
> > > > > > > > >  static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
> > > > > > > > >  {
> > > > > > > > >       struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
> > > > > > > > > +     struct of_phandle_args *dma_spec =
> > > > > > > > > +             container_of(param, struct of_phandle_args, args[0]);
> > > > > > > > >       u32 slave_id = *(u32 *)param;
> > > > > > > > >
> > > > > > > > > +     if (chan->device->dev->of_node != dma_spec->np)
> > > > > > > >
> > > > > > > > Are you not using of_dma_find_controller() that does this, so this would
> > > > > > > > be useless!
> > > > > > >
> > > > > > > Yes, we can use of_dma_find_controller(), but that will be a little
> > > > > > > complicated than current solution. Since we need introduce one
> > > > > > > structure to save the node to validate in the filter function like
> > > > > > > below, which seems make things complicated. But if you still like to
> > > > > > > use of_dma_find_controller(), I can change to use it in next version.
> > > > > >
> > > > > > Sorry I should have clarified more..
> > > > > >
> > > > > > of_dma_find_controller() is called by xlate, so you already run this
> > > > > > check, so why use this :)
> > > > >
> > > > > The of_dma_find_controller() can save the requested device node into
> > > > > dma_spec, and in the of_dma_simple_xlate() function, it will call
> > > > > dma_request_channel() to request one channel, but it did not validate
> > > > > the device node to find the corresponding dma device in
> > > > > dma_request_channel(). So we should in our filter function to validate
> > > > > the device node with the device node specified by the dma_spec. Hope I
> > > > > make things clear.
> > > >
> > > > But dma_request_channel() calls of_dma_request_slave_channel() which
> > > > invokes of_dma_find_controller() why is it broken for you if that is the
> > > > case..
> > >
> > > No,the calling process should be:
> > > dma_request_slave_channel()
> > > --->dma_request_chan()--->of_dma_request_slave_channel()---->of_dma_simple_xlate()
> > > ----> dma_request_channel().
>
> The thing is that this is a generic issue, so fix should be in the core
> and not in the driver. Agree in you case of_dma_find_controller() is not
> invoked, so we should fix that in core
>
> >
> > You can check other drivers, they also will save the device node to
> > validate in their filter function.
> > For example the imx-sdma driver:
> > https://elixir.bootlin.com/linux/v5.1-rc6/source/drivers/dma/imx-sdma.c#L1931
>
> Exactly, more the reason this should be in core :)

Sorry for late reply due to my holiday.

OK, I can move the fix into the core. So I think I will drop this
patch from my patchset, and I will create another patch set to fix the
device node validation issue with converting other drivers which did
the similar things. Thanks.

Patch

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 0f92e60..9f99d4b 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -1020,8 +1020,13 @@  static void sprd_dma_free_desc(struct virt_dma_desc *vd)
 static bool sprd_dma_filter_fn(struct dma_chan *chan, void *param)
 {
 	struct sprd_dma_chn *schan = to_sprd_dma_chan(chan);
+	struct of_phandle_args *dma_spec =
+		container_of(param, struct of_phandle_args, args[0]);
 	u32 slave_id = *(u32 *)param;
 
+	if (chan->device->dev->of_node != dma_spec->np)
+		return false;
+
 	schan->dev_id = slave_id;
 	return true;
 }