| Message ID | d77dca51a14087873627d735a17adcfde5517398.1555330115.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some bugs and add new feature for Spreadtrum DMA engine | expand |
On 15-04-19, 20:14, Baolin Wang wrote: > From: Eric Long <eric.long@unisoc.com> > > 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 <eric.long@unisoc.com> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > 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 -- ~Vinod
On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote: > > On 15-04-19, 20:14, Baolin Wang wrote: > > From: Eric Long <eric.long@unisoc.com> > > > > 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 <eric.long@unisoc.com> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > --- > > 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); -- Baolin Wang Best Regards
On 29-04-19, 20:20, Baolin Wang wrote: > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote: > > > > On 15-04-19, 20:14, Baolin Wang wrote: > > > From: Eric Long <eric.long@unisoc.com> > > > > > > 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 <eric.long@unisoc.com> > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > --- > > > 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 :) -- ~Vinod
On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote: > > On 29-04-19, 20:20, Baolin Wang wrote: > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > On 15-04-19, 20:14, Baolin Wang wrote: > > > > From: Eric Long <eric.long@unisoc.com> > > > > > > > > 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 <eric.long@unisoc.com> > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > > --- > > > > 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. -- Baolin Wang Best Regards
On 30-04-19, 13:30, Baolin Wang wrote: > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote: > > > > On 29-04-19, 20:20, Baolin Wang wrote: > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > On 15-04-19, 20:14, Baolin Wang wrote: > > > > > From: Eric Long <eric.long@unisoc.com> > > > > > > > > > > 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 <eric.long@unisoc.com> > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > > > --- > > > > > 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.. -- ~Vinod
On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote: > > On 30-04-19, 13:30, Baolin Wang wrote: > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > On 29-04-19, 20:20, Baolin Wang wrote: > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > > > On 15-04-19, 20:14, Baolin Wang wrote: > > > > > > From: Eric Long <eric.long@unisoc.com> > > > > > > > > > > > > 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 <eric.long@unisoc.com> > > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > > > > --- > > > > > > 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(). -- Baolin Wang Best Regards
Hi Vinod, On Tue, 30 Apr 2019 at 16:34, Baolin Wang <baolin.wang@linaro.org> wrote: > > On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote: > > > > On 30-04-19, 13:30, Baolin Wang wrote: > > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > On 29-04-19, 20:20, Baolin Wang wrote: > > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > > > > > On 15-04-19, 20:14, Baolin Wang wrote: > > > > > > > From: Eric Long <eric.long@unisoc.com> > > > > > > > > > > > > > > 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 <eric.long@unisoc.com> > > > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > > > > > --- > > > > > > > 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 -- Baolin Wang Best Regards
On 30-04-19, 16:53, Baolin Wang wrote: > Hi Vinod, > > On Tue, 30 Apr 2019 at 16:34, Baolin Wang <baolin.wang@linaro.org> wrote: > > > > On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > On 30-04-19, 13:30, Baolin Wang wrote: > > > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > > > On 29-04-19, 20:20, Baolin Wang wrote: > > > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > > > > > > > On 15-04-19, 20:14, Baolin Wang wrote: > > > > > > > > From: Eric Long <eric.long@unisoc.com> > > > > > > > > > > > > > > > > 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 <eric.long@unisoc.com> > > > > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > > > > > > --- > > > > > > > > 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 :) -- ~Vinod
Hi Vinod, On Thu, 2 May 2019 at 14:01, Vinod Koul <vkoul@kernel.org> wrote: > > On 30-04-19, 16:53, Baolin Wang wrote: > > Hi Vinod, > > > > On Tue, 30 Apr 2019 at 16:34, Baolin Wang <baolin.wang@linaro.org> wrote: > > > > > > On Tue, 30 Apr 2019 at 16:30, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > On 30-04-19, 13:30, Baolin Wang wrote: > > > > > On Mon, 29 Apr 2019 at 22:05, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > > > > > On 29-04-19, 20:20, Baolin Wang wrote: > > > > > > > On Mon, 29 Apr 2019 at 19:57, Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > > > > > > > > > On 15-04-19, 20:14, Baolin Wang wrote: > > > > > > > > > From: Eric Long <eric.long@unisoc.com> > > > > > > > > > > > > > > > > > > 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 <eric.long@unisoc.com> > > > > > > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > > > > > > > --- > > > > > > > > > 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. -- Baolin Wang Best Regards
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; }