Re: [PATCH 4/4] cdc-acm: clear halt condition

[Oliver Neukum]

On Mon, 2016-11-07 at 11:50 +0100, Ladislav Michl wrote:
> On Mon, Nov 07, 2016 at 11:26:10AM +0100, Oliver Neukum wrote:
> > This is looking good. Please resend so Greg can pick it up.
> 
> One more question. I rather point on it explicitely as it changes
> driver behaviour. Previously, urb was submitted again only on
> status == 0. This caused driver to run out of urbs on error.
> That was cause of my original problem. Now driver is resubmitting
> urbs on error, except status is -ECONNRESET, -ENOENT or -ESHUTDOWN;
> for status -EPIPE halt is cleared. Does this change look reasonable
> to you?

Yes, it is a vast improvement.

Thanks
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cdc-acm: clear halt condition

[Ladislav Michl]

On Mon, Nov 07, 2016 at 11:26:10AM +0100, Oliver Neukum wrote:
> This is looking good. Please resend so Greg can pick it up.

One more question. I rather point on it explicitely as it changes
driver behaviour. Previously, urb was submitted again only on
status == 0. This caused driver to run out of urbs on error.
That was cause of my original problem. Now driver is resubmitting
urbs on error, except status is -ECONNRESET, -ENOENT or -ESHUTDOWN;
for status -EPIPE halt is cleared. Does this change look reasonable
to you?

Regards,
ladis
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cdc-acm: clear halt condition

[Oliver Neukum]

On Sun, 2016-11-06 at 23:31 +0100, Ladislav Michl wrote:
> On Sun, Nov 06, 2016 at 10:30:02PM +0100, Oliver Neukum wrote:
> > Hi,
> > 
> > almost. Two issues left with this section.
> > 
> > 1. It makes no sense to check the results of usb_clear_halt()
> > If it fails, we cannot do anything sensible. We have to restart
> > IO and hope for the best. Not doing it that way risks introducing
> > a regression.
> > 
> > 2. usb_autopm_put_interface() must be after acm_submit_read_urbs()
> > because the URBs are to be killed when the device is suspended.
> > 
> > And on a related note:
> > 
> > 3. The device may be reset externally before the work queue
> > is executed. pre_reset() needs to clear the flag EVENT_RX_STALL.
> 
> Hi,
> 
> thanks for feedback. Here's an update:

This is looking good. Please resend so Greg can pick it up.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cdc-acm: clear halt condition

[Ladislav Michl]

On Sun, Nov 06, 2016 at 10:30:02PM +0100, Oliver Neukum wrote:
> Hi,
> 
> almost. Two issues left with this section.
> 
> 1. It makes no sense to check the results of usb_clear_halt()
> If it fails, we cannot do anything sensible. We have to restart
> IO and hope for the best. Not doing it that way risks introducing
> a regression.
> 
> 2. usb_autopm_put_interface() must be after acm_submit_read_urbs()
> because the URBs are to be killed when the device is suspended.
> 
> And on a related note:
> 
> 3. The device may be reset externally before the work queue
> is executed. pre_reset() needs to clear the flag EVENT_RX_STALL.

Hi,

thanks for feedback. Here's an update:
-- >8 --
Subject: [PATCHv3 4/4] cdc-acm: clear halt condition

Signed-off-by: Ladislav Michl 
---
 drivers/usb/class/cdc-acm.c | 59 +++--
 drivers/usb/class/cdc-acm.h |  3 +++
 2 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index b76c95c..711d680 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -424,15 +424,29 @@ static void acm_read_bulk_callback(struct urb *urb)
return;
}
 
