| Message ID | 20181012094454.22841-5-ben.dooks@codethink.co.uk |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] dma: tegra: avoid overflow of byte tracking | expand |
Hi Ben,
I love your patch! Perhaps something to improve:
[auto build test WARNING on sof-driver-fuweitax/master]
[also build test WARNING on v4.19-rc7 next-20181012]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ben-Dooks/dma-tegra-avoid-overflow-of-byte-tracking/20181013-023951
base: https://github.com/fuweitax/linux master
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:332:0,
from include/linux/kernel.h:14,
from include/linux/clk.h:16,
from drivers//dma/tegra20-apb-dma.c:20:
drivers//dma/tegra20-apb-dma.c: In function 'tegra_dma_tx_status':
include/linux/dynamic_debug.h:135:3: warning: 'done' may be used uninitialized in this function [-Wmaybe-uninitialized]
__dynamic_dev_dbg(&descriptor, dev, fmt, \
^~~~~~~~~~~~~~~~~
drivers//dma/tegra20-apb-dma.c:816:6: note: 'done' was declared here
int done;
^~~~
In file included from include/linux/printk.h:332:0,
from include/linux/kernel.h:14,
from include/linux/clk.h:16,
from drivers//dma/tegra20-apb-dma.c:20:
include/linux/dynamic_debug.h:135:3: warning: 'wcount' may be used uninitialized in this function [-Wmaybe-uninitialized]
__dynamic_dev_dbg(&descriptor, dev, fmt, \
^~~~~~~~~~~~~~~~~
drivers//dma/tegra20-apb-dma.c:811:16: note: 'wcount' was declared here
unsigned long wcount;
^~~~~~
>> drivers//dma/tegra20-apb-dma.c:843:30: warning: 'sg_req' may be used uninitialized in this function [-Wmaybe-uninitialized]
result = residual - sg_req->req_len;
~~~~~~^~~~~~~~~
drivers//dma/tegra20-apb-dma.c:894:27: note: 'sg_req' was declared here
struct tegra_dma_sg_req *sg_req;
^~~~~~
vim +/sg_req +843 drivers//dma/tegra20-apb-dma.c
804
805 static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
806 struct tegra_dma_sg_req *sg_req,
807 struct tegra_dma_desc *dma_desc,
808 unsigned int residual)
809 {
810 unsigned long status = 0x0;
811 unsigned long wcount;
812 unsigned long ahbptr;
813 unsigned long tmp = 0x0;
814 unsigned int result;
815 int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
816 int done;
817
818 /* if we're not the current request, then don't alter the residual */
819 if (sg_req != list_first_entry(&tdc->pending_sg_req,
820 struct tegra_dma_sg_req, node)) {
821 result = residual;
822 ahbptr = 0xffffffff;
823 goto done;
824 }
825
826 /* loop until we have a reliable result for residual */
827 do {
828 ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
829 status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
830 tmp = tdc_read(tdc, 0x08); /* total count for debug */
831
832 /* check status, if channel isn't busy then skip */
833 if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
834 result = residual;
835 break;
836 }
837
838 /* if we've got an interrupt pending on the channel, don't
839 * try and deal with the residue as the hardware has likely
840 * moved on to the next buffer. return all data moved.
841 */
842 if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
> 843 result = residual - sg_req->req_len;
844 break;
845 }
846
847 if (tdc->tdma->chip_data->support_separate_wcount_reg)
848 wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
849 else
850 wcount = status;
851
852 /* If the request is at the full point, then there is a
853 * chance that we have read the status register in the
854 * middle of the hardware reloading the next buffer.
855 *
856 * The sequence seems to be at the end of the buffer, to
857 * load the new word count before raising the EOC flag (or
858 * changing the ping-pong flag which could have also been
859 * used to determine a new buffer). This means there is a
860 * small window where we cannot determine zero-done for the
861 * current buffer, or moved to next buffer.
862 *
863 * If done shows 0, then retry the load, as it may hit the
864 * above hardware race. We will either get a new value which
865 * is from the first buffer, or we get an EOC (new buffer)
866 * or both a new value and an EOC...
867 */
868 done = get_current_xferred_count(tdc, sg_req, wcount);
869 if (done != 0) {
870 result = residual - done;
871 break;
872 }
873
874 ndelay(100);
875 } while (--retries > 0);
876
877 if (retries <= 0) {
878 dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
879 result = residual;
880 }
881
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c index 4f7d1e576d03..ce2888f67254 100644 --- a/drivers/dma/tegra20-apb-dma.c +++ b/drivers/dma/tegra20-apb-dma.c @@ -802,6 +802,90 @@ static int tegra_dma_terminate_all(struct dma_chan *dc) return 0; } +static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc, + struct tegra_dma_sg_req *sg_req, + struct tegra_dma_desc *dma_desc, + unsigned int residual) +{ + unsigned long status = 0x0; + unsigned long wcount; + unsigned long ahbptr; + unsigned long tmp = 0x0; + unsigned int result; + int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10; + int done; + + /* if we're not the current request, then don't alter the residual */ + if (sg_req != list_first_entry(&tdc->pending_sg_req, + struct tegra_dma_sg_req, node)) { + result = residual; + ahbptr = 0xffffffff; + goto done; + } + + /* loop until we have a reliable result for residual */ + do { + ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR); + status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS); + tmp = tdc_read(tdc, 0x08); /* total count for debug */ + + /* check status, if channel isn't busy then skip */ + if (!(status & TEGRA_APBDMA_STATUS_BUSY)) { + result = residual; + break; + } + + /* if we've got an interrupt pending on the channel, don't + * try and deal with the residue as the hardware has likely + * moved on to the next buffer. return all data moved. + */ + if (status & TEGRA_APBDMA_STATUS_ISE_EOC) { + result = residual - sg_req->req_len; + break; + } + + if (tdc->tdma->chip_data->support_separate_wcount_reg) + wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER); + else + wcount = status; + + /* If the request is at the full point, then there is a + * chance that we have read the status register in the + * middle of the hardware reloading the next buffer. + * + * The sequence seems to be at the end of the buffer, to + * load the new word count before raising the EOC flag (or + * changing the ping-pong flag which could have also been + * used to determine a new buffer). This means there is a + * small window where we cannot determine zero-done for the + * current buffer, or moved to next buffer. + * + * If done shows 0, then retry the load, as it may hit the + * above hardware race. We will either get a new value which + * is from the first buffer, or we get an EOC (new buffer) + * or both a new value and an EOC... + */ + done = get_current_xferred_count(tdc, sg_req, wcount); + if (done != 0) { + result = residual - done; + break; + } + + ndelay(100); + } while (--retries > 0); + + if (retries <= 0) { + dev_err(tdc2dev(tdc), "timeout waiting for dma load\n"); + result = residual; + } + +done: + dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n", + sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done); + + return result; +} + static enum dma_status tegra_dma_tx_status(struct dma_chan *dc, dma_cookie_t cookie, struct dma_tx_state *txstate) { @@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc, residual = dma_desc->bytes_requested - (dma_desc->bytes_transferred % dma_desc->bytes_requested); + residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual); dma_set_residue(txstate, residual); } @@ -1436,12 +1521,7 @@ static int tegra_dma_probe(struct platform_device *pdev) BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | BIT(DMA_SLAVE_BUSWIDTH_8_BYTES); tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV); - /* - * XXX The hardware appears to support - * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's - * only used by this driver during tegra_dma_terminate_all() - */ - tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT; + tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; tdma->dma_dev.device_config = tegra_dma_slave_config; tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all; tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
The tx_status callback does not report the state of the transfer beyond complete segments. This causes problems with users such as ALSA when applications want to know accurately how much data has been moved. This patch addes a function tegra_dma_update_residual() to query the hardware and modify the residual information accordinly. It takes into account any hardware issues when trying to read the state, such as delays between finishing a buffer and signalling the interrupt. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- drivers/dma/tegra20-apb-dma.c | 92 ++++++++++++++++++++++++++++++++--- 1 file changed, 86 insertions(+), 6 deletions(-)