| Message ID | 1432145174-11534-2-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Geert, Thank you for the patch. On Wednesday 20 May 2015 20:06:07 Geert Uytterhoeven wrote: > To function correctly in the presence of an IOMMU, the DMA buffers must > be managed using the DMA channel's device instead of the platform > device's device. > > Make sure to free the DMA memory before releasing the channel, and avoid > freeing it twice (when DMA initialization succeeded before, but failed > partially on next open). > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/tty/serial/sh-sci.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index 583aa563d4038f8b..8756d186e86891ac 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -1372,10 +1372,13 @@ static void sci_rx_dma_release(struct sci_port *s, > bool enable_pio) > > s->chan_rx = NULL; > s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL; > + if (sg_dma_address(&s->sg_rx[0])) { 0 is a valid DMA address. You should use dma_mapping_error() to test for invalid addresses. I'm afraid we don't have any API to set a DMA address to an invalid value, but we can probably use DMA_ERROR_CODE for now even if it's limited to ARM. > + dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, > + sg_virt(&s->sg_rx[0]), > + sg_dma_address(&s->sg_rx[0])); > + sg_dma_address(&s->sg_rx[0]) = 0; > + } > dma_release_channel(chan); > - if (sg_dma_address(&s->sg_rx[0])) > - dma_free_coherent(port->dev, s->buf_len_rx * 2, > - sg_virt(&s->sg_rx[0]), sg_dma_address(&s->sg_rx[0])); > if (enable_pio) > sci_start_rx(port); > } > @@ -1706,7 +1709,8 @@ static void sci_request_dma(struct uart_port *port) > sg_set_page(&s->sg_tx, virt_to_page(port->state->xmit.buf), > UART_XMIT_SIZE, > (uintptr_t)port->state->xmit.buf & ~PAGE_MASK); > - nent = dma_map_sg(port->dev, &s->sg_tx, 1, DMA_TO_DEVICE); > + nent = dma_map_sg(chan->device->dev, &s->sg_tx, 1, > + DMA_TO_DEVICE); > if (!nent) > sci_tx_dma_release(s, false); > else > @@ -1735,8 +1739,9 @@ static void sci_request_dma(struct uart_port *port) > s->chan_rx = chan; > > s->buf_len_rx = 2 * max(16, (int)port->fifosize); > - buf[0] = dma_alloc_coherent(port->dev, s->buf_len_rx * 2, > - &dma[0], GFP_KERNEL); > + buf[0] = dma_alloc_coherent(chan->device->dev, > + s->buf_len_rx * 2, &dma[0], > + GFP_KERNEL); I might be mistaken but I think this will break with the shdma-mux infrastructure, as the IOMMUs will be associated with the physical DMA controller and not the mux device. Maybe this just calls for getting rid of shdma-mux. I'm knowingly forgetting about the other option that would require fixing the problem in the DMA mapping implementation :-) Those issues aren't introduced by this patch, so you can add my Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> here and fix them later. > > if (!buf[0]) { > dev_warn(port->dev,
On Sat, May 23, 2015 at 9:01 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wednesday 20 May 2015 20:06:07 Geert Uytterhoeven wrote: >> To function correctly in the presence of an IOMMU, the DMA buffers must >> be managed using the DMA channel's device instead of the platform >> device's device. >> >> Make sure to free the DMA memory before releasing the channel, and avoid >> freeing it twice (when DMA initialization succeeded before, but failed >> partially on next open). >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> drivers/tty/serial/sh-sci.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c >> index 583aa563d4038f8b..8756d186e86891ac 100644 >> --- a/drivers/tty/serial/sh-sci.c >> +++ b/drivers/tty/serial/sh-sci.c >> @@ -1372,10 +1372,13 @@ static void sci_rx_dma_release(struct sci_port *s, >> bool enable_pio) >> >> s->chan_rx = NULL; >> s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL; >> + if (sg_dma_address(&s->sg_rx[0])) { > > 0 is a valid DMA address. You should use dma_mapping_error() to test for 0 is not a valid DMA addresss on sh... > invalid addresses. I'm afraid we don't have any API to set a DMA address to an > invalid value, but we can probably use DMA_ERROR_CODE for now even if it's > limited to ARM. There's no DMA_ERROR_CODE on sh. Furthermore, while dma_mapping_error() on ARM checks for DMA_ERROR_CODE (~0), sh checks for zero... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Wednesday 15 July 2015 13:37:36 Geert Uytterhoeven wrote: > On Sat, May 23, 2015 at 9:01 PM, Laurent Pinchart wrote: > > On Wednesday 20 May 2015 20:06:07 Geert Uytterhoeven wrote: > >> To function correctly in the presence of an IOMMU, the DMA buffers must > >> be managed using the DMA channel's device instead of the platform > >> device's device. > >> > >> Make sure to free the DMA memory before releasing the channel, and avoid > >> freeing it twice (when DMA initialization succeeded before, but failed > >> partially on next open). > >> > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > >> --- > >> > >> drivers/tty/serial/sh-sci.c | 17 +++++++++++------ > >> 1 file changed, 11 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > >> index 583aa563d4038f8b..8756d186e86891ac 100644 > >> --- a/drivers/tty/serial/sh-sci.c > >> +++ b/drivers/tty/serial/sh-sci.c > >> @@ -1372,10 +1372,13 @@ static void sci_rx_dma_release(struct sci_port > >> *s, bool enable_pio) > >> > >> s->chan_rx = NULL; > >> s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL; > >> > >> + if (sg_dma_address(&s->sg_rx[0])) { > > > > 0 is a valid DMA address. You should use dma_mapping_error() to test for > > 0 is not a valid DMA addresss on sh... It is on ARM though. > > invalid addresses. I'm afraid we don't have any API to set a DMA address > > to an invalid value, but we can probably use DMA_ERROR_CODE for now even > > if it's limited to ARM. > > There's no DMA_ERROR_CODE on sh. > > Furthermore, while dma_mapping_error() on ARM checks for DMA_ERROR_CODE > (~0), sh checks for zero... That's why drivers are supposed to use dma_mapping_error(). The problem is the lack of an API to set a DMA address to an invalid value in an architecture- independent way.
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 583aa563d4038f8b..8756d186e86891ac 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -1372,10 +1372,13 @@ static void sci_rx_dma_release(struct sci_port *s, bool enable_pio) s->chan_rx = NULL; s->cookie_rx[0] = s->cookie_rx[1] = -EINVAL; + if (sg_dma_address(&s->sg_rx[0])) { + dma_free_coherent(chan->device->dev, s->buf_len_rx * 2, + sg_virt(&s->sg_rx[0]), + sg_dma_address(&s->sg_rx[0])); + sg_dma_address(&s->sg_rx[0]) = 0; + } dma_release_channel(chan); - if (sg_dma_address(&s->sg_rx[0])) - dma_free_coherent(port->dev, s->buf_len_rx * 2, - sg_virt(&s->sg_rx[0]), sg_dma_address(&s->sg_rx[0])); if (enable_pio) sci_start_rx(port); } @@ -1706,7 +1709,8 @@ static void sci_request_dma(struct uart_port *port) sg_set_page(&s->sg_tx, virt_to_page(port->state->xmit.buf), UART_XMIT_SIZE, (uintptr_t)port->state->xmit.buf & ~PAGE_MASK); - nent = dma_map_sg(port->dev, &s->sg_tx, 1, DMA_TO_DEVICE); + nent = dma_map_sg(chan->device->dev, &s->sg_tx, 1, + DMA_TO_DEVICE); if (!nent) sci_tx_dma_release(s, false); else @@ -1735,8 +1739,9 @@ static void sci_request_dma(struct uart_port *port) s->chan_rx = chan; s->buf_len_rx = 2 * max(16, (int)port->fifosize); - buf[0] = dma_alloc_coherent(port->dev, s->buf_len_rx * 2, - &dma[0], GFP_KERNEL); + buf[0] = dma_alloc_coherent(chan->device->dev, + s->buf_len_rx * 2, &dma[0], + GFP_KERNEL); if (!buf[0]) { dev_warn(port->dev,
To function correctly in the presence of an IOMMU, the DMA buffers must be managed using the DMA channel's device instead of the platform device's device. Make sure to free the DMA memory before releasing the channel, and avoid freeing it twice (when DMA initialization succeeded before, but failed partially on next open). Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/tty/serial/sh-sci.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)