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

From: Davidlohr Bueso
Date: Wed Nov 04 2020 - 19:35:03 EST


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().

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.

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?

Thanks,
Davidlohr