From mboxrd@z Thu Jan  1 00:00:00 1970
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Wed, 11 Jul 2018 15:15:57 +0000
Subject: Re: [PATCH v2 2/3] serial: sh-sci: Stop TX DMA workqueue during port shutdown
Message-Id: <CAMuHMdUyhFT=tRp3_9ACWruLCDTLtfmQHyYDsZOu4xCieE++Lw@mail.gmail.com>
List-Id: <linux-sh.vger.kernel.org>
References: <20180706090543.31625-1-geert+renesas@glider.be>
 <20180706090543.31625-3-geert+renesas@glider.be> <20180711145149.muwetfr2mzbd5dc4@verge.net.au>
In-Reply-To: <20180711145149.muwetfr2mzbd5dc4@verge.net.au>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: Simon Horman <horms@verge.net.au>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>, Greg KH <gregkh@linuxfoundation.org>, Jiri Slaby <jslaby@suse.com>, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>, Ulrich Hecht <ulrich.hecht+renesas@gmail.com>, Wolfram Sang <wsa+renesas@sang-engineering.com>, "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>, Linux-Renesas <linux-renesas-soc@vger.kernel.org>, Linux-sh list <linux-sh@vger.kernel.org>

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

-- 
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