[v3,1/2] dmaengine: add interface of dma_get_slave_channel

Commit Message

Vinod Koul Aug. 19, 2013, 5:25 a.m. UTC

On Thu, Aug 15, 2013 at 03:02:42PM +0800, Zhangfei Gao wrote:
> Suggested by Arnd, add dma_get_slave_channel interface
> Dma host driver could get specific channel specificied by request line, rather than filter.
> 
> host example:
> static struct dma_chan *xx_of_dma_simple_xlate(struct of_phandle_args *dma_spec,
> 		struct of_dma *ofdma)
> {
> 	struct xx_dma_dev *d = ofdma->of_dma_data;
> 	unsigned int request = dma_spec->args[0];
> 
> 	if (request > d->dma_requests)
> 		return NULL;
> 
> 	return dma_get_slave_channel(&(d->chans[request].vc.chan));
> }
> 
> probe:
> of_dma_controller_register((&op->dev)->of_node, xx_of_dma_simple_xlate, d);
> 
> Signed-off-by: Zhangfei Gao <[e-mail]>
> Acked-by: Arnd Bergmann <[e-mail]>
> ---
>  drivers/dma/dmaengine.c   |   25 +++++++++++++++++++++++++
>  include/linux/dmaengine.h |    1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 9e56745..38dd8c3 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -504,6 +504,31 @@ static struct dma_chan *private_candidate(const dma_cap_mask_t *mask,
>  }
>  
>  /**
> + * dma_request_channel - try to get specific channel exclusively
> + * @chan: target channel
> + */
> +struct dma_chan *dma_get_slave_channel(struct dma_chan *chan)
> +{
> +	int err = -EBUSY;
> +
> +	/* lock against __dma_request_channel */
> +	mutex_lock(&dma_list_mutex);
> +
> +	if (chan->client_count == 0) {
> +		err = dma_chan_get(chan);
> +		if (err)
> +			pr_debug("%s: failed to get %s: (%d)\n",
> +				__func__, dma_chan_name(chan), err);
> +	} else
> +		chan = NULL;
> +
> +	mutex_unlock(&dma_list_mutex);
> +
> +	return chan;
> +}
> +EXPORT_SYMBOL_GPL(dma_get_slave_channel);
No, this is not the right approach.

When patch is applied and you find an issue you fix the issue. I have already
pushed and merged this. So removing is a bad idea.
you provide a fix for this miss.

Also for this you need to give due credit to Dan (reporter). That is how the
process works.

Anyway I have applied the below fix and will push it out.

---
dmaengine: fix - error: potential NULL dereference 'chan'

commit 7bb587f4 "dmaengine: add interface of dma_get_slave_channel" introduced
the above error so fix it

Reported-by: Dan Carpenter <[e-mail]>
Suggested-by: Zhangfei Gao <[e-mail]>
Signed-off-by: Vinod Koul <[e-mail]>
---
 drivers/dma/dmaengine.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)


--
~Vinod

Comments

Zhangfei Gao Aug. 20, 2013, 7:58 a.m. UTC | #1

> No, this is not the right approach.
>
> When patch is applied and you find an issue you fix the issue. I have already
> pushed and merged this. So removing is a bad idea.
> you provide a fix for this miss.
>
> Also for this you need to give due credit to Dan (reporter). That is how the
> process works.
>
> Anyway I have applied the below fix and will push it out.
>
> ---
> dmaengine: fix - error: potential NULL dereference 'chan'
>
> commit 7bb587f4 "dmaengine: add interface of dma_get_slave_channel" introduced
> the above error so fix it
>
> Reported-by: Dan Carpenter <[e-mail]>
> Suggested-by: Zhangfei Gao <[e-mail]>
> Signed-off-by: Vinod Koul <[e-mail]>

Thanks Vinod for the kind education.
Understand, will take care next time.

