Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760704AbcLPKN6 (ORCPT ); Fri, 16 Dec 2016 05:13:58 -0500 Received: from smtprelay.synopsys.com ([198.182.60.111]:56528 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759680AbcLPKNt (ORCPT ); Fri, 16 Dec 2016 05:13:49 -0500 Subject: Re: [PATCH 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma To: Appana Durga Kedareswara Rao , Jose Abreu , "dan.j.williams@intel.com" , "vinod.koul@intel.com" , "michal.simek@xilinx.com" , Soren Brinkmann , "moritz.fischer@ettus.com" , "laurent.pinchart@ideasonboard.com" , "luis@debethencourt.com" , Anirudha Sarangi References: <1481814682-31780-1-git-send-email-appanad@xilinx.com> <1481814682-31780-3-git-send-email-appanad@xilinx.com> <9d92984b-e04a-cd29-e933-d8ea4d610c94@synopsys.com> CC: "dmaengine@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" From: Jose Abreu Message-ID: <689b1077-6ee3-60d0-1fdf-0a125003a479@synopsys.com> Date: Fri, 16 Dec 2016 10:11:30 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.107.19.46] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3914 Lines: 124 Hi Kedar, On 15-12-2016 19:09, Appana Durga Kedareswara Rao wrote: > Hi Jose Miguel Abreu, > > Thanks for the review.... > >>> - last = segment; >>> + for (j = 0; j < chan->num_frms; ) { >>> + list_for_each_entry(segment, &desc->segments, node) >> { >>> + if (chan->ext_addr) >>> + vdma_desc_write_64(chan, >>> + >> XILINX_VDMA_REG_START_ADDRESS_64(i++), >>> + segment->hw.buf_addr, >>> + segment->hw.buf_addr_msb); >>> + else >>> + vdma_desc_write(chan, >>> + >> XILINX_VDMA_REG_START_ADDRESS(i++), >>> + segment->hw.buf_addr); >>> + >>> + last = segment; >> Hmm, is it possible to submit more than one segment? If so, then i and j will get >> out of sync. > If h/w is configured for more than 1 frame buffer and user submits more than one frame buffer > We can submit more than one frame/ segment to hw right?? I'm not sure. When I used VDMA driver I always submitted only one segment and multiple descriptors. But the problem is, for example: If you have: descriptor1 (2 segments) descriptor2 (2 segments) And you have 3 frame buffers in the HW. Then: 1st frame buffer will have: descriptor1 -> segment1 2nd frame buffer will have: descriptor1 -> segment2 3rd frame buffer will have: descriptor2 -> segment1 but, 4th frame buffer will have: descriptor2 -> segment2 <---- INVALID because there is only 3 frame buffers So, maybe a check inside the loop "list_for_each_entry(segment, &desc->segments, node)" could be a nice to have. > >>> + } >>> + list_del(&desc->node); >>> + list_add_tail(&desc->node, &chan->active_list); >>> + j++; >> But if i is non zero and pending_list has more than num_frms then i will not >> wrap-around as it should and will write to invalid framebuffer location, right? > Yep will fix in v2... > > If (if (list_empty(&chan->pending_list)) || (i == chan->num_frms) > break; > > Above condition is sufficient right??? Looks ok. > >>> + if (list_empty(&chan->pending_list)) >>> + break; >>> + desc = list_first_entry(&chan->pending_list, >>> + struct >> xilinx_dma_tx_descriptor, >>> + node); >>> } >>> >>> if (!last) >>> @@ -1114,14 +1124,13 @@ static void xilinx_vdma_start_transfer(struct >> xilinx_dma_chan *chan) >>> vdma_desc_write(chan, XILINX_DMA_REG_FRMDLY_STRIDE, >>> last->hw.stride); >>> vdma_desc_write(chan, XILINX_DMA_REG_VSIZE, last- >>> hw.vsize); >> Maybe a check that all framebuffers contain valid addresses should be done >> before programming vsize so that VDMA does not try to write to invalid >> addresses. > Do we really need to check for valid address??? > I didn't get you what to do you mean by invalid address could you please explain??? > In the driver we are reading form the pending_list which will be updated by pep_interleaved_dma > Call so we are under assumption that user sends the proper address right??? What I mean by valid address is to check that i variable has already been incremented by num_frms at least once since a VDMA reset. This way you know that you have programmed all the addresses of the frame buffers with an address and they are non-zero. Best regards, Jose Miguel Abreu > >>> + >>> + chan->desc_submitcount += j; >>> + chan->desc_pendingcount -= j; >>> } >>> >>> chan->idle = false; >>> if (!chan->has_sg) { >>> - list_del(&desc->node); >>> - list_add_tail(&desc->node, &chan->active_list); >>> - chan->desc_submitcount++; >>> - chan->desc_pendingcount--; >>> if (chan->desc_submitcount == chan->num_frms) >>> chan->desc_submitcount = 0; >> "desc_submitcount >= chan->num_frms would be safer here. > Sure will fix in v2... > > Regards, > Kedar. > >>> } else { >> Best regards, >> Jose Miguel Abreu >> -- >> To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body >> of a message to majordomo@vger.kernel.org More majordomo info at >> http://vger.kernel.org/majordomo-info.html