From: Appana Durga Kedareswara Rao <appana.durga.rao@xilinx.com> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: "dan.j.williams@intel.com" <dan.j.williams@intel.com>, "vinod.koul@intel.com" <vinod.koul@intel.com>, "michal.simek@xilinx.com" <michal.simek@xilinx.com>, Soren Brinkmann <sorenb@xilinx.com>, "moritz.fischer@ettus.com" <moritz.fischer@ettus.com>, "luis@debethencourt.com" <luis@debethencourt.com>, "Jose.Abreu@synopsys.com" <Jose.Abreu@synopsys.com>, "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: RE: [PATCH 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma Date: Mon, 19 Dec 2016 15:41:25 +0000 [thread overview] Message-ID: <C246CAC1457055469EF09E3A7AC4E11A4A65D5AC@XAP-PVEXMBX01.xlnx.xilinx.com> (raw) In-Reply-To: <30884836.ckISXSrEvA@avalon> Hi Laurent Pinchart, Thanks for the review... > > + int i = 0, j = 0; > > > > if (chan->desc_submitcount < chan->num_frms) > > i = chan->desc_submitcount; > > I don't get this. i seems to index into a segment start address array, but gets > initialized with a variable documented as "Descriptor h/w submitted count". I'm > not familiar with the hardware, but it makes no sense to me. Here i is the h/w buffer address. For ex: If the h/w is configured for 3 frame buffers and user submits 4 desc's Then we need to submit only 3 frame buffers to the h/w and the next desc will be submitted After there is a room for buffers I mean when the free buffer is available. > > > - 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; > > Isn't it an issue to write the descriptors only after calling > xilinx_dma_start() ? Until writing to the VSIZE h/w won't get started... > > > + 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); > > I assume the size of the start address array to be limited by the hardware, but I > don't see how this code prevents from overflowing this. > > The whole function is very difficult to understand, it probably requires a rewrite. Will fix it in v2... > > > + last = segment; > > + } > > + list_del(&desc->node); > > + list_add_tail(&desc->node, &chan->active_list); > > + j++; > > + if (list_empty(&chan->pending_list)) > > + break; > > + desc = list_first_entry(&chan->pending_list, > > + struct > xilinx_dma_tx_descriptor, > > + node); > > } > > 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; > > } else { > > While at it, can you merge this into the previous if (chan->has_sg) { ... } else { ... } > ? Having them separate is confusing. Ok sure will fix in v2... Regards, Kedar. > > > -- > Regards, > > Laurent Pinchart
WARNING: multiple messages have this Message-ID (diff)
From: appana.durga.rao@xilinx.com (Appana Durga Kedareswara Rao) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma Date: Mon, 19 Dec 2016 15:41:25 +0000 [thread overview] Message-ID: <C246CAC1457055469EF09E3A7AC4E11A4A65D5AC@XAP-PVEXMBX01.xlnx.xilinx.com> (raw) In-Reply-To: <30884836.ckISXSrEvA@avalon> Hi Laurent Pinchart, Thanks for the review... > > + int i = 0, j = 0; > > > > if (chan->desc_submitcount < chan->num_frms) > > i = chan->desc_submitcount; > > I don't get this. i seems to index into a segment start address array, but gets > initialized with a variable documented as "Descriptor h/w submitted count". I'm > not familiar with the hardware, but it makes no sense to me. Here i is the h/w buffer address. For ex: If the h/w is configured for 3 frame buffers and user submits 4 desc's Then we need to submit only 3 frame buffers to the h/w and the next desc will be submitted After there is a room for buffers I mean when the free buffer is available. > > > - 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; > > Isn't it an issue to write the descriptors only after calling > xilinx_dma_start() ? Until writing to the VSIZE h/w won't get started... > > > + 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); > > I assume the size of the start address array to be limited by the hardware, but I > don't see how this code prevents from overflowing this. > > The whole function is very difficult to understand, it probably requires a rewrite. Will fix it in v2... > > > + last = segment; > > + } > > + list_del(&desc->node); > > + list_add_tail(&desc->node, &chan->active_list); > > + j++; > > + if (list_empty(&chan->pending_list)) > > + break; > > + desc = list_first_entry(&chan->pending_list, > > + struct > xilinx_dma_tx_descriptor, > > + node); > > } > > 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; > > } else { > > While at it, can you merge this into the previous if (chan->has_sg) { ... } else { ... } > ? Having them separate is confusing. Ok sure will fix in v2... Regards, Kedar. > > > -- > Regards, > > Laurent Pinchart
next prev parent reply other threads:[~2016-12-19 15:41 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-12-15 15:11 [PATCH 0/3] dmaengine: xilinx_dma: Bug fixes Kedareswara rao Appana 2016-12-15 15:11 ` Kedareswara rao Appana 2016-12-15 15:11 ` [PATCH 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor Kedareswara rao Appana 2016-12-15 15:11 ` Kedareswara rao Appana 2016-12-15 15:45 ` Jose Abreu 2016-12-15 15:45 ` Jose Abreu 2016-12-15 18:49 ` Appana Durga Kedareswara Rao 2016-12-15 18:49 ` Appana Durga Kedareswara Rao 2016-12-16 15:35 ` Laurent Pinchart 2016-12-16 15:35 ` Laurent Pinchart 2016-12-19 15:39 ` Appana Durga Kedareswara Rao 2016-12-19 15:39 ` Appana Durga Kedareswara Rao 2016-12-19 17:18 ` Laurent Pinchart 2016-12-19 17:18 ` Laurent Pinchart 2016-12-23 8:49 ` Appana Durga Kedareswara Rao 2016-12-23 8:49 ` Appana Durga Kedareswara Rao 2016-12-15 15:11 ` [PATCH 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma Kedareswara rao Appana 2016-12-15 15:11 ` Kedareswara rao Appana 2016-12-15 16:10 ` Jose Abreu 2016-12-15 16:10 ` Jose Abreu 2016-12-15 19:09 ` Appana Durga Kedareswara Rao 2016-12-15 19:09 ` Appana Durga Kedareswara Rao 2016-12-16 10:11 ` Jose Abreu 2016-12-16 10:11 ` Jose Abreu 2016-12-19 15:40 ` Appana Durga Kedareswara Rao 2016-12-19 15:40 ` Appana Durga Kedareswara Rao 2016-12-16 15:54 ` Laurent Pinchart 2016-12-16 15:54 ` Laurent Pinchart 2016-12-19 15:41 ` Appana Durga Kedareswara Rao [this message] 2016-12-19 15:41 ` Appana Durga Kedareswara Rao 2016-12-15 15:11 ` [PATCH 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario Kedareswara rao Appana 2016-12-15 15:11 ` Kedareswara rao Appana
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=C246CAC1457055469EF09E3A7AC4E11A4A65D5AC@XAP-PVEXMBX01.xlnx.xilinx.com \ --to=appana.durga.rao@xilinx.com \ --cc=Jose.Abreu@synopsys.com \ --cc=dan.j.williams@intel.com \ --cc=dmaengine@vger.kernel.org \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luis@debethencourt.com \ --cc=michal.simek@xilinx.com \ --cc=moritz.fischer@ettus.com \ --cc=sorenb@xilinx.com \ --cc=vinod.koul@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.