Re: [PATCH] usb/mos7720: process deferred urbs in a workqueue

[Johan Hovold <johan@kernel.org>]

On Wed, Nov 04, 2020 at 04:13:07PM -0800, Davidlohr Bueso wrote:
> On Wed, 04 Nov 2020, Johan Hovold wrote:
> 
> >Hmm. I took at closer look at the parport code and it seems the current
> >implementation is already racy but that removing the tasklet is going to
> >widen that that window.
> >
> >Those register writes in restore() should be submitted before any
> >later requests. Perhaps setting a flag and flushing the work in
> >parport_prologue() could work?
> 
> Ah, I see and agree. Considering work is only deferred from restore_state()
> I don't even think we need a flag, no? We can let parport_prologue()
> just flush_work() unconditionally (right before taking the disc_mutex)
> which for the most part will be idle anyway. The flush_work() also becomes
> saner now that we'll stop rescheduling work in send_deferred_urbs().

A flag isn't strictly needed, no, but it could be used to avoid some of
the flush_work() overhead for every parport callback. The restore-state
work will typically only be queued once.
 
> Also, but not strictly related to this. What do you think of deferring all
> work in write_parport_reg_nonblock() unconditionally? I'd like to avoid
> that mutex_trylock() because eventually I'll be re-adding a warn in the
> locking code, but that would also simplify the code done here in the
> nonblocking irq write. I'm not at all familiar with parport, but I would
> think that restore_state context would not care.

Sounds good to me. As long as the state is restored before submitting
further requests we should be fine. That would even allow getting rid of
write_parport_reg_nonblock() as you can restore the state using
synchronous calls from the worker thread. Should simplify things quite a
bit.

> >On the other hand, the restore() implementation looks broken in that it
> >doesn't actually restore the provided state. I'll go fix that up.
> 
> How did this thing ever work?

The shadow registers are initialised at probe so as long as you don't
switch to a different parallel-port driver without disconnecting the
mos7715 in between it works.

Johan