Re: çå: Re: çå: Re: [PATCH] tty: max3100: Fix oops while 'cat/proc/tty/driver/ttyMAX'

[Jiri Slaby]

On 09/15/2018, 04:14 AM, chen.lin5@xxxxxxxxxx wrote:
> yes, creationÂandÂdestroyÂofÂtheÂworkqueue is not locked, I think
> maybeÂthere is some
> 
> remainder work to doÂinÂdestroy-wq, so I cannot sure if there is
> anyÂusage aboutÂlockÂdestroy-wq.
> 
> 
> What you worried of the races is about this Â? Â
> 
> Â Â-->ÂwhenÂmax3100_shutdown, Âdestroy_workqueue is doing,Âs->workqueue
> is not NULL, at this moment,Âget_mctrl is executed, destroyingÂwq is
> queued again.
> 
> bu this cannot happen, becasueÂs->force_end_work = 1
> beforeÂdestroy_workqueue , soÂmax3100_dowork do nothing.

Oh, so this relies on flush_workqueue or destroy_workqueue to be a
barrier (so that the assignment to end_work is not reordered), correct?

> static void max3100_shutdown(struct uart_port *port)
> 
> {
> 
> ...
> 
> s->force_end_work = 1;
> 
> 
> if (s->poll_time > 0)
> 
> del_timer_sync(&s->timer);
> 
> 
> if (s->workqueue) {
> 
> flush_workqueue(s->workqueue);
> 
> destroy_workqueue(s->workqueue);
> 
> s->workqueue = NULL;
> 
> }
> 
> 
> 
> static void max3100_dowork(struct max3100_port *s)
> 
> {
> 
> if (!s->force_end_work && !freezing(current) && !s->suspending)
> 
> queue_work(s->workqueue, &s->work);
> 
> }

Also, on the first open:
        s->force_end_work = 0;
        s->parity = 0;
        s->rts = 0;

        sprintf(b, "max3100-%d", s->minor);
        s->workqueue = create_freezable_workqueue(b);
        if (!s->workqueue) {
                dev_warn(&s->spi->dev, "cannot create workqueue\n");
                return -EBUSY;
        }

Here, s->force_end_work is 0, s->workqueue is non-NULL, but s->work is
garbage until:

        INIT_WORK(&s->work, max3100_work);


INIT_WORK should be in max3100_probe as far as I can see (or at least
before create_freezable_workqueue), right? But those variable
assignments also rely on some implicit barrier which is not there.

thanks,
-- 
js
suse labs