[PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id

[PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id

[Shardar Shariff Md]

Initialize default channel slave_id(req_sel) to invalid id
(i.e max supported slave id + 1) to avoid overwriting of slave_id
during tegra_dma_slave_config() with client data if slave_id
is not initialized through DT

Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

---
- Instead of initializing the slave id to -1 define macros for
  max slave id and invalid slave id and do the checks accordingly.
---
 drivers/dma/tegra20-apb-dma.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 3871f29..2957e26 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -114,6 +114,9 @@
 /* Channel base address offset from APBDMA base address */
 #define TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET	0x1000
 
+#define TEGRA_APBDMA_SLAVE_ID_MAX		31
+#define TEGRA_APBDMA_SLAVE_ID_INVALID		(TEGRA_APBDMA_SLAVE_ID_MAX + 1)
+
 struct tegra_dma;
 
 /*
@@ -353,7 +356,7 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
 	}
 
 	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
-	if (!tdc->slave_id)
+	if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID)
 		tdc->slave_id = sconfig->slave_id;
 	tdc->config_init = true;
 	return 0;
@@ -1236,7 +1239,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 	}
 	pm_runtime_put(tdma->dev);
 
-	tdc->slave_id = 0;
+	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
 }
 
 static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
@@ -1253,6 +1256,11 @@ static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
 	tdc = to_tegra_dma_chan(chan);
 	tdc->slave_id = dma_spec->args[0];
 
+	if (tdc->slave_id > TEGRA_APBDMA_SLAVE_ID_MAX) {
+		dev_err(tdc2dev(tdc), "Invalid slave id\n");
+		return NULL;
+	}
+
 	return chan;
 }
 
@@ -1389,6 +1397,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 				&tdma->dma_dev.channels);
 		tdc->tdma = tdma;
 		tdc->id = i;
+		tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
 
 		tasklet_init(&tdc->tasklet, tegra_dma_tasklet,
 				(unsigned long)tdc);
-- 
1.8.1.5

Re: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id

[Jon Hunter]

Hi Shardar,

On 22/04/16 13:44, Shardar Shariff Md wrote:
> Initialize default channel slave_id(req_sel) to invalid id
> (i.e max supported slave id + 1) to avoid overwriting of slave_id
> during tegra_dma_slave_config() with client data if slave_id
> is not initialized through DT
> 
> Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
> 
> ---
> - Instead of initializing the slave id to -1 define macros for
>   max slave id and invalid slave id and do the checks accordingly.
> ---
>  drivers/dma/tegra20-apb-dma.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 3871f29..2957e26 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -114,6 +114,9 @@
>  /* Channel base address offset from APBDMA base address */
>  #define TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET	0x1000
>  
> +#define TEGRA_APBDMA_SLAVE_ID_MAX		31

Sorry, thinking about this some more, given that the ID is programmed into
a field in the CSR register could we just define as mask here for the CSR
field ...

#define TEGRA_APBDMA_CSR_REQ_SEL_MASK	0x1f

Note this should be defined with the other CSR register field definitions.

> +#define TEGRA_APBDMA_SLAVE_ID_INVALID		(TEGRA_APBDMA_SLAVE_ID_MAX + 1)
>
>  struct tegra_dma;
>  
>  /*
> @@ -353,7 +356,7 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
>  	}
>  
>  	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
> -	if (!tdc->slave_id)
> +	if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID)
>  		tdc->slave_id = sconfig->slave_id;

I believe I mentioned before that here we should ...

	if (tdc->slave_id != TEGRA_APBDMA_SLAVE_ID_INVALID)
		return -EBUSY;

	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
	tdc->slave_id = sconfig->slave_id;

>  	tdc->config_init = true;
>  	return 0;
> @@ -1236,7 +1239,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>  	}
>  	pm_runtime_put(tdma->dev);
>  
> -	tdc->slave_id = 0;
> +	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
>  }
>  
>  static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
> @@ -1253,6 +1256,11 @@ static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
>  	tdc = to_tegra_dma_chan(chan);
>  	tdc->slave_id = dma_spec->args[0];
>  
> +	if (tdc->slave_id > TEGRA_APBDMA_SLAVE_ID_MAX) {
> +		dev_err(tdc2dev(tdc), "Invalid slave id\n");
> +		return NULL;
> +	}
> +
>  	return chan;
>  }
>  
> @@ -1389,6 +1397,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  				&tdma->dma_dev.channels);
>  		tdc->tdma = tdma;
>  		tdc->id = i;
> +		tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
>  
>  		tasklet_init(&tdc->tasklet, tegra_dma_tasklet,
>  				(unsigned long)tdc);

Otherwise looks good.

Cheers
Jon

Re: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2516 bytes --]

On Fri, Apr 22, 2016 at 06:14:53PM +0530, Shardar Shariff Md wrote:
> Initialize default channel slave_id(req_sel) to invalid id
> (i.e max supported slave id + 1) to avoid overwriting of slave_id
> during tegra_dma_slave_config() with client data if slave_id
> is not initialized through DT
> 
> Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ---
> - Instead of initializing the slave id to -1 define macros for
>   max slave id and invalid slave id and do the checks accordingly.
> ---
>  drivers/dma/tegra20-apb-dma.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 3871f29..2957e26 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -114,6 +114,9 @@
>  /* Channel base address offset from APBDMA base address */
>  #define TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET	0x1000
>  
> +#define TEGRA_APBDMA_SLAVE_ID_MAX		31
> +#define TEGRA_APBDMA_SLAVE_ID_INVALID		(TEGRA_APBDMA_SLAVE_ID_MAX + 1)
> +
>  struct tegra_dma;
>  
>  /*
> @@ -353,7 +356,7 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
>  	}
>  
>  	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
> -	if (!tdc->slave_id)
> +	if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID)
>  		tdc->slave_id = sconfig->slave_id;
>  	tdc->config_init = true;
>  	return 0;
> @@ -1236,7 +1239,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>  	}
>  	pm_runtime_put(tdma->dev);
>  
> -	tdc->slave_id = 0;
> +	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
>  }
>  
>  static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
> @@ -1253,6 +1256,11 @@ static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
>  	tdc = to_tegra_dma_chan(chan);
>  	tdc->slave_id = dma_spec->args[0];
>  
> +	if (tdc->slave_id > TEGRA_APBDMA_SLAVE_ID_MAX) {
> +		dev_err(tdc2dev(tdc), "Invalid slave id\n");
> +		return NULL;
> +	}

Should this check happen before the channel is requested? As it is,
you're setting up the private data of the channel with a slave_id and
error out after that. As I understand the channel would already have
its ID set to the invalid ID, but without going too deep into the DMA
engine core code it looks as though once requested, the channel needs
to be released again (which the invalid ID handling code here doesn't).

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id

[Jon Hunter]

On 22/04/16 13:44, Shardar Shariff Md wrote:
> Initialize default channel slave_id(req_sel) to invalid id
> (i.e max supported slave id + 1) to avoid overwriting of slave_id
> during tegra_dma_slave_config() with client data if slave_id
> is not initialized through DT
> 
> Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ---
> - Instead of initializing the slave id to -1 define macros for
>   max slave id and invalid slave id and do the checks accordingly.
> ---
>  drivers/dma/tegra20-apb-dma.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 3871f29..2957e26 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -114,6 +114,9 @@
>  /* Channel base address offset from APBDMA base address */
>  #define TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET	0x1000
>  
> +#define TEGRA_APBDMA_SLAVE_ID_MAX		31
> +#define TEGRA_APBDMA_SLAVE_ID_INVALID		(TEGRA_APBDMA_SLAVE_ID_MAX + 1)
> +
>  struct tegra_dma;
>  
>  /*
> @@ -353,7 +356,7 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
>  	}
>  
>  	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
> -	if (!tdc->slave_id)
> +	if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID)
>  		tdc->slave_id = sconfig->slave_id;
>  	tdc->config_init = true;
>  	return 0;
> @@ -1236,7 +1239,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>  	}
>  	pm_runtime_put(tdma->dev);
>  
> -	tdc->slave_id = 0;
> +	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
>  }
>  
>  static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
> @@ -1253,6 +1256,11 @@ static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
>  	tdc = to_tegra_dma_chan(chan);
>  	tdc->slave_id = dma_spec->args[0];
>  
> +	if (tdc->slave_id > TEGRA_APBDMA_SLAVE_ID_MAX) {
> +		dev_err(tdc2dev(tdc), "Invalid slave id\n");
> +		return NULL;
> +	}

Please move this test to before the dma_get_any_slave_channel(),
otherwise we could leak the DMA channel!

Cheers
Jon

RE: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id

[Shardar Mohammed]

Thanks for the review, please check my comments inline in [Shardar].

> On 22/04/16 13:44, Shardar Shariff Md wrote:
> > Initialize default channel slave_id(req_sel) to invalid id (i.e max
> > supported slave id + 1) to avoid overwriting of slave_id during
> > tegra_dma_slave_config() with client data if slave_id is not
> > initialized through DT
> >
> > Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > ---
> > - Instead of initializing the slave id to -1 define macros for
> >   max slave id and invalid slave id and do the checks accordingly.
> > ---
> >  drivers/dma/tegra20-apb-dma.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/dma/tegra20-apb-dma.c
> > b/drivers/dma/tegra20-apb-dma.c index 3871f29..2957e26 100644
> > --- a/drivers/dma/tegra20-apb-dma.c
> > +++ b/drivers/dma/tegra20-apb-dma.c
> > @@ -114,6 +114,9 @@
> >  /* Channel base address offset from APBDMA base address */
> >  #define TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET	0x1000
> >
> > +#define TEGRA_APBDMA_SLAVE_ID_MAX		31
> 
> Sorry, thinking about this some more, given that the ID is programmed into a
> field in the CSR register could we just define as mask here for the CSR field ...
> 
> #define TEGRA_APBDMA_CSR_REQ_SEL_MASK	0x1f
> 
> Note this should be defined with the other CSR register field definitions.
[Shardar] Will take care of it. One doubt do you mean below.
Will define this with other CSR register fields.
#define TEGRA_APBDMA_CSR_REQ_SEL_MASK	0x1f

#define TEGRA_APBDMA_SLAVE_ID_MAX		TEGRA_APBDMA_CSR_REQ_SEL_MASK

> 
> > +#define TEGRA_APBDMA_SLAVE_ID_INVALID
> 	(TEGRA_APBDMA_SLAVE_ID_MAX + 1)
> >
> >  struct tegra_dma;
> >
> >  /*
> > @@ -353,7 +356,7 @@ static int tegra_dma_slave_config(struct dma_chan
> *dc,
> >  	}
> >
> >  	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
> > -	if (!tdc->slave_id)
> > +	if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID)
> >  		tdc->slave_id = sconfig->slave_id;
> 
> I believe I mentioned before that here we should ...
> 
> 	if (tdc->slave_id != TEGRA_APBDMA_SLAVE_ID_INVALID)
> 		return -EBUSY;

[Shardar] By the time this function tegra_dma_slave_config is called, slave_id here is already initialized through DT(provided option) through function tegra_dma_of_xlate().
And filling up the slave id here from sconfig structure that is provided by dma client driver is used here if it dma client is not initialized through DT. 
Please correct me if am wrong.

> 
> 	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
> 	tdc->slave_id = sconfig->slave_id;
> 
> >  	tdc->config_init = true;
> >  	return 0;
> > @@ -1236,7 +1239,7 @@ static void
> tegra_dma_free_chan_resources(struct dma_chan *dc)
> >  	}
> >  	pm_runtime_put(tdma->dev);
> >
> > -	tdc->slave_id = 0;
> > +	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
> >  }
> >
> >  static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args
> > *dma_spec, @@ -1253,6 +1256,11 @@ static struct dma_chan
> *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
> >  	tdc = to_tegra_dma_chan(chan);
> >  	tdc->slave_id = dma_spec->args[0];
> >
> > +	if (tdc->slave_id > TEGRA_APBDMA_SLAVE_ID_MAX) {
> > +		dev_err(tdc2dev(tdc), "Invalid slave id\n");
> > +		return NULL;
> > +	}
> > +
> >  	return chan;
> >  }
> >
> > @@ -1389,6 +1397,7 @@ static int tegra_dma_probe(struct
> platform_device *pdev)
> >  				&tdma->dma_dev.channels);
> >  		tdc->tdma = tdma;
> >  		tdc->id = i;
> > +		tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
> >
> >  		tasklet_init(&tdc->tasklet, tegra_dma_tasklet,
> >  				(unsigned long)tdc);
> 
> Otherwise looks good.
> 
> Cheers
> Jon

RE: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id

[Shardar Mohammed]

> On 22/04/16 13:44, Shardar Shariff Md wrote:
> > Initialize default channel slave_id(req_sel) to invalid id (i.e max
> > supported slave id + 1) to avoid overwriting of slave_id during
> > tegra_dma_slave_config() with client data if slave_id is not
> > initialized through DT
> >
> > Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
> >
> > ---
> > - Instead of initializing the slave id to -1 define macros for
> >   max slave id and invalid slave id and do the checks accordingly.
> > ---
> >  drivers/dma/tegra20-apb-dma.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/dma/tegra20-apb-dma.c
> > b/drivers/dma/tegra20-apb-dma.c index 3871f29..2957e26 100644
> > --- a/drivers/dma/tegra20-apb-dma.c
> > +++ b/drivers/dma/tegra20-apb-dma.c
> > @@ -114,6 +114,9 @@
> >  /* Channel base address offset from APBDMA base address */
> >  #define TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET	0x1000
> >
> > +#define TEGRA_APBDMA_SLAVE_ID_MAX		31
> > +#define TEGRA_APBDMA_SLAVE_ID_INVALID
> 	(TEGRA_APBDMA_SLAVE_ID_MAX + 1)
> > +
> >  struct tegra_dma;
> >
> >  /*
> > @@ -353,7 +356,7 @@ static int tegra_dma_slave_config(struct dma_chan
> *dc,
> >  	}
> >
> >  	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
> > -	if (!tdc->slave_id)
> > +	if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID)
> >  		tdc->slave_id = sconfig->slave_id;
> >  	tdc->config_init = true;
> >  	return 0;
> > @@ -1236,7 +1239,7 @@ static void
> tegra_dma_free_chan_resources(struct dma_chan *dc)
> >  	}
> >  	pm_runtime_put(tdma->dev);
> >
> > -	tdc->slave_id = 0;
> > +	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
> >  }
> >
> >  static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args
> > *dma_spec, @@ -1253,6 +1256,11 @@ static struct dma_chan
> *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
> >  	tdc = to_tegra_dma_chan(chan);
> >  	tdc->slave_id = dma_spec->args[0];
> >
> > +	if (tdc->slave_id > TEGRA_APBDMA_SLAVE_ID_MAX) {
> > +		dev_err(tdc2dev(tdc), "Invalid slave id\n");
> > +		return NULL;
> > +	}
> 
> Please move this test to before the dma_get_any_slave_channel(),
> otherwise we could leak the DMA channel!

[Shardar] Will take care of this.
> 
> Cheers
> Jon

RE: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id

[Shardar Mohammed]

> On Fri, Apr 22, 2016 at 06:14:53PM +0530, Shardar Shariff Md wrote:
> > Initialize default channel slave_id(req_sel) to invalid id (i.e max
> > supported slave id + 1) to avoid overwriting of slave_id during
> > tegra_dma_slave_config() with client data if slave_id is not
> > initialized through DT
> >
> > Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > ---
> > - Instead of initializing the slave id to -1 define macros for
> >   max slave id and invalid slave id and do the checks accordingly.
> > ---
> >  drivers/dma/tegra20-apb-dma.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/dma/tegra20-apb-dma.c
> > b/drivers/dma/tegra20-apb-dma.c index 3871f29..2957e26 100644
> > --- a/drivers/dma/tegra20-apb-dma.c
> > +++ b/drivers/dma/tegra20-apb-dma.c
> > @@ -114,6 +114,9 @@
> >  /* Channel base address offset from APBDMA base address */
> >  #define TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET	0x1000
> >
> > +#define TEGRA_APBDMA_SLAVE_ID_MAX		31
> > +#define TEGRA_APBDMA_SLAVE_ID_INVALID
> 	(TEGRA_APBDMA_SLAVE_ID_MAX + 1)
> > +
> >  struct tegra_dma;
> >
> >  /*
> > @@ -353,7 +356,7 @@ static int tegra_dma_slave_config(struct dma_chan
> *dc,
> >  	}
> >
> >  	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
> > -	if (!tdc->slave_id)
> > +	if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID)
> >  		tdc->slave_id = sconfig->slave_id;
> >  	tdc->config_init = true;
> >  	return 0;
> > @@ -1236,7 +1239,7 @@ static void
> tegra_dma_free_chan_resources(struct dma_chan *dc)
> >  	}
> >  	pm_runtime_put(tdma->dev);
> >
> > -	tdc->slave_id = 0;
> > +	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
> >  }
> >
> >  static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args
> > *dma_spec, @@ -1253,6 +1256,11 @@ static struct dma_chan
> *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
> >  	tdc = to_tegra_dma_chan(chan);
> >  	tdc->slave_id = dma_spec->args[0];
> >
> > +	if (tdc->slave_id > TEGRA_APBDMA_SLAVE_ID_MAX) {
> > +		dev_err(tdc2dev(tdc), "Invalid slave id\n");
> > +		return NULL;
> > +	}
> 
> Should this check happen before the channel is requested? As it is, you're
> setting up the private data of the channel with a slave_id and error out after
> that. As I understand the channel would already have its ID set to the invalid
> ID, but without going too deep into the DMA engine core code it looks as
> though once requested, the channel needs to be released again (which the
> invalid ID handling code here doesn't).	
[Shardar] Thanks will correct this.
> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1

Re: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id

[Jon Hunter]

On 22/04/16 14:56, Shardar Mohammed wrote:
> Thanks for the review, please check my comments inline in [Shardar].
> 
>> On 22/04/16 13:44, Shardar Shariff Md wrote:
>>> Initialize default channel slave_id(req_sel) to invalid id (i.e max
>>> supported slave id + 1) to avoid overwriting of slave_id during
>>> tegra_dma_slave_config() with client data if slave_id is not
>>> initialized through DT
>>>
>>> Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> ---
>>> - Instead of initializing the slave id to -1 define macros for
>>>   max slave id and invalid slave id and do the checks accordingly.
>>> ---
>>>  drivers/dma/tegra20-apb-dma.c | 13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c
>>> b/drivers/dma/tegra20-apb-dma.c index 3871f29..2957e26 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>> @@ -114,6 +114,9 @@
>>>  /* Channel base address offset from APBDMA base address */
>>>  #define TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET	0x1000
>>>
>>> +#define TEGRA_APBDMA_SLAVE_ID_MAX		31
>>
>> Sorry, thinking about this some more, given that the ID is programmed into a
>> field in the CSR register could we just define as mask here for the CSR field ...
>>
>> #define TEGRA_APBDMA_CSR_REQ_SEL_MASK	0x1f
>>
>> Note this should be defined with the other CSR register field definitions.
> [Shardar] Will take care of it. One doubt do you mean below.
> Will define this with other CSR register fields.
> #define TEGRA_APBDMA_CSR_REQ_SEL_MASK	0x1f
> 
> #define TEGRA_APBDMA_SLAVE_ID_MAX		TEGRA_APBDMA_CSR_REQ_SEL_MASK

We don't need the MAX definition as well.

>>
>>> +#define TEGRA_APBDMA_SLAVE_ID_INVALID
>> 	(TEGRA_APBDMA_SLAVE_ID_MAX + 1)
>>>
>>>  struct tegra_dma;
>>>
>>>  /*
>>> @@ -353,7 +356,7 @@ static int tegra_dma_slave_config(struct dma_chan
>> *dc,
>>>  	}
>>>
>>>  	memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
>>> -	if (!tdc->slave_id)
>>> +	if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID)
>>>  		tdc->slave_id = sconfig->slave_id;
>>
>> I believe I mentioned before that here we should ...
>>
>> 	if (tdc->slave_id != TEGRA_APBDMA_SLAVE_ID_INVALID)
>> 		return -EBUSY;
> 
> [Shardar] By the time this function tegra_dma_slave_config is called, slave_id here is already initialized through DT(provided option) through function tegra_dma_of_xlate().
> And filling up the slave id here from sconfig structure that is provided by dma client driver is used here if it dma client is not initialized through DT. 
> Please correct me if am wrong.

Yes you are right. So on further inspection this allows the user to set
the slave ID if it is not set yet. So I think we need ...

	if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID) {
		if (sconfig->slave_id > TEGRA_APBDMA_CSR_REQ_SEL_MASK)
			return -EINVAL;
		else
			tdc->slave_id = sconfig->slave_id;
	}

Cheers
Jon