-   if (status) {
-   set_bit(rb->index, >read_urbs_free);
-   if ((status != -ENOENT) || (urb->actual_length == 0))
-   return;
+   switch (status) {
+   case 0:
+   usb_mark_last_busy(acm->dev);
+   acm_process_read_urb(acm, urb);
+   break;
+   case -EPIPE:
+   set_bit(EVENT_RX_STALL, >flags);
+   schedule_work(>work);
+   return;
+   case -ECONNRESET:
+   case -ENOENT:
+   case -ESHUTDOWN:
+   dev_dbg(>data->dev,
+   "%s - urb shutting down with status: %d\n",
+   __func__, status);
+   return;
+   default:
+   dev_dbg(>data->dev,
+   "%s - nonzero urb status received: %d\n",
+   __func__, status);
+   break;
}
 
-   usb_mark_last_busy(acm->dev);
-
-   acm_process_read_urb(acm, urb);
/*
 * Unthrottle may run on another CPU which needs to see events
 * in the same order. Submission has an implict barrier
@@ -468,14 +482,33 @@ static void acm_write_bulk(struct urb *urb)
spin_lock_irqsave(>write_lock, flags);
acm_write_done(acm, wb);
spin_unlock_irqrestore(>write_lock, flags);
+   set_bit(EVENT_TTY_WAKEUP, >flags);
schedule_work(>work);
 }
 
 static void acm_softint(struct work_struct *work)
 {
+   int i;
struct acm *acm = container_of(work, struct acm, work);
 
-   tty_port_tty_wakeup(>port);
+   if (test_bit(EVENT_RX_STALL, >flags)) {
+   if (!(usb_autopm_get_interface(acm->data))) {
+   for (i = 0; i < acm->rx_buflimit; i++) {
+   usb_kill_urb(acm->read_urbs[i]);
+   set_bit(i, >read_urbs_free);
+   }
+   usb_clear_halt(acm->dev, acm->in);
+   acm_submit_read_urbs(acm, GFP_KERNEL);
+   usb_autopm_put_interface(acm->data);
+
+   }
+   clear_bit(EVENT_RX_STALL, >flags);
+   }
+
+   if (test_bit(EVENT_TTY_WAKEUP, >flags)) {
+   tty_port_tty_wakeup(>port);
+   clear_bit(EVENT_TTY_WAKEUP, >flags);
+   }
 }
 
 /*
@@ -1654,6 +1687,15 @@ static int acm_reset_resume(struct usb_interface *intf)
 
 #endif /* CONFIG_PM */
 
+static int acm_pre_reset(struct usb_interface *intf)
+{
+   struct acm *acm = usb_get_intfdata(intf);
+
+   clear_bit(EVENT_RX_STALL, >flags);
+
+   return 0;
+}
+
 #define NOKIA_PCSUITE_ACM_INFO(x) \
USB_DEVICE_AND_INTERFACE_INFO(0x0421, x, \
USB_CLASS_COMM, USB_CDC_SUBCLASS_ACM, \
@@ -1895,6 +1937,7 @@ static struct usb_driver acm_driver = {
.resume =   acm_resume,
.reset_resume = acm_reset_resume,
 #endif
+   .pre_reset =acm_pre_reset,
.id_table = acm_ids,
 #ifdef CONFIG_PM
.supports_autosuspend = 1,
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index 58ddd25..1db974d 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -103,6 +103,9 @@ struct acm {
spinlock_t write_lock;
struct mutex mutex;
bool disconnected;
+   unsigned long flags;
+#  define EVENT_TTY_WAKEUP 0
+#  define EVENT_RX_STALL   1
struct usb_cdc_line_coding line;/* bits, stop, parity */
struct work_struct work;/* work queue entry for 
line discipline waking up */
unsigned int ctrlin;/* input control lines 
(DCD, DSR, RI, break, 

Re: [PATCH 4/4] cdc-acm: clear halt condition

[Oliver Neukum]

On Sun, 2016-11-06 at 10:07 +0100, Ladislav Michl wrote:
> +   if (test_bit(EVENT_RX_STALL, >flags)) {
> +   status = usb_autopm_get_interface(acm->data);
> +   if (!status) {
> +   for (i = 0; i < acm->rx_buflimit; i++) {
> +   usb_kill_urb(acm->read_urbs[i]);
> +   set_bit(i, >read_urbs_free);
> +   }
> +   status = usb_clear_halt(acm->dev, acm->in);
> +   usb_autopm_put_interface(acm->data);
> +   if (!status)
> +   acm_submit_read_urbs(acm, GFP_KERNEL);

Hi,

almost. Two issues left with this section.

1. It makes no sense to check the results of usb_clear_halt()
If it fails, we cannot do anything sensible. We have to restart
IO and hope for the best. Not doing it that way risks introducing
a regression.

2. usb_autopm_put_interface() must be after acm_submit_read_urbs()
because the URBs are to be killed when the device is suspended.

And on a related note:

3. The device may be reset externally before the work queue
is executed. pre_reset() needs to clear the flag EVENT_RX_STALL.

Regards
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] cdc-acm: clear halt condition

[Ladislav Michl]

On Sun, Nov 06, 2016 at 07:25:19AM +0100, Oliver Neukum wrote:
> On Sun, 2016-11-06 at 01:36 +0100, Ladislav Michl wrote:
> >  
> > @@ -475,7 +490,30 @@ static void acm_softint(struct work_struct *work)
> >  {
> > struct acm *acm = container_of(work, struct acm, work);
> >  
> > -   tty_port_tty_wakeup(>port);
> > +   dev_vdbg(>data->dev, "scheduled work\n");
> > +
> > +   if (test_bit(EVENT_RX_STALL, >flags)) {
> > +   int i, status;
> > +
> > +   for (i = 0; i < acm->rx_buflimit; i++) {
> > +   usb_kill_urb(acm->read_urbs[i]);
> > +   set_bit(i, >read_urbs_free);
> > +   }
> > +
> > +   status = usb_autopm_get_interface(acm->data);
> 
> No. If you really resume the device here, you reanimate the URBs
> you just killed. The order must be inverted.
> 
> > +   if (!status) {
> > +   status = usb_clear_halt(acm->dev, acm->in);
> > +   usb_autopm_put_interface(acm->data);
> > +   }
> > +   if (!status)
> > +   acm_submit_read_urbs(acm, GFP_KERNEL);
> 
> No, you would kill the device. Either do it all conditionally
> or nothing.

I do not follow. Did you mean something like this?
-- >8 --
Subject: [PATCHv2 4/4] cdc-acm: clear halt condition

Signed-off-by: Ladislav Michl 
---
 drivers/usb/class/cdc-acm.c | 53 ++---
 drivers/usb/class/cdc-acm.h |  3 +++
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index b76c95c..c122fdd 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -424,15 +424,29 @@ static void acm_read_bulk_callback(struct urb *urb)
return;
}
 
-   if (status) {
-   set_bit(rb->index, >read_urbs_free);
-   if ((status != -ENOENT) || (urb->actual_length == 0))
-   return;
+   switch (status) {
+   case 0:
+   usb_mark_last_busy(acm->dev);
+   acm_process_read_urb(acm, urb);
+   break;
+   case -EPIPE:
+   set_bit(EVENT_RX_STALL, >flags);
+   schedule_work(>work);
+   return;
+   case -ECONNRESET:
+   case -ENOENT:
+   case -ESHUTDOWN:
+   dev_dbg(>data->dev,
+   "%s - urb shutting down with status: %d\n",
+   __func__, status);
+   return;
+   default:
+   dev_dbg(>data->dev,
+   "%s - nonzero urb status received: %d\n",
+   __func__, status);
+   break;
}
 
-   usb_mark_last_busy(acm->dev);
-
-   acm_process_read_urb(acm, urb);
/*
 * Unthrottle may run on another CPU which needs to see events
 * in the same order. Submission has an implict barrier
@@ -468,14 +482,37 @@ static void acm_write_bulk(struct urb *urb)
spin_lock_irqsave(>write_lock, flags);
acm_write_done(acm, wb);
spin_unlock_irqrestore(>write_lock, flags);
+   set_bit(EVENT_TTY_WAKEUP, >flags);
schedule_work(>work);
 }
 
 static void acm_softint(struct work_struct *work)
 {
+   int i, status;
struct acm *acm = container_of(work, struct acm, work);
 
-   tty_port_tty_wakeup(>port);
+   dev_vdbg(>data->dev, "scheduled work\n");
+
+   if (test_bit(EVENT_RX_STALL, >flags)) {
+   status = usb_autopm_get_interface(acm->data);
+   if (!status) {
+   for (i = 0; i < acm->rx_buflimit; i++) {
+   usb_kill_urb(acm->read_urbs[i]);
+   set_bit(i, >read_urbs_free);
+   }
+   status = usb_clear_halt(acm->dev, acm->in);
+   usb_autopm_put_interface(acm->data);
+   if (!status)
+   acm_submit_read_urbs(acm, GFP_KERNEL);
+
+   }
+   clear_bit(EVENT_RX_STALL, >flags);
+   }
+
+   if (test_bit(EVENT_TTY_WAKEUP, >flags)) {
+   tty_port_tty_wakeup(>port);
+   clear_bit(EVENT_TTY_WAKEUP, >flags);
+   }
 }
 
 /*
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index 58ddd25..1db974d 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -103,6 +103,9 @@ struct acm {
spinlock_t write_lock;
struct mutex mutex;
bool disconnected;
+   unsigned long flags;
+#  define EVENT_TTY_WAKEUP 0
+#  define EVENT_RX_STALL   1
struct usb_cdc_line_coding line;/* bits, stop, parity */
struct work_struct work;/* work queue entry for 
line discipline waking up */
unsigned int ctrlin;/* input control lines 
(DCD, DSR, RI, break, overruns) */
-- 

Re: [PATCH 4/4] cdc-acm: clear halt condition

[Oliver Neukum]

On Sun, 2016-11-06 at 01:36 +0100, Ladislav Michl wrote:
>  
> @@ -475,7 +490,30 @@ static void acm_softint(struct work_struct *work)
>  {
>   struct acm *acm = container_of(work, struct acm, work);
>  
> - tty_port_tty_wakeup(>port);
> + dev_vdbg(>data->dev, "scheduled work\n");
> +
> + if (test_bit(EVENT_RX_STALL, >flags)) {
> + int i, status;
> +
> + for (i = 0; i < acm->rx_buflimit; i++) {
> + usb_kill_urb(acm->read_urbs[i]);
> + set_bit(i, >read_urbs_free);
> + }
> +
> + status = usb_autopm_get_interface(acm->data);

No. If you really resume the device here, you reanimate the URBs
you just killed. The order must be inverted.

> + if (!status) {
> + status = usb_clear_halt(acm->dev, acm->in);
> + usb_autopm_put_interface(acm->data);
> + }
> + if (!status)
> + acm_submit_read_urbs(acm, GFP_KERNEL);

No, you would kill the device. Either do it all conditionally
or nothing.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html