linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: David Herrmann <dh.herrmann@gmail.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
	"Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Subject: Re: [PATCH 6/7] Input: xpad: handle "present" and "gone" correctly
Date: Mon, 3 Feb 2014 20:47:11 +0100	[thread overview]
Message-ID: <20140203194711.GC3625@kroah.com> (raw)
In-Reply-To: <CANq1E4QcTBDoTbyJrQSVPz8SNs+wa1ONhi8U1pqwJSxPZo18uQ@mail.gmail.com>

On Mon, Feb 03, 2014 at 06:37:13PM +0100, David Herrmann wrote:
> Hi
> 
> On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> >
> > Handle the "a new device is present" message properly by dynamically
> > creating the input device at this point in time.  This requires a
> > workqueue as we are in interrupt context when we learn about this.
> >
> > Also properly disconnect any devices that we are told are removed.
> >
> > Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/input/joystick/xpad.c | 50 ++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 40 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > index 7a07b95790d7..d342d41a7a0d 100644
> > --- a/drivers/input/joystick/xpad.c
> > +++ b/drivers/input/joystick/xpad.c
> > @@ -293,8 +293,11 @@ struct usb_xpad {
> >         int xtype;                      /* type of xbox device */
> >         int joydev_id;                  /* the minor of the device */
> >         const char *name;               /* name of the device */
> > +       struct work_struct work;        /* to init/remove device from callback */
> >  };
> >
> > +static int xpad_init_input(struct usb_xpad *xpad);
> > +
> >  /*
> >   *     xpad_process_packet
> >   *
> > @@ -437,6 +440,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
> >         input_sync(dev);
> >  }
> >
> > +static void presence_work_function(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\n");
> > +               }
> > +       } else {
> > +               input_unregister_device(xpad->dev);
> > +       }
> > +}
> > +
> >  /*
> >   * xpad360w_process_packet
> >   *
> > @@ -451,16 +470,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
> >   * 01.1 - Pad state (Bytes 4+) valid
> >   *
> >   */
> > -
> >  static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> >  {
> >         /* Presence change */
> >         if (data[0] & 0x08) {
> >                 if (data[1] & 0x80) {
> > -                       xpad->pad_present = 1;
> > -                       usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> > -               } else
> > -                       xpad->pad_present = 0;
> > +                       if (!xpad->pad_present) {
> > +                               xpad->pad_present = 1;
> > +                               usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> > +                               schedule_work(&xpad->work);
> > +                       }
> > +               } else {
> > +                       if (xpad->pad_present) {
> > +                               xpad->pad_present = 0;
> > +                               schedule_work(&xpad->work);
> > +                       }
> > +               }
> >         }
> >
> >         /* Valid pad data */
> > @@ -507,10 +532,13 @@ static void xpad_irq_in(struct urb *urb)
> >         }
> >
> >  exit:
> > -       retval = usb_submit_urb(urb, GFP_ATOMIC);
> > -       if (retval)
> > -               dev_err(dev, "%s - usb_submit_urb failed with result %d\n",
> > -                       __func__, retval);
> > +       if (xpad->pad_present) {
> > +               retval = usb_submit_urb(urb, GFP_ATOMIC);
> > +               if (retval)
> > +                       dev_err(dev,
> > +                               "%s - usb_submit_urb failed with result %d\n",
> > +                               __func__, retval);
> > +       }
> >  }
> >
> >  static void xpad_bulk_out(struct urb *urb)
> > @@ -991,6 +1019,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, presence_work_function);
> >
> >         if (xpad->xtype == XTYPE_UNKNOWN) {
> >                 if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
> > @@ -1136,7 +1165,8 @@ static void xpad_disconnect(struct usb_interface *intf)
> >         struct usb_xpad *xpad = usb_get_intfdata (intf);
> >
> >         xpad_led_disconnect(xpad);
> > -       input_unregister_device(xpad->dev);
> 
> You need cancel_work_sync(&xpad->work) here.
> And I think you need to do that *after* killing the urbs. Otherwise,
> the work might get rescheduled in parallel to this disconnect
> callback.

Ah, you are right, that should fix a USB core warning of submitting an
urb when the device has been removed that I saw a few times in testing.
Thanks for pointing it out.

greg k-h

  reply	other threads:[~2014-02-03 19:45 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-01-31 13:03 [PATCH 0/7] Input: xpad: Fix wireless controller connection and leds Greg Kroah-Hartman
2014-01-31 13:03 ` [PATCH 1/7] Input: xpad: use proper endpoint type Greg Kroah-Hartman
2014-01-31 13:03 ` [PATCH 2/7] Input: xpad: set the LEDs properly on XBox Wireless controllers Greg Kroah-Hartman
2014-02-03 17:31   ` David Herrmann
2014-02-03 19:48     ` Greg Kroah-Hartman
2014-02-03 22:35       ` Pierre-Loup A. Griffais
2014-01-31 13:03 ` [PATCH 3/7] Input: xpad: move the input device creation to a new function Greg Kroah-Hartman
2014-01-31 13:03 ` [PATCH 4/7] Input: xpad: Set the correct LED number Greg Kroah-Hartman
2014-02-03  8:22   ` Greg Kroah-Hartman
2014-01-31 13:03 ` [PATCH 5/7] Input: xpad: disconnect all Wireless controllers at init Greg Kroah-Hartman
2014-02-03 22:22   ` Pierre-Loup A. Griffais
2014-01-31 13:03 ` [PATCH 6/7] Input: xpad: handle "present" and "gone" correctly Greg Kroah-Hartman
2014-02-03 17:37   ` David Herrmann
2014-02-03 19:47     ` Greg Kroah-Hartman [this message]
2014-01-31 13:03 ` [PATCH 7/7] Input: xpad: properly name the LED class devices Greg Kroah-Hartman
2014-02-03 17:39   ` David Herrmann
2014-02-03 19:46     ` Greg Kroah-Hartman
2014-02-06  3:11 ` [PATCH 0/7] Input: xpad: Fix wireless controller connection and leds Zachary Lund

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140203194711.GC3625@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=dh.herrmann@gmail.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=linux-input@vger.kernel.org \
    --cc=pgriffais@valvesoftware.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).