| Message ID | 20180706090543.31625-3-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 06, 2018 at 11:05:42AM +0200, Geert Uytterhoeven wrote: > The transmit DMA workqueue is never stopped, hence the work function may > be called after the port has been shut down. > > Fix this race condition by cancelling queued work, if any, before DMA > release. Don't initialize the work if DMA initialization failed, as it > won't be used anyway. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v2: > - No changes. > --- > drivers/tty/serial/sh-sci.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index 674dc65454ae0684..939749073e7bdb11 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -1293,6 +1293,7 @@ static void sci_tx_dma_release(struct sci_port *s) > { > struct dma_chan *chan = s->chan_tx_saved; > > + cancel_work_sync(&s->work_tx); > s->chan_tx_saved = s->chan_tx = NULL; > s->cookie_tx = -EINVAL; > dmaengine_terminate_all(chan); > @@ -1548,10 +1549,9 @@ static void sci_request_dma(struct uart_port *port) > __func__, UART_XMIT_SIZE, > port->state->xmit.buf, &s->tx_dma_addr); > > + INIT_WORK(&s->work_tx, work_fn_tx); > s->chan_tx_saved = s->chan_tx = chan; Is it ok that work_fn_tx reads and writes s->work_tx which is set after the call to INIT_WORK() ? > } > - > - INIT_WORK(&s->work_tx, work_fn_tx); > } > > chan = sci_request_dma_chan(port, DMA_DEV_TO_MEM); > -- > 2.17.1 > -- 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 Simon, On Wed, Jul 11, 2018 at 4:52 PM Simon Horman <horms@verge.net.au> wrote: > On Fri, Jul 06, 2018 at 11:05:42AM +0200, Geert Uytterhoeven wrote: > > The transmit DMA workqueue is never stopped, hence the work function may > > be called after the port has been shut down. > > > > Fix this race condition by cancelling queued work, if any, before DMA > > release. Don't initialize the work if DMA initialization failed, as it > > won't be used anyway. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- a/drivers/tty/serial/sh-sci.c > > +++ b/drivers/tty/serial/sh-sci.c > > @@ -1293,6 +1293,7 @@ static void sci_tx_dma_release(struct sci_port *s) > > { > > struct dma_chan *chan = s->chan_tx_saved; > > > > + cancel_work_sync(&s->work_tx); > > s->chan_tx_saved = s->chan_tx = NULL; > > s->cookie_tx = -EINVAL; > > dmaengine_terminate_all(chan); > > @@ -1548,10 +1549,9 @@ static void sci_request_dma(struct uart_port *port) > > __func__, UART_XMIT_SIZE, > > port->state->xmit.buf, &s->tx_dma_addr); > > > > + INIT_WORK(&s->work_tx, work_fn_tx); > > s->chan_tx_saved = s->chan_tx = chan; > > Is it ok that work_fn_tx reads and writes s->work_tx which writes s->chan_tx? > is set after the call to INIT_WORK() ? Yes, unlike interrupt handlers, it isn't called until software kicks the workqueue using schedule_work(&s->work_tx); > > } > > - > > - INIT_WORK(&s->work_tx, work_fn_tx); > > } > > > > chan = sci_request_dma_chan(port, DMA_DEV_TO_MEM); Gr{oetje,eeting}s, Geert
On Wed, Jul 11, 2018 at 05:15:57PM +0200, Geert Uytterhoeven wrote: > Hi Simon, > > On Wed, Jul 11, 2018 at 4:52 PM Simon Horman <horms@verge.net.au> wrote: > > On Fri, Jul 06, 2018 at 11:05:42AM +0200, Geert Uytterhoeven wrote: > > > The transmit DMA workqueue is never stopped, hence the work function may > > > be called after the port has been shut down. > > > > > > Fix this race condition by cancelling queued work, if any, before DMA > > > release. Don't initialize the work if DMA initialization failed, as it > > > won't be used anyway. > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > --- a/drivers/tty/serial/sh-sci.c > > > +++ b/drivers/tty/serial/sh-sci.c > > > @@ -1293,6 +1293,7 @@ static void sci_tx_dma_release(struct sci_port *s) > > > { > > > struct dma_chan *chan = s->chan_tx_saved; > > > > > > + cancel_work_sync(&s->work_tx); > > > s->chan_tx_saved = s->chan_tx = NULL; > > > s->cookie_tx = -EINVAL; > > > dmaengine_terminate_all(chan); > > > @@ -1548,10 +1549,9 @@ static void sci_request_dma(struct uart_port *port) > > > __func__, UART_XMIT_SIZE, > > > port->state->xmit.buf, &s->tx_dma_addr); > > > > > > + INIT_WORK(&s->work_tx, work_fn_tx); > > > s->chan_tx_saved = s->chan_tx = chan; > > > > Is it ok that work_fn_tx reads and writes s->work_tx which > > writes s->chan_tx? Yes :) > > is set after the call to INIT_WORK() ? > > Yes, unlike interrupt handlers, it isn't called until software kicks the > workqueue using schedule_work(&s->work_tx); Ok, so we are sure access is only happening on one CPU? > > > > } > > > - > > > - INIT_WORK(&s->work_tx, work_fn_tx); > > > } > > > > > > chan = sci_request_dma_chan(port, DMA_DEV_TO_MEM); > > 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 Simon, On Fri, Jul 13, 2018 at 9:42 AM Simon Horman <horms@verge.net.au> wrote: > On Wed, Jul 11, 2018 at 05:15:57PM +0200, Geert Uytterhoeven wrote: > > On Wed, Jul 11, 2018 at 4:52 PM Simon Horman <horms@verge.net.au> wrote: > > > On Fri, Jul 06, 2018 at 11:05:42AM +0200, Geert Uytterhoeven wrote: > > > > The transmit DMA workqueue is never stopped, hence the work function may > > > > be called after the port has been shut down. > > > > > > > > Fix this race condition by cancelling queued work, if any, before DMA > > > > release. Don't initialize the work if DMA initialization failed, as it > > > > won't be used anyway. > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > --- a/drivers/tty/serial/sh-sci.c > > > > +++ b/drivers/tty/serial/sh-sci.c > > > > @@ -1293,6 +1293,7 @@ static void sci_tx_dma_release(struct sci_port *s) > > > > { > > > > struct dma_chan *chan = s->chan_tx_saved; > > > > > > > > + cancel_work_sync(&s->work_tx); > > > > s->chan_tx_saved = s->chan_tx = NULL; > > > > s->cookie_tx = -EINVAL; > > > > dmaengine_terminate_all(chan); > > > > @@ -1548,10 +1549,9 @@ static void sci_request_dma(struct uart_port *port) > > > > __func__, UART_XMIT_SIZE, > > > > port->state->xmit.buf, &s->tx_dma_addr); > > > > > > > > + INIT_WORK(&s->work_tx, work_fn_tx); > > > > s->chan_tx_saved = s->chan_tx = chan; > > > > > > Is it ok that work_fn_tx reads and writes s->work_tx which > > > > writes s->chan_tx? > > Yes :) > > > > is set after the call to INIT_WORK() ? > > > > Yes, unlike interrupt handlers, it isn't called until software kicks the > > workqueue using schedule_work(&s->work_tx); > > Ok, so we are sure access is only happening on one CPU? sci_request_dma() is called during early port setup, before it can be used. So no other CPU should be using it (unless it contains a time machine ;-) Gr{oetje,eeting}s, Geert
On Fri, Jul 13, 2018 at 10:00:05AM +0200, Geert Uytterhoeven wrote: > Hi Simon, > > On Fri, Jul 13, 2018 at 9:42 AM Simon Horman <horms@verge.net.au> wrote: > > On Wed, Jul 11, 2018 at 05:15:57PM +0200, Geert Uytterhoeven wrote: > > > On Wed, Jul 11, 2018 at 4:52 PM Simon Horman <horms@verge.net.au> wrote: > > > > On Fri, Jul 06, 2018 at 11:05:42AM +0200, Geert Uytterhoeven wrote: > > > > > The transmit DMA workqueue is never stopped, hence the work function may > > > > > be called after the port has been shut down. > > > > > > > > > > Fix this race condition by cancelling queued work, if any, before DMA > > > > > release. Don't initialize the work if DMA initialization failed, as it > > > > > won't be used anyway. > > > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > --- a/drivers/tty/serial/sh-sci.c > > > > > +++ b/drivers/tty/serial/sh-sci.c > > > > > @@ -1293,6 +1293,7 @@ static void sci_tx_dma_release(struct sci_port *s) > > > > > { > > > > > struct dma_chan *chan = s->chan_tx_saved; > > > > > > > > > > + cancel_work_sync(&s->work_tx); > > > > > s->chan_tx_saved = s->chan_tx = NULL; > > > > > s->cookie_tx = -EINVAL; > > > > > dmaengine_terminate_all(chan); > > > > > @@ -1548,10 +1549,9 @@ static void sci_request_dma(struct uart_port *port) > > > > > __func__, UART_XMIT_SIZE, > > > > > port->state->xmit.buf, &s->tx_dma_addr); > > > > > > > > > > + INIT_WORK(&s->work_tx, work_fn_tx); > > > > > s->chan_tx_saved = s->chan_tx = chan; > > > > > > > > Is it ok that work_fn_tx reads and writes s->work_tx which > > > > > > writes s->chan_tx? > > > > Yes :) > > > > > > is set after the call to INIT_WORK() ? > > > > > > Yes, unlike interrupt handlers, it isn't called until software kicks the > > > workqueue using schedule_work(&s->work_tx); > > > > Ok, so we are sure access is only happening on one CPU? > > sci_request_dma() is called during early port setup, before it can be used. > So no other CPU should be using it (unless it contains a time machine ;-) Thanks, that is what I was/am missing. -- 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
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 674dc65454ae0684..939749073e7bdb11 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -1293,6 +1293,7 @@ static void sci_tx_dma_release(struct sci_port *s) { struct dma_chan *chan = s->chan_tx_saved; + cancel_work_sync(&s->work_tx); s->chan_tx_saved = s->chan_tx = NULL; s->cookie_tx = -EINVAL; dmaengine_terminate_all(chan); @@ -1548,10 +1549,9 @@ static void sci_request_dma(struct uart_port *port) __func__, UART_XMIT_SIZE, port->state->xmit.buf, &s->tx_dma_addr); + INIT_WORK(&s->work_tx, work_fn_tx); s->chan_tx_saved = s->chan_tx = chan; } - - INIT_WORK(&s->work_tx, work_fn_tx); } chan = sci_request_dma_chan(port, DMA_DEV_TO_MEM);
The transmit DMA workqueue is never stopped, hence the work function may be called after the port has been shut down. Fix this race condition by cancelling queued work, if any, before DMA release. Don't initialize the work if DMA initialization failed, as it won't be used anyway. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v2: - No changes. --- drivers/tty/serial/sh-sci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)