On Thu, Apr 13, 2017 at 04:06:47AM -0400, A Sun wrote:
>
> fix previous v1 patch error; incorrect location of "ir->pipe_in = pipe;"
> caused null pointer dereference
>
> Bug:
>
> Once IR blasting or mceusb device commands fail with mce_async_callback() TX -EPIPE error, all subsequent TX to device then fail with the same error.
> ...
> [ 249.986174] mceusb 1-1.2:1.0: requesting 38000 HZ carrier
> [ 249.986210] mceusb 1-1.2:1.0: send request called (size=0x4)
> [ 249.986256] mceusb 1-1.2:1.0: send request complete (res=0)
> [ 249.986403] mceusb 1-1.2:1.0: Error: request urb status = -32 (TX HALT)
> [ 249.999885] mceusb 1-1.2:1.0: send request called (size=0x3)
> [ 249.999929] mceusb 1-1.2:1.0: send request complete (res=0)
> [ 250.000013] mceusb 1-1.2:1.0: Error: request urb status = -32 (TX HALT)
> [ 250.019830] mceusb 1-1.2:1.0: send request called (size=0x21)
> [ 250.019868] mceusb 1-1.2:1.0: send request complete (res=0)
> [ 250.020007] mceusb 1-1.2:1.0: Error: request urb status = -32 (TX HALT)
> ...
>
> Fix:
>
> Message pertains to TX usb halt (stall) condition requiring usb_clear_halt() call in non-interrupt context to recover.
> Add USB TX halt error handling similar to the RX halt handling case from an earlier patch proposal.
> Reorder some mceusb code to accommodate TX halt error handling.
>
> This patch depends on the earlier proposed patch set:
> [PATCH 1/3] [media] mceusb: RX -EPIPE (urb status = -32) lockup failure fix
> [PATCH 2/3] [media] mceusb: sporadic RX truncation corruption fix
> [PATCH 3/3] [media] mceusb: fix inaccurate debug buffer dumps and misleading
>
> Tested with:
>
> Linux raspberrypi 4.4.50-v7+ #970 SMP Mon Feb 20 19:18:29 GMT 2017 armv7l GNU/Linux
> mceusb 1-1.2:1.0: Registered SMK eHome Infrared Transceiver with mce emulator interface version 1
> mceusb 1-1.2:1.0: 2 tx ports (0x1 cabled) and 2 rx sensors (0x1 active)
>
> Fault simulation/injection is by executing the following USB operation in a mceusb instrumented driver, prior to TX I/O.
> retval = usb_control_msg(ir->usbdev, usb_sndctrlpipe(ir->usbdev, 0),
> USB_REQ_SET_FEATURE, USB_RECIP_ENDPOINT,
> USB_ENDPOINT_HALT, usb_pipeendpoint(ir->pipe_out),
> NULL, 0, USB_CTRL_SET_TIMEOUT);
> dev_dbg(ir->dev, "set halt retval, %d", retval);
>
> After setting halt state for the TX endpoint, perform an lirc "irsend" to generate TX traffic to device.
> After the TX HALT, the patch restores subsequent TX to working state.
> ...
> [ 508.009638] mceusb 1-1.2:1.0: send request called (size=0x3)
> [ 508.009697] mceusb 1-1.2:1.0: send request complete (res=0)
> [ 508.009847] mce_async_callback()
> [ 508.009864] mceusb 1-1.2:1.0: Error: request urb status = -32 (TX HALT)
> [ 508.009890] mceusb 1-1.2:1.0: kevent 0 scheduled
> [ 508.021552] mceusb 1-1.2:1.0: send request called (size=0x21)
> [ 508.021598] mceusb 1-1.2:1.0: send request complete (res=0)
> [ 508.021963] mce_async_callback()
> [ 508.021981] mceusb 1-1.2:1.0: tx data: 84 b0 0c 8c 0c 84 8c 0c 8c 0c 84 8c 0c 8c 0c 84 98 0c 98 0c 84 98 0c 8c 0c 84 8c 0c 8c 0c 81 8c 80 (length=33)
> [ 508.021997] mceusb 1-1.2:1.0: Raw IR data, 0 pulse/space samples
> [ 508.066627] mceusb 1-1.2:1.0: send request called (size=0x3)
> [ 508.066669] mceusb 1-1.2:1.0: send request complete (res=0)
> [ 508.066841] mce_async_callback()
> [ 508.066858] mceusb 1-1.2:1.0: tx data: 9f 08 03 (length=3)
> ...
>
> Open issue(s):
>
> Testing with Pinnacle mceusb device reveals device specific (non USB 2.0 standard) misbehavior with respect to USB TX halt.
>
> Linux raspberrypi 4.4.50-v7+ #970 SMP Mon Feb 20 19:18:29 GMT 2017 armv7l GNU/Linux
> mceusb 1-1.2:1.0: Registered Pinnacle Systems PCTV Remote USB with mce emulator interface version 1
> mceusb 1-1.2:1.0: 2 tx ports (0x1 cabled) and 2 rx sensors (0x1 active)
>
> The Pinnacle device failed Linux usbtest module (modded to bind to the Pinnacle) test 13 with bogus halt status and -110 (-ETIMEDOUT) errors.
>
> [ 4558.114664] usbcore: deregistering interface driver mceusb
> [14956.572207] usbtest 1-1.2:1.0: mce ir device
> [14956.572234] usbtest 1-1.2:1.0: full-speed {control bulk-out} tests
> [14956.572363] usbcore: registered new interface driver usbtest
> [15241.341143] usbtest 1-1.2:1.0: TEST 1: write 512 bytes 1000 times
> [15456.690845] usbtest 1-1.2:1.0: TEST 13: set/clear 1000 halts
> [15456.691362] usbtest 1-1.2:1.0: ep 02 bogus status: 0001 != 0
> [15456.691381] usbtest 1-1.2:1.0: halts failed, iterations left 999
>
> [37432.646344] usbcore: deregistering interface driver mceusb
> [37468.447929] usbtest 1-1.2:1.0: mce ir device
> [37468.447956] usbtest 1-1.2:1.0: full-speed {control bulk-out} tests
> [37468.448079] usbcore: registered new interface driver usbtest
> [37519.150810] usbtest 1-1.2:1.0: TEST 1: write 512 bytes 1000 times
> [37537.853493] usbtest 1-1.2:1.0: TEST 13: set/clear 1000 halts
> [37547.866871] usb 1-1.2: verify_not_halted failed, iterations left 0, status -110 (not 0)
> [37547.866901] usbtest 1-1.2:1.0: halts failed, iterations left 999
>
> With mceusb, upon executing usb_clear_halt() on this Pinnacle mceusb USB TX end-point, regardless of its halt/stall state, TX functionality silently ceases.
> mce_async_callback() invocations cease, and there are no other error indications from usb_submit_urb() or anywhere else.
> An escalating USB reset was necessary to restore this device to working state.
> Certain mceusb devices may require device specific recovery procedure for TX halt conditions, which this patch does not address.
>
> Signed-off-by: A Sun <as1033x@xxxxxxxxxxx>
> ---
> drivers/media/rc/mceusb.c | 89 +++++++++++++++++++++++++++++------------------
> 1 file changed, 56 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> index a9a9a85..af46860 100644
> --- a/drivers/media/rc/mceusb.c
> +++ b/drivers/media/rc/mceusb.c
> @@ -419,6 +419,7 @@ struct mceusb_dev {
> struct urb *urb_in;
> unsigned int pipe_in;
> struct usb_endpoint_descriptor *usb_ep_out;
> + unsigned int pipe_out;
>
> /* buffers and dma */
> unsigned char *buf_in;
> @@ -456,7 +457,8 @@ struct mceusb_dev {
> u8 txports_cabled; /* bitmask of transmitters with cable */
> u8 rxports_active; /* bitmask of active receive sensors */
>
> - /* kevent support */
> + /* async error handler mceusb_deferred_kevent() support
> + * via workqueue kworker (previously keventd) threads */
Multi-line comments should be like:
/*
* async error handler mceusb_deferred_kevent() support
* via workqueue kworker (previously keventd) threads
*/
Then again the comment says no more than what you can read in the source
code, so I would rephrase it
/* clear halt should be done in process context */
> struct work_struct kevent;
> unsigned long kevent_flags;
> # define EVENT_TX_HALT 0
> @@ -694,6 +696,22 @@ static void mceusb_dev_printdata(struct mceusb_dev *ir, char *buf,
> #endif
> }
>
> +/*
> + * Schedule work that can't be done in interrupt handlers
> + * (mceusb_dev_recv() and mce_async_callback()) nor tasklets.
> + * Invokes mceusb_deferred_kevent() for recovering from
> + * error events specified by the kevent bit field.
> + */
> +static void mceusb_defer_kevent(struct mceusb_dev *ir, int kevent)
> +{
> + set_bit(kevent, &ir->kevent_flags);
> + if (!schedule_work(&ir->kevent)) {
> + dev_err(ir->dev, "kevent %d may have been dropped", kevent);
> + } else {
> + dev_dbg(ir->dev, "kevent %d scheduled", kevent);
> + }
You don't need curly braces when there is only one statement on an if/else
branch.
> +}
> +
> static void mce_async_callback(struct urb *urb)
> {
> struct mceusb_dev *ir;
> @@ -720,6 +738,11 @@ static void mce_async_callback(struct urb *urb)
> break;
>
> case -EPIPE:
> + dev_err(ir->dev, "Error: request urb status = %d (TX HALT)",
> + urb->status);
> + mceusb_defer_kevent(ir, EVENT_TX_HALT);
> + break;
> +
> default:
> dev_err(ir->dev, "Error: request urb status = %d", urb->status);
> break;
> @@ -734,7 +757,7 @@ static void mce_async_callback(struct urb *urb)
> static void mce_request_packet(struct mceusb_dev *ir, unsigned char *data,
> int size)
> {
> - int res, pipe;
> + int res;
> struct urb *async_urb;
> struct device *dev = ir->dev;
> unsigned char *async_buf;
> @@ -753,17 +776,12 @@ static void mce_request_packet(struct mceusb_dev *ir, unsigned char *data,
>
> /* outbound data */
> if (usb_endpoint_xfer_int(ir->usb_ep_out)) {
> - pipe = usb_sndintpipe(ir->usbdev,
> - ir->usb_ep_out->bEndpointAddress);
> - usb_fill_int_urb(async_urb, ir->usbdev, pipe, async_buf,
> - size, mce_async_callback, ir,
> + usb_fill_int_urb(async_urb, ir->usbdev, ir->pipe_out,
> + async_buf, size, mce_async_callback, ir,
> ir->usb_ep_out->bInterval);
> } else {
> - pipe = usb_sndbulkpipe(ir->usbdev,
> - ir->usb_ep_out->bEndpointAddress);
> - usb_fill_bulk_urb(async_urb, ir->usbdev, pipe,
> - async_buf, size, mce_async_callback,
> - ir);
> + usb_fill_bulk_urb(async_urb, ir->usbdev, ir->pipe_out,
> + async_buf, size, mce_async_callback, ir);
> }
No curly braces needed now that there is one statement left in each
branch.
> memcpy(async_buf, data, size);
>
> @@ -1034,23 +1052,6 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len)
> }
> }
>
> -/*
> - * Workqueue task dispatcher
> - * for work that can't be done in interrupt handlers
> - * (mceusb_dev_recv() and mce_async_callback()) nor tasklets.
> - * Invokes mceusb_deferred_kevent() for recovering from
> - * error events specified by the kevent bit field.
> - */
> -static void mceusb_defer_kevent(struct mceusb_dev *ir, int kevent)
> -{
> - set_bit(kevent, &ir->kevent_flags);
> - if (!schedule_work(&ir->kevent)) {
> - dev_err(ir->dev, "kevent %d may have been dropped", kevent);
> - } else {
> - dev_dbg(ir->dev, "kevent %d scheduled", kevent);
> - }
> -}
> -
> static void mceusb_dev_recv(struct urb *urb)
> {
> struct mceusb_dev *ir;
> @@ -1220,16 +1221,28 @@ static void mceusb_deferred_kevent(struct work_struct *work)
> if (status < 0) {
> dev_err(ir->dev, "rx clear halt error %d",
> status);
> - return;
> + goto done_rx_halt;
This function can easily be re-written without gotos and it will be
much more readible.
> }
> clear_bit(EVENT_RX_HALT, &ir->kevent_flags);
> status = usb_submit_urb(ir->urb_in, GFP_KERNEL);
> if (status < 0) {
> dev_err(ir->dev, "rx unhalt submit urb error %d",
> status);
> - return;
> + goto done_rx_halt;
> }
> }
> +done_rx_halt:
> + if (test_bit(EVENT_TX_HALT, &ir->kevent_flags)) {
> + status = usb_clear_halt(ir->usbdev, ir->pipe_out);
> + if (status < 0) {
> + dev_err(ir->dev, "tx clear halt error %d",
> + status);
> + goto done_tx_halt;
> + }
> + clear_bit(EVENT_TX_HALT, &ir->kevent_flags);
> + }
> +done_tx_halt:
> + return;
> }
>
> static struct rc_dev *mceusb_init_rc_dev(struct mceusb_dev *ir)
> @@ -1373,6 +1386,7 @@ static int mceusb_dev_probe(struct usb_interface *intf,
> if (!ir->urb_in)
> goto urb_in_alloc_fail;
>
> + ir->pipe_in = pipe;
> ir->usbdev = usb_get_dev(dev);
> ir->dev = &intf->dev;
> ir->len_in = maxp;
> @@ -1383,6 +1397,13 @@ static int mceusb_dev_probe(struct usb_interface *intf,
>
> /* Saving usb interface data for use by the transmitter routine */
> ir->usb_ep_out = ep_out;
> + if (usb_endpoint_xfer_int(ir->usb_ep_out)) {
> + ir->pipe_out = usb_sndintpipe(ir->usbdev,
> + ir->usb_ep_out->bEndpointAddress);
> + } else {
> + ir->pipe_out = usb_sndbulkpipe(ir->usbdev,
> + ir->usb_ep_out->bEndpointAddress);
> + }
Unnecessary braces.
> if (dev->descriptor.iManufacturer
> && usb_string(dev, dev->descriptor.iManufacturer,
> @@ -1394,13 +1415,14 @@ static int mceusb_dev_probe(struct usb_interface *intf,
> snprintf(name + strlen(name), sizeof(name) - strlen(name),
> " %s", buf);
>
> + /* initialize async USB error handler before registering
> + * or activating any mceusb RX and TX functions */
Bad multiline comment.
> + INIT_WORK(&ir->kevent, mceusb_deferred_kevent);
> +
> ir->rc = mceusb_init_rc_dev(ir);
> if (!ir->rc)
> goto rc_dev_fail;
>
> - ir->pipe_in = pipe;
> - INIT_WORK(&ir->kevent, mceusb_deferred_kevent);
> -
> /* wire up inbound data handler */
> if (usb_endpoint_xfer_int(ep_in)) {
> usb_fill_int_urb(ir->urb_in, dev, pipe, ir->buf_in, maxp,
> @@ -1450,6 +1472,7 @@ static int mceusb_dev_probe(struct usb_interface *intf,
>
> /* Error-handling path */
> rc_dev_fail:
> + cancel_work_sync(&ir->kevent);
> usb_put_dev(ir->usbdev);
> usb_kill_urb(ir->urb_in);
> usb_free_urb(ir->urb_in);
> --
> 2.1.4
