netdev - Re: [PATCH 2/2] net: davinci_cpdma: reduce time holding chan->lock in cpdma_chan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <66d886a4-4852-096c-a3b3-3faec3eb167f@ti.com>
Date:	Tue, 26 Jul 2016 17:25:58 +0300
From:	Grygorii Strashko <grygorii.strashko@...com>
To:	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Mugunthan V N <mugunthanvnm@...com>
CC:	<linux-omap@...r.kernel.org>, <netdev@...r.kernel.org>,
	<kernel@...gutronix.de>
Subject: Re: [PATCH 2/2] net: davinci_cpdma: reduce time holding chan->lock in
 cpdma_chan_submit

On 07/26/2016 03:02 PM, Uwe Kleine-König wrote:
> Allocating and preparing a dma descriptor doesn't need to happen under
> the channel's lock. So do this before taking the channel's lock. The only
> down side is that the dma descriptor might be allocated even though the
> channel is about to be stopped. This is unlikely though.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> ---
>  drivers/net/ethernet/ti/davinci_cpdma.c | 38 +++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index 5ffa04a306c6..ba3462707ae3 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -542,24 +542,10 @@ int cpdma_chan_submit(struct cpdma_chan *chan, void *token, void *data,
>  	u32				mode;
>  	int				ret = 0;
>  
> -	spin_lock_irqsave(&chan->lock, flags);
> -
> -	if (chan->state == CPDMA_STATE_TEARDOWN) {
> -		ret = -EINVAL;
> -		goto unlock_ret;
> -	}
> -
> -	if (chan->count >= chan->desc_num)	{
> -		chan->stats.desc_alloc_fail++;
> -		ret = -ENOMEM;
> -		goto unlock_ret;
> -	}

I'm not sure this is right thing to do. This check is expected to be strict
and means "channel has exhausted the available descriptors, so further descs allocation does not allowed".


This also might affect on Ivan's work [1] "[PATCH 0/4]  net: ethernet: ti: cpsw: add multi-queue support"



[1] https://lkml.org/lkml/2016/6/30/603
-- 
regards,
-grygorii