BTW, it is strange I do not see such compile warning here :(

Dan Carpenter Aug. 20, 2013, 8:13 a.m. UTC | #2

On Tue, Aug 20, 2013 at 03:58:41PM +0800, zhangfei gao wrote:

> BTW, it is strange I do not see such compile warning here :(

This was a Smatch static checker warning, not a regular build
warning.

regards,
dan carpenter

Vinod Koul Aug. 20, 2013, 8:22 a.m. UTC | #3

On Tue, Aug 20, 2013 at 03:58:41PM +0800, zhangfei gao wrote:
> > No, this is not the right approach.
> >
> > When patch is applied and you find an issue you fix the issue. I have already
> > pushed and merged this. So removing is a bad idea.
> > you provide a fix for this miss.
> >
> > Also for this you need to give due credit to Dan (reporter). That is how the
> > process works.
> >
> > Anyway I have applied the below fix and will push it out.
> >
> > ---
> > dmaengine: fix - error: potential NULL dereference 'chan'
> >
> > commit 7bb587f4 "dmaengine: add interface of dma_get_slave_channel" introduced
> > the above error so fix it
> >
> > Reported-by: Dan Carpenter <[e-mail]>
> > Suggested-by: Zhangfei Gao <[e-mail]>
> > Signed-off-by: Vinod Koul <[e-mail]>
> 
> Thanks Vinod for the kind education.
> Understand, will take care next time.
No issues...

> 
> BTW, it is strange I do not see such compile warning here :(
Try running smatch and you will see. Compiler will not worry about these, static
checkers would. Since the bug report is from Dan, my guess is it would be
triggered by Smatch.

~Vinod

Vinod Koul Aug. 20, 2013, 8:22 a.m. UTC | #4

On Tue, Aug 20, 2013 at 11:13:48AM +0300, Dan Carpenter wrote:
> On Tue, Aug 20, 2013 at 03:58:41PM +0800, zhangfei gao wrote:
> 
> > BTW, it is strange I do not see such compile warning here :(
> 
> This was a Smatch static checker warning, not a regular build
> warning.
my guess was right!

~Vinod

Zhangfei Gao Aug. 27, 2013, 2:04 a.m. UTC | #5

On 13-08-20 04:13 PM, Dan Carpenter wrote:
> On Tue, Aug 20, 2013 at 03:58:41PM +0800, zhangfei gao wrote:
>
>> BTW, it is strange I do not see such compile warning here :(
>
> This was a Smatch static checker warning, not a regular build
> warning.
>
> regards,
> dan carpenter
>
Dear Dan

Thanks for introducing this valuable tool.
Have download from git://repo.or.cz/smatch.git

When trying use this tool, could get some warning,
make CHECK="~/bin/smatch -p=kernel --no-data" C=1 | tee warns.txt

Also some strange warning, like "SQL error #2: no such table: 
caller_info", is it ignored?

Besides, when test this case for check smatch setting.
Still not reproduce the issue, should be not config correct?

Only get
drivers/dma/dmaengine.c:845 dma_async_device_register() warn: possible 
memory leak of 'idr_ref'
drivers/dma/dmaengine.c:866 dma_async_device_register() warn: possible 
memory leak of 'idr_ref'

Anyway, though config not totally correct, it really could find some 
warning.

Thanks

Dan Carpenter Sept. 3, 2013, 8:39 p.m. UTC | #6

On Tue, Aug 27, 2013 at 10:04:48AM +0800, zhangfei wrote:
> On 13-08-20 04:13 PM, Dan Carpenter wrote:
> >On Tue, Aug 20, 2013 at 03:58:41PM +0800, zhangfei gao wrote:
> >
> >>BTW, it is strange I do not see such compile warning here :(
> >
> >This was a Smatch static checker warning, not a regular build
> >warning.
> >
> >regards,
> >dan carpenter
> >
> Dear Dan
> 
> Thanks for introducing this valuable tool.
> Have download from git://repo.or.cz/smatch.git
> 
> When trying use this tool, could get some warning,
> make CHECK="~/bin/smatch -p=kernel --no-data" C=1 | tee warns.txt
> 
> Also some strange warning, like "SQL error #2: no such table:
> caller_info", is it ignored?
> 

Oops.  I'll fix that.  The problem is that I don't normally test with
--no-data.  Sorry about that.

> Besides, when test this case for check smatch setting.
> Still not reproduce the issue, should be not config correct?

This warning requires that the cross function database be set up.

regards,
dan carpenter

Patch

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 5932ab1..755ba2f 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -514,16 +514,16 @@  struct dma_chan *dma_get_slave_channel(struct dma_chan *chan)
 	/* lock against __dma_request_channel */
 	mutex_lock(&dma_list_mutex);
 
-	if (chan->client_count == 0)
+	if (chan->client_count == 0) {
 		err = dma_chan_get(chan);
-	else
+		if (err)
+			pr_debug("%s: failed to get %s: (%d)\n",
+				__func__, dma_chan_name(chan), err);
+	} else
 		chan = NULL;
 
 	mutex_unlock(&dma_list_mutex);
 
-	if (err)
-		pr_debug("%s: failed to get %s: (%d)\n",
-			__func__, dma_chan_name(chan), err);
 
 	return chan;
 }