linux-kernel - Re: [PATCH 2/5] dmaengine: at_hdmac: add cyclic DMA operation support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <4D9C8A9D.9070008@atmel.com>
Date:	Wed, 06 Apr 2011 17:45:33 +0200
From:	Nicolas Ferre <nicolas.ferre@...el.com>
To:	"Koul, Vinod" <vinod.koul@...el.com>
CC:	dan.j.williams@...el.com, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] dmaengine: at_hdmac: add cyclic DMA operation support

Le 06/04/2011 16:02, Koul, Vinod :
> On Wed, 2011-04-06 at 14:50 +0200, Nicolas Ferre wrote:
>> Le 06/04/2011 12:08, Koul, Vinod :
>>> On Fri, 2011-04-01 at 19:19 +0200, Nicolas Ferre wrote:
>>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@...el.com>
>>>> ---
>>>>  drivers/dma/at_hdmac.c      |  188 +++++++++++++++++++++++++++++++++++++++---
>>>>  drivers/dma/at_hdmac_regs.h |   14 +++-
>>>>  2 files changed, 186 insertions(+), 16 deletions(-)
>>>>
>>>> +static void atc_handle_cyclic(struct at_dma_chan *atchan)
>>>> +{
>>>> +	struct at_desc			*first = atc_first_active(atchan);
>>>> +	struct dma_async_tx_descriptor	*txd = &first->txd;
>>>> +	dma_async_tx_callback		callback = txd->callback;
>>>> +	void				*param = txd->callback_param;
>>>> +
>>>> +	dev_vdbg(chan2dev(&atchan->chan_common),
>>>> +			"new cyclic period llp 0x%08x\n",
>>>> +			channel_readl(atchan, DSCR));
>>>> +
>>>> +	if (callback)
>>>> +		callback(param);
>>>> +}
>>> You dont seem to be doing much expect calling callback, so doesn't it
>>> make sense to write so much code for just calling callback? 
>>
>> I do not totally follow you here: you mean that I should reduce the
>> amount of variables in this function or is it a more global comment
>> about my way of handling cyclic operations?
> Well, in handling of cyclic operation you are only calling the callback
> if set. If you plan to add more functionality to this function in future
> then okay, otherwise you may reconsider this and avoid the function as
> you are not doing much here, if it reduces code size. This is okay this
> way also.
> I had no comment on variables as I understand first two are unavoidable

As far as code size and execution path is concerned, this function is
expanded by the compiler as an inline one and I checked that it only
takes a few lines of assembly (without function call) if the debug trace
is not requested (~7 lines into atc_tasklet() with one call to
atc_first_active() which is needed anyway).

I would prefer to keep this simple "pseudo-inline" function just for a
matter of code clarity.

Best regards,
-- 
Nicolas Ferre

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/