Message ID | 20181114101342.7199-2-ben.dooks@codethink.co.uk |
---|---|
State | Superseded |
Headers | show |
Series | [1/3] dma: tegra: avoid overflow of byte tracking | expand |
On 14.11.2018 13:13, Ben Dooks wrote: > The dma_desc->bytes_transferred counter tracks the number of bytes > moved by the DMA channel. This is then used to calculate the information > passed back in the in the tegra_dma_tx_status callback, which is usually > fine. > > When the DMA channel is configured as continous, then the bytes_transferred > counter will increase over time and eventually overflow to become negative > so the residue count will become invalid and the ALSA sound-dma code will > report invalid hardware pointer values to the application. This results in > some users becoming confused about the playout position and putting audio > data in the wrong place. > > To fix this issue, always ensure the bytes_transferred field is modulo the > size of the request. We only do this for the case of the cyclic transfer > done ISR as anyone attempting to move 2GiB of DMA data in one transfer > is unlikely. > > Note, we don't fix the issue that we should /never/ transfer a negative > number of bytes so we could make those fields unsigned. > > Reviewed-by: Dmitry Osipenko <digetx@gmail.com> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > drivers/dma/tegra20-apb-dma.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > index 9a558e30c461..8219ab88a507 100644 > --- a/drivers/dma/tegra20-apb-dma.c > +++ b/drivers/dma/tegra20-apb-dma.c > @@ -636,7 +636,10 @@ static void handle_cont_sngl_cycle_dma_done(struct tegra_dma_channel *tdc, > > sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq), node); > dma_desc = sgreq->dma_desc; > - dma_desc->bytes_transferred += sgreq->req_len; > + /* if we dma for long enough the transfer count will wrap */ > + dma_desc->bytes_transferred = > + (dma_desc->bytes_transferred + sgreq->req_len) % > + dma_desc->bytes_requested; > > /* Callback need to be call */ > if (!dma_desc->cb_count) > I also actually tested that audio playback breaks after the overflow and this patch fixes it. Tested-by: Dmitry Osipenko <digetx@gmail.com>
On 2018-11-14 12:12, Dmitry Osipenko wrote: > On 14.11.2018 13:13, Ben Dooks wrote: >> The dma_desc->bytes_transferred counter tracks the number of bytes >> moved by the DMA channel. This is then used to calculate the >> information >> passed back in the in the tegra_dma_tx_status callback, which is >> usually >> fine. >> >> When the DMA channel is configured as continous, then the >> bytes_transferred >> counter will increase over time and eventually overflow to become >> negative >> so the residue count will become invalid and the ALSA sound-dma code >> will >> report invalid hardware pointer values to the application. This >> results in >> some users becoming confused about the playout position and putting >> audio >> data in the wrong place. >> >> To fix this issue, always ensure the bytes_transferred field is modulo >> the >> size of the request. We only do this for the case of the cyclic >> transfer >> done ISR as anyone attempting to move 2GiB of DMA data in one transfer >> is unlikely. >> >> Note, we don't fix the issue that we should /never/ transfer a >> negative >> number of bytes so we could make those fields unsigned. >> >> Reviewed-by: Dmitry Osipenko <digetx@gmail.com> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> --- >> drivers/dma/tegra20-apb-dma.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/dma/tegra20-apb-dma.c >> b/drivers/dma/tegra20-apb-dma.c >> index 9a558e30c461..8219ab88a507 100644 >> --- a/drivers/dma/tegra20-apb-dma.c >> +++ b/drivers/dma/tegra20-apb-dma.c >> @@ -636,7 +636,10 @@ static void >> handle_cont_sngl_cycle_dma_done(struct tegra_dma_channel *tdc, >> >> sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq), >> node); >> dma_desc = sgreq->dma_desc; >> - dma_desc->bytes_transferred += sgreq->req_len; >> + /* if we dma for long enough the transfer count will wrap */ >> + dma_desc->bytes_transferred = >> + (dma_desc->bytes_transferred + sgreq->req_len) % >> + dma_desc->bytes_requested; >> >> /* Callback need to be call */ >> if (!dma_desc->cb_count) >> > > I also actually tested that audio playback breaks after the overflow > and this patch fixes it. Thanks, I should have posted a link to a test-patch I had a while ago. > Tested-by: Dmitry Osipenko <digetx@gmail.com>
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c index 9a558e30c461..8219ab88a507 100644 --- a/drivers/dma/tegra20-apb-dma.c +++ b/drivers/dma/tegra20-apb-dma.c @@ -636,7 +636,10 @@ static void handle_cont_sngl_cycle_dma_done(struct tegra_dma_channel *tdc, sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq), node); dma_desc = sgreq->dma_desc; - dma_desc->bytes_transferred += sgreq->req_len; + /* if we dma for long enough the transfer count will wrap */ + dma_desc->bytes_transferred = + (dma_desc->bytes_transferred + sgreq->req_len) % + dma_desc->bytes_requested; /* Callback need to be call */ if (!dma_desc->cb_count)