Re: [PATCH v2] USB: serial: mos7720: defer state restore to a workqueue

2020-11-18 Thread Johan Hovold
On Tue, Nov 17, 2020 at 08:48:37AM -0800, Davidlohr Bueso wrote:
> The parallel port restore operation currently defers writes
> to a tasklet, if it sees a locked disconnect mutex. The
> driver goes to a lot of trouble to ensure writes happen
> in a non-blocking context, but things can be greatly
> simplified if it's done in regular process context and
> this is not a system performance critical path. As such,
> instead of doing the state restore writes in irq context,
> use a workqueue and just do regular synchronous writes.
> 
> In addition to the cleanup, this also imposes less on the
> overall system as tasklets have been deprecated because
> of it's BH implications, potentially blocking a higher
> priority task from running. We also get rid of hacks
> such as trylocking a mutex in irq, something which does

So this was never done in irq either. Perhaps you can remove that bit
too.

> not play nice with priority boosting in PREEMPT_RT.
> 
> Signed-off-by: Davidlohr Bueso 
> ---
> Changes from v1: remove bogus irq comment.

You seem to have some problem with your mail setup. The patch fails to
apply, and both checkpatch and git warn about the patch being corrupt:

warning: Patch sent with format=flowed; space at the end of lines might 
be lost.
Applying: USB: serial: mos7720: defer state restore to a workqueue
error: corrupt patch at line 12
Patch failed at 0001 USB: serial: mos7720: defer state restore to a 
workqueue

Can you look into that and verify that you can send the patch to
yourself and apply it first?

Johan


[PATCH v2] USB: serial: mos7720: defer state restore to a workqueue

2020-11-17 Thread Davidlohr Bueso

The parallel port restore operation currently defers writes
to a tasklet, if it sees a locked disconnect mutex. The
driver goes to a lot of trouble to ensure writes happen
in a non-blocking context, but things can be greatly
simplified if it's done in regular process context and
this is not a system performance critical path. As such,
instead of doing the state restore writes in irq context,
use a workqueue and just do regular synchronous writes.

In addition to the cleanup, this also imposes less on the
overall system as tasklets have been deprecated because
of it's BH implications, potentially blocking a higher
priority task from running. We also get rid of hacks
such as trylocking a mutex in irq, something which does
not play nice with priority boosting in PREEMPT_RT.

Signed-off-by: Davidlohr Bueso 
---
Changes from v1: remove bogus irq comment.

drivers/usb/serial/mos7720.c | 234 ++-
1 file changed, 35 insertions(+), 199 deletions(-)

diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
index 5a5d2a95070e..41ee2984a0df 100644
--- a/drivers/usb/serial/mos7720.c
+++ b/drivers/usb/serial/mos7720.c
@@ -79,14 +79,6 @@ MODULE_DEVICE_TABLE(usb, id_table);
#define DCR_INIT_VAL   0x0c /* SLCTIN, nINIT */
#define ECR_INIT_VAL   0x00 /* SPP mode */

-struct urbtracker {
-   struct mos7715_parport  *mos_parport;
-   struct list_headurblist_entry;
-   struct kref ref_count;
-   struct urb  *urb;
-   struct usb_ctrlrequest  *setup;
-};
-
enum mos7715_pp_modes {
SPP = 0<<5,
PS2 = 1<<5,  /* moschip calls this 'NIBBLE' mode */
@@ -96,12 +88,9 @@ enum mos7715_pp_modes {
struct mos7715_parport {
struct parport  *pp;   /* back to containing struct */
struct kref ref_count; /* to instance of this struct */
-   struct list_headdeferred_urbs; /* list deferred async urbs */
-   struct list_headactive_urbs;   /* list async urbs in flight */
-   spinlock_t  listlock;  /* protects list access */
boolmsg_pending;   /* usb sync call pending */
struct completion   syncmsg_compl; /* usb sync call completed */
-   struct tasklet_struct   urb_tasklet;   /* for sending deferred urbs */
+   struct work_struct  work;  /* restore deferred writes */
struct usb_serial   *serial;   /* back to containing struct */
__u8shadowECR; /* parallel port regs... */
__u8shadowDCR;
@@ -265,174 +254,8 @@ static void destroy_mos_parport(struct kref *kref)
kfree(mos_parport);
}

-static void destroy_urbtracker(struct kref *kref)
-{
-   struct urbtracker *urbtrack =
-   container_of(kref, struct urbtracker, ref_count);
-   struct mos7715_parport *mos_parport = urbtrack->mos_parport;
-
-   usb_free_urb(urbtrack->urb);
-   kfree(urbtrack->setup);
-   kfree(urbtrack);
-   kref_put(_parport->ref_count, destroy_mos_parport);
-}
-
/*
- * This runs as a tasklet when sending an urb in a non-blocking parallel
- * port callback had to be deferred because the disconnect mutex could not be
- * obtained at the time.
- */
-static void send_deferred_urbs(struct tasklet_struct *t)
-{
-   int ret_val;
-   unsigned long flags;
-   struct mos7715_parport *mos_parport = from_tasklet(mos_parport, t,
-  urb_tasklet);
-   struct urbtracker *urbtrack, *tmp;
-   struct list_head *cursor, *next;
-   struct device *dev;
-
-   /* if release function ran, game over */
-   if (unlikely(mos_parport->serial == NULL))
-   return;
-
-   dev = _parport->serial->dev->dev;
-
-   /* try again to get the mutex */
-   if (!mutex_trylock(_parport->serial->disc_mutex)) {
-   dev_dbg(dev, "%s: rescheduling tasklet\n", __func__);
-   tasklet_schedule(_parport->urb_tasklet);
-   return;
-   }
-
-   /* if device disconnected, game over */
-   if (unlikely(mos_parport->serial->disconnected)) {
-   mutex_unlock(_parport->serial->disc_mutex);
-   return;
-   }
-
-   spin_lock_irqsave(_parport->listlock, flags);
-   if (list_empty(_parport->deferred_urbs)) {
-   spin_unlock_irqrestore(_parport->listlock, flags);
-   mutex_unlock(_parport->serial->disc_mutex);
-   dev_dbg(dev, "%s: deferred_urbs list empty\n", __func__);
-   return;
-   }
-
-   /* move contents of deferred_urbs list to active_urbs list and submit */
-   list_for_each_safe(cursor, next, _parport->deferred_urbs)
-   list_move_tail(cursor, _parport->active_urbs);
-   list_for_each_entry_safe(urbtrack, tmp, _parport->active_urbs,
-   urblist_entry) {
-