* Re: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id
2016-04-22 12:44 [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id Shardar Shariff Md
@ 2016-04-22 13:33 ` Jon Hunter
2016-04-22 13:56 ` Shardar Mohammed
2016-04-22 13:35 ` Thierry Reding
2016-04-22 13:43 ` Jon Hunter
2 siblings, 1 reply; 8+ messages in thread
From: Jon Hunter @ 2016-04-22 13:33 UTC (permalink / raw)
To: Shardar Shariff Md, ldewangan, vinod.koul, dan.j.williams,
swarren, thierry.reding, gnurou, dmaengine, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id
2016-04-22 13:33 ` Jon Hunter
@ 2016-04-22 13:56 ` Shardar Mohammed
2016-04-22 14:40 ` Jon Hunter
0 siblings, 1 reply; 8+ messages in thread
From: Shardar Mohammed @ 2016-04-22 13:56 UTC (permalink / raw)
To: Jonathan Hunter, Laxman Dewangan, vinod.koul@intel.com,
dan.j.williams@intel.com, swarren@wwwdotorg.org,
thierry.reding@gmail.com, gnurou@gmail.com,
dmaengine@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
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@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.
[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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id
2016-04-22 13:56 ` Shardar Mohammed
@ 2016-04-22 14:40 ` Jon Hunter
0 siblings, 0 replies; 8+ messages in thread
From: Jon Hunter @ 2016-04-22 14:40 UTC (permalink / raw)
To: Shardar Mohammed, Laxman Dewangan, vinod.koul@intel.com,
dan.j.williams@intel.com, swarren@wwwdotorg.org,
thierry.reding@gmail.com, gnurou@gmail.com,
dmaengine@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
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@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.
> [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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id
2016-04-22 12:44 [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id Shardar Shariff Md
2016-04-22 13:33 ` Jon Hunter
@ 2016-04-22 13:35 ` Thierry Reding
2016-04-22 13:58 ` Shardar Mohammed
2016-04-22 13:43 ` Jon Hunter
2 siblings, 1 reply; 8+ messages in thread
From: Thierry Reding @ 2016-04-22 13:35 UTC (permalink / raw)
To: Shardar Shariff Md
Cc: ldewangan, vinod.koul, dan.j.williams, swarren, gnurou, dmaengine,
linux-tegra, linux-kernel, jonathanh
[-- Attachment #1: Type: text/plain, Size: 2487 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@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;
> + }
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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id
2016-04-22 13:35 ` Thierry Reding
@ 2016-04-22 13:58 ` Shardar Mohammed
0 siblings, 0 replies; 8+ messages in thread
From: Shardar Mohammed @ 2016-04-22 13:58 UTC (permalink / raw)
To: Thierry Reding
Cc: Laxman Dewangan, vinod.koul@intel.com, dan.j.williams@intel.com,
swarren@wwwdotorg.org, gnurou@gmail.com,
dmaengine@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org, Jonathan Hunter
> 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@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;
> > + }
>
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id
2016-04-22 12:44 [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id Shardar Shariff Md
2016-04-22 13:33 ` Jon Hunter
2016-04-22 13:35 ` Thierry Reding
@ 2016-04-22 13:43 ` Jon Hunter
2016-04-22 13:57 ` Shardar Mohammed
2 siblings, 1 reply; 8+ messages in thread
From: Jon Hunter @ 2016-04-22 13:43 UTC (permalink / raw)
To: Shardar Shariff Md, ldewangan, vinod.koul, dan.j.williams,
swarren, thierry.reding, gnurou, dmaengine, linux-tegra,
linux-kernel
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!
Cheers
Jon
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH v2] dmaengine: tegra-apb: proper default init of channel slave_id
2016-04-22 13:43 ` Jon Hunter
@ 2016-04-22 13:57 ` Shardar Mohammed
0 siblings, 0 replies; 8+ messages in thread
From: Shardar Mohammed @ 2016-04-22 13:57 UTC (permalink / raw)
To: Jonathan Hunter, Laxman Dewangan, vinod.koul@intel.com,
dan.j.williams@intel.com, swarren@wwwdotorg.org,
thierry.reding@gmail.com, gnurou@gmail.com,
dmaengine@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread