From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54195) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brobq-0007N3-UN for qemu-devel@nongnu.org; Wed, 05 Oct 2016 11:59:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brobj-0004qn-JK for qemu-devel@nongnu.org; Wed, 05 Oct 2016 11:59:53 -0400 MIME-Version: 1.0 Sender: alistair23@gmail.com In-Reply-To: <20161004205619.GA31579@toto> References: <1475604703-3381-1-git-send-email-rutu.shah.26@gmail.com> <20161004205619.GA31579@toto> From: Alistair Francis Date: Wed, 5 Oct 2016 08:59:08 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCHv2] Reducing stack frame size in stream_process_mem2s() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: rutu.shah.26@gmail.com, Stefan Hajnoczi , qemu-arm , "qemu-devel@nongnu.org Developers" , Alistair Francis On Tue, Oct 4, 2016 at 1:56 PM, Edgar E. Iglesias wrote: > On Tue, Oct 04, 2016 at 11:41:42PM +0530, rutu.shah.26@gmail.com wrote: >> From: Rutuja Shah >> >> Hi, >> This patch allocates memory for txbuf in struct Stream rather than the stack. >> As a result, the stack frame size is reduced of stream_process_mem2s(). > > Hi Rutuja, > > A nit-pick: > The commit message is not meant to be a converstation-like message, so > normally we don't include greetings. > The expected format is to have an empty line between the message and > the SoB line. > > Here's an example: > > Allocate memory for txbuf in struct Stream rather than on the stack. > As a result, the stack frame size is reduced for stream_process_mem2s(). > > Signed-off-by: Rutuja Shah > > > Other than that, this looks good to me: > Reviewed-by: Edgar E. Iglesias I agree with Edgar, just taking out the hi and adding a new line should be enough. That can probably be done when applying it? Reviewed-by: Alistair Francis Thanks, Alistair > > >> Signed-off-by: Rutuja Shah >> --- >> hw/dma/xilinx_axidma.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c >> index b135a5f..6065689 100644 >> --- a/hw/dma/xilinx_axidma.c >> +++ b/hw/dma/xilinx_axidma.c >> @@ -111,6 +111,7 @@ struct Stream { >> unsigned int complete_cnt; >> uint32_t regs[R_MAX]; >> uint8_t app[20]; >> + unsigned char txbuf[16 * 1024]; >> }; >> >> struct XilinxAXIDMAStreamSlave { >> @@ -256,7 +257,6 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev, >> StreamSlave *tx_control_dev) >> { >> uint32_t prev_d; >> - unsigned char txbuf[16 * 1024]; >> unsigned int txlen; >> >> if (!stream_running(s) || stream_idle(s)) { >> @@ -277,17 +277,17 @@ static void stream_process_mem2s(struct Stream *s, StreamSlave *tx_data_dev, >> } >> >> txlen = s->desc.control & SDESC_CTRL_LEN_MASK; >> - if ((txlen + s->pos) > sizeof txbuf) { >> + if ((txlen + s->pos) > sizeof s->txbuf) { >> hw_error("%s: too small internal txbuf! %d\n", __func__, >> txlen + s->pos); >> } >> >> cpu_physical_memory_read(s->desc.buffer_address, >> - txbuf + s->pos, txlen); >> + s->txbuf + s->pos, txlen); >> s->pos += txlen; >> >> if (stream_desc_eof(&s->desc)) { >> - stream_push(tx_data_dev, txbuf, s->pos); >> + stream_push(tx_data_dev, s->txbuf, s->pos); >> s->pos = 0; >> stream_complete(s); >> } >> -- >> 1.9.1 >> >