From patchwork Mon Dec 14 23:29:16 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dmitry Torokhov X-Patchwork-Id: 7849931 Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 6AB62BEEE1 for ; Mon, 14 Dec 2015 23:29:24 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 215F320320 for ; Mon, 14 Dec 2015 23:29:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5678420304 for ; Mon, 14 Dec 2015 23:29:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753675AbbLNX3U (ORCPT ); Mon, 14 Dec 2015 18:29:20 -0500 Received: from mail-pf0-f180.google.com ([209.85.192.180]:36649 "EHLO mail-pf0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753654AbbLNX3T (ORCPT ); Mon, 14 Dec 2015 18:29:19 -0500 Received: by pfbu66 with SMTP id u66so69054105pfb.3 for ; Mon, 14 Dec 2015 15:29:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=H6Hf0zq5diMhz9yo85z5+EIexR8PtU2tcXHdUsEtsEM=; b=kTi72hJsY7clPxVRFUMhSzZmpKIZhfcmWDLYhYOx40ytfCAJyAJjymjhipgLmWwnSq RTfgnkmwUrFgYmNBRFkhIGuCWfyPF04tXCbzRbuXnK9cPXZA5TwfjC6jT10vw5MvIpLP gWmMwC4hkLOUua4yXdodtM+eFoudQRUNTZgvfbl/5SNBWtUgje4I98Xot3tVifSa8LfB T58vPBoP5zquA771Nuv6IyOATmPy75nXoFdxmi5IQqRIqKmFM8T18MOYokH9M8RT1j1L SCk92eshPBGM8UJ6/6M/9tJJFyrZTQ5mgAsNMzAOXrzfBhMjtS3kOWWM2S9JfFEAX1IK bJFQ== X-Received: by 10.98.86.3 with SMTP id k3mr24831208pfb.111.1450135759110; Mon, 14 Dec 2015 15:29:19 -0800 (PST) Received: from dtor-ws ([2620:0:1000:1301:1dc6:d5f5:521b:1126]) by smtp.gmail.com with ESMTPSA id d14sm44621590pfj.4.2015.12.14.15.29.17 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 14 Dec 2015 15:29:18 -0800 (PST) Date: Mon, 14 Dec 2015 15:29:16 -0800 From: Dmitry Torokhov To: Pavel Rojtberg Cc: linux-input@vger.kernel.org, pgriffais@valvesoftware.com, gregkh@linuxfoundation.org Subject: Re: [PATCH 1/5] Input: xpad: handle "present" and "gone" correctly Message-ID: <20151214232916.GA16209@dtor-ws> References: <1446391899-24250-1-git-send-email-rojtberg@gmail.com> <1446391899-24250-2-git-send-email-rojtberg@gmail.com> <20151210070239.GD35505@dtor-ws> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20151210070239.GD35505@dtor-ws> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Dec 09, 2015 at 11:02:39PM -0800, Dmitry Torokhov wrote: > On Sun, Nov 01, 2015 at 04:31:35PM +0100, Pavel Rojtberg wrote: > > From: "Pierre-Loup A. Griffais" > > > > Handle the "a new device is present" message properly by dynamically > > creating the input device at this point in time. This means we now do > > not "preallocate" all 4 devices when a single > > wireless base station is seen. This requires a workqueue as we are in > > interrupt context when we learn about this. > > > > static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data) > > { > > + int presence; > > + > > /* Presence change */ > > if (data[0] & 0x08) { > > - if (data[1] & 0x80) { > > - xpad->pad_present = 1; > > - /* > > - * Light up the segment corresponding to > > - * controller number. > > - */ > > - xpad_identify_controller(xpad); > > - } else > > - xpad->pad_present = 0; > > + presence = (data[1] & 0x80) != 0; > > + > > + if (xpad->pad_present != presence) { > > + xpad->pad_present = presence; > > + schedule_work(&xpad->work); > > + } > > I think this is racy: we'll crash if we get motion packets before we > finish creating input device. I think we should be returning whether we > want to have xpad->irq_in URB be re-submitted and in case we scheduke > work we'd return "false" and have work resubmit IRQ when it is done > creating or destroying input device. Hmm, after I thought about it some more I am not sure that simply not submitting any more input URBs is sufficient: what will happen if the device sends motion event before we send our query (I.e. user is holding a button on the controller). I think it is safer to make sure we are not accessing half-created or half-gone input device when processing packet. Could you please take a look at the new version of this patch below? Thanks! diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c index c44cbd4..1d51d24 100644 --- a/drivers/input/joystick/xpad.c +++ b/drivers/input/joystick/xpad.c @@ -76,6 +76,8 @@ */ #include +#include +#include #include #include #include @@ -319,10 +321,12 @@ MODULE_DEVICE_TABLE(usb, xpad_table); struct usb_xpad { struct input_dev *dev; /* input device interface */ + struct input_dev __rcu *x360w_dev; struct usb_device *udev; /* usb device */ struct usb_interface *intf; /* usb interface */ - int pad_present; + bool pad_present; + bool input_created; struct urb *irq_in; /* urb for interrupt in report */ unsigned char *idata; /* input data */ @@ -343,8 +347,12 @@ struct usb_xpad { int xtype; /* type of xbox device */ int pad_nr; /* the order x360 pads were attached */ const char *name; /* name of the device */ + struct work_struct work; /* init/remove device from callback */ }; +static int xpad_init_input(struct usb_xpad *xpad); +static void xpad_deinit_input(struct usb_xpad *xpad); + /* * xpad_process_packet * @@ -424,11 +432,9 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d * http://www.free60.org/wiki/Gamepad */ -static void xpad360_process_packet(struct usb_xpad *xpad, +static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev, u16 cmd, unsigned char *data) { - struct input_dev *dev = xpad->dev; - /* digital pad */ if (xpad->mapping & MAP_DPAD_TO_BUTTONS) { /* dpad as buttons (left, right, up, down) */ @@ -495,7 +501,30 @@ static void xpad360_process_packet(struct usb_xpad *xpad, input_sync(dev); } -static void xpad_identify_controller(struct usb_xpad *xpad); +static void xpad_presence_work(struct work_struct *work) +{ + struct usb_xpad *xpad = container_of(work, struct usb_xpad, work); + int error; + + if (xpad->pad_present) { + error = xpad_init_input(xpad); + if (error) { + /* complain only, not much else we can do here */ + dev_err(&xpad->dev->dev, + "unable to init device: %d\n", error); + } else { + rcu_assign_pointer(xpad->x360w_dev, xpad->dev); + } + } else { + RCU_INIT_POINTER(xpad->x360w_dev, NULL); + synchronize_rcu(); + /* + * Now that we are sure xpad360w_process_packet is not + * using input device we can get rid of it. + */ + xpad_deinit_input(xpad); + } +} /* * xpad360w_process_packet @@ -513,24 +542,28 @@ static void xpad_identify_controller(struct usb_xpad *xpad); */ static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data) { + struct input_dev *dev; + bool present; + /* Presence change */ if (data[0] & 0x08) { - if (data[1] & 0x80) { - xpad->pad_present = 1; - /* - * Light up the segment corresponding to - * controller number. - */ - xpad_identify_controller(xpad); - } else - xpad->pad_present = 0; + present = (data[1] & 0x80) != 0; + + if (xpad->pad_present != present) { + xpad->pad_present = present; + schedule_work(&xpad->work); + } } /* Valid pad data */ if (data[1] != 0x1) return; - xpad360_process_packet(xpad, cmd, &data[4]); + rcu_read_lock(); + dev = rcu_dereference(xpad->x360w_dev); + if (dev) + xpad360_process_packet(xpad, dev, cmd, &data[4]); + rcu_read_unlock(); } /* @@ -659,7 +692,7 @@ static void xpad_irq_in(struct urb *urb) switch (xpad->xtype) { case XTYPE_XBOX360: - xpad360_process_packet(xpad, 0, xpad->idata); + xpad360_process_packet(xpad, xpad->dev, 0, xpad->idata); break; case XTYPE_XBOX360W: xpad360w_process_packet(xpad, 0, xpad->idata); @@ -1001,14 +1034,7 @@ static int xpad_led_probe(struct usb_xpad *xpad) if (error) goto err_free_id; - if (xpad->xtype == XTYPE_XBOX360) { - /* - * Light up the segment corresponding to controller - * number on wired devices. On wireless we'll do that - * when they respond to "presence" packet. - */ - xpad_identify_controller(xpad); - } + xpad_identify_controller(xpad); return 0; @@ -1097,8 +1123,11 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs) static void xpad_deinit_input(struct usb_xpad *xpad) { - xpad_led_disconnect(xpad); - input_unregister_device(xpad->dev); + if (xpad->input_created) { + xpad->input_created = false; + xpad_led_disconnect(xpad); + input_unregister_device(xpad->dev); + } } static int xpad_init_input(struct usb_xpad *xpad) @@ -1181,6 +1210,7 @@ static int xpad_init_input(struct usb_xpad *xpad) if (error) goto err_disconnect_led; + xpad->input_created = true; return 0; err_disconnect_led: @@ -1241,6 +1271,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id xpad->mapping = xpad_device[i].mapping; xpad->xtype = xpad_device[i].xtype; xpad->name = xpad_device[i].name; + INIT_WORK(&xpad->work, xpad_presence_work); if (xpad->xtype == XTYPE_UNKNOWN) { if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) { @@ -1277,10 +1308,6 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id usb_set_intfdata(intf, xpad); - error = xpad_init_input(xpad); - if (error) - goto err_deinit_output; - if (xpad->xtype == XTYPE_XBOX360W) { /* * Submit the int URB immediately rather than waiting for open @@ -1292,7 +1319,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id xpad->irq_in->dev = xpad->udev; error = usb_submit_urb(xpad->irq_in, GFP_KERNEL); if (error) - goto err_deinit_input; + goto err_deinit_output; /* * Send presence packet. @@ -1304,13 +1331,15 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id error = xpad_inquiry_pad_presence(xpad); if (error) goto err_kill_in_urb; + } else { + error = xpad_init_input(xpad); + if (error) + goto err_deinit_output; } return 0; err_kill_in_urb: usb_kill_urb(xpad->irq_in); -err_deinit_input: - xpad_deinit_input(xpad); err_deinit_output: xpad_deinit_output(xpad); err_free_in_urb: @@ -1327,17 +1356,19 @@ static void xpad_disconnect(struct usb_interface *intf) { struct usb_xpad *xpad = usb_get_intfdata (intf); - xpad_deinit_input(xpad); - xpad_deinit_output(xpad); - - if (xpad->xtype == XTYPE_XBOX360W) { + if (xpad->xtype == XTYPE_XBOX360W) usb_kill_urb(xpad->irq_in); - } + + cancel_work_sync(&xpad->work); + + xpad_deinit_input(xpad); usb_free_urb(xpad->irq_in); usb_free_coherent(xpad->udev, XPAD_PKT_LEN, xpad->idata, xpad->idata_dma); + xpad_deinit_output(xpad); + kfree(xpad); usb_set_intfdata(intf, NULL);