Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Dmitry Torokhov]

On Mon, Sep 11, 2017 at 4:29 PM, Benson Leung  wrote:
> Hi Oliver,
>
> On Mon, Sep 11, 2017 at 01:02:52PM -0700, Benson Leung wrote:
>> Thanks for the pointer. I'll respin the patch with the no_resume version of
>> usb_autopm_get_interface and retest.
>>
>
> I went and tried this patch but with usb_autopm_get_interface_no_resume 
> instead
> and the original bug I was trying to fix with a Yubikey hid security key came
> back, so something is still wrong here.
>
> I went ahead and filed a bug to track this here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=764112
>
> I'll find some time to dig into this deeper and understand why this device
> ends up getting stuck in active instead of suspending when all handles are
> closed.

Meh, it is problem in hidraw that basically does
usb_autopm_put_interface() before letting usbhid_close() to run. Once
I swap  hid_hw_power() and hid_hw_close() it autosuspends properly.

I just sent out a patch.

Thanks.

-- 
Dmitry
--
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: usb/gadget: stalls in dummy_timer

[Dmitry Torokhov]

On Tue, Sep 12, 2017 at 05:48:51PM +0200, Andrey Konovalov wrote:
> On Mon, Sep 11, 2017 at 8:54 PM, Dmitry Torokhov
> <dmitry.torok...@gmail.com> wrote:
> > On Mon, Sep 11, 2017 at 8:15 AM, Andrey Konovalov <andreyk...@google.com> 
> > wrote:
> >> On Mon, Sep 11, 2017 at 3:25 PM, Alan Stern <st...@rowland.harvard.edu> 
> >> wrote:
> >>> On Mon, 11 Sep 2017, Andrey Konovalov wrote:
> >>>
> >>>> Hi!
> >>>>
> >>>> I've been getting stall reports like this one while fuzzing the USB
> >>>> stack with gadgetfs. I'm wondering whether this is a bug in gadgetfs
> >>>> or is this report induced by the changes I've made to the USB core
> >>>> code. I didn't touch gadgetfs code though (except for adding a few
> >>>> printk's).
> >>>>
> >>>> I'm on commit 81a84ad3cb5711cec79f4dd53a4ce026b092c432
> >>>
> >>> It's possible that this was caused by commit f16443a034c7 ("USB:
> >>> gadgetfs, dummy-hcd, net2280: fix locking for callbacks").  I've been
> >>> meaning to repair the commit but haven't done it yet.
> >>>
> >>> Can you test with that commit reverted?  You may end up seeing other
> >>> problems instead -- the ones that commit was intended to solve -- but
> >>> perhaps the stalls won't occur.
> >>
> >> So I've reverted both: the changes I made to USB core and the commit
> >> you mentioned, still saw the stalls.
> >>
> >> I've manged to find a way to reproduce this and now I'm not sure the
> >> problem is actually in gadgetfs, it might be the usbtouchscreen
> >> driver.
> >>
> >> The crash log is below.
> >>
> >> Thanks!
> >>
> >> gadgetfs: bound to dummy_udc driver
> >> usb 1-1: new full-speed USB device number 2 using dummy_hcd
> >> gadgetfs: connected
> >> gadgetfs: disconnected
> >> gadgetfs: connected
> >> usb 1-1: config 8 interface 0 altsetting 9 endpoint 0x8F has an
> >> invalid bInterval 0, changing to 10
> >> usb 1-1: config 8 interface 0 altsetting 9 endpoint 0x8F has invalid
> >> maxpacket 839, setting to 64
> >> usb 1-1: config 8 interface 0 altsetting 9 endpoint 0x7 has invalid
> >> maxpacket 1839, setting to 64
> >> usb 1-1: config 8 interface 0 has no altsetting 0
> >> usb 1-1: New USB device found, idVendor=0403, idProduct=f9e9
> >> usb 1-1: New USB device strings: Mfr=4, Product=8, SerialNumber=255
> >> usb 1-1: Product: a
> >> usb 1-1: Manufacturer: a
> >> usb 1-1: SerialNumber: a
> >> gadgetfs: configuration #8
> >> input: a a as /devices/platform/dummy_hcd.0/usb1/1-1/1-1:8.0/input/input8
> >> evbug: Connected device: input8 (a a at usb-dummy_hcd.0-1/input0)
> >> kworker/0:0: page allocation failure: order:0,
> >> mode:0x1280020(GFP_ATOMIC|__GFP_NOTRACK), nodemask=(null)
> >> kworker/0:0 cpuset=/ mems_allowed=0
> >
> > It seems you are running out of memory.
> >
> >> Swap cache stats: add 0, delete 0, find 0/0
> >> Free swap  = 0kB
> >> Total swap = 0kB
> >
> > And no swap. I think you need to give the box a bit more memory (or
> > there might be a leak somewhere).
> 
> Increasing the amount of memory doesn't help. It looks like
> usbtouch_irq() gets called in an infinite loop, and calls
> usb_submit_urb on each iteration, until the kernel runs out of memory.

Yes, that is exactly how USB interrupt-driven devices work. Their URB
completion routine handles the data and immediately resubmits URB to get
more data. The device/HCD will signal interrupt once more data is
available and the process starts over again. The only time we stop
resubmitting URBs if we get one of the following errors:

case -ETIME:
/* this urb is timing out */
dev_dbg(dev,
"%s - urb timed out - was the device unplugged?\n",
__func__);
return;
case -ECONNRESET:
case -ENOENT:
case -ESHUTDOWN:
case -EPIPE:
/* this urb is terminated, clean up */
dev_dbg(dev, "%s - urb shutting down with status: %d\n",
__func__, urb->status);
return;

So I'd start looking into dummy_hcd and see why it does not discard
processed URBs fast enough in your setup.

Thanks.

-- 
Dmitry
--
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: usb/gadget: stalls in dummy_timer

[Dmitry Torokhov]

On Mon, Sep 11, 2017 at 8:15 AM, Andrey Konovalov  wrote:
> On Mon, Sep 11, 2017 at 3:25 PM, Alan Stern  wrote:
>> On Mon, 11 Sep 2017, Andrey Konovalov wrote:
>>
>>> Hi!
>>>
>>> I've been getting stall reports like this one while fuzzing the USB
>>> stack with gadgetfs. I'm wondering whether this is a bug in gadgetfs
>>> or is this report induced by the changes I've made to the USB core
>>> code. I didn't touch gadgetfs code though (except for adding a few
>>> printk's).
>>>
>>> I'm on commit 81a84ad3cb5711cec79f4dd53a4ce026b092c432
>>
>> It's possible that this was caused by commit f16443a034c7 ("USB:
>> gadgetfs, dummy-hcd, net2280: fix locking for callbacks").  I've been
>> meaning to repair the commit but haven't done it yet.
>>
>> Can you test with that commit reverted?  You may end up seeing other
>> problems instead -- the ones that commit was intended to solve -- but
>> perhaps the stalls won't occur.
>
> So I've reverted both: the changes I made to USB core and the commit
> you mentioned, still saw the stalls.
>
> I've manged to find a way to reproduce this and now I'm not sure the
> problem is actually in gadgetfs, it might be the usbtouchscreen
> driver.
>
> The crash log is below.
>
> Thanks!
>
> gadgetfs: bound to dummy_udc driver
> usb 1-1: new full-speed USB device number 2 using dummy_hcd
> gadgetfs: connected
> gadgetfs: disconnected
> gadgetfs: connected
> usb 1-1: config 8 interface 0 altsetting 9 endpoint 0x8F has an
> invalid bInterval 0, changing to 10
> usb 1-1: config 8 interface 0 altsetting 9 endpoint 0x8F has invalid
> maxpacket 839, setting to 64
> usb 1-1: config 8 interface 0 altsetting 9 endpoint 0x7 has invalid
> maxpacket 1839, setting to 64
> usb 1-1: config 8 interface 0 has no altsetting 0
> usb 1-1: New USB device found, idVendor=0403, idProduct=f9e9
> usb 1-1: New USB device strings: Mfr=4, Product=8, SerialNumber=255
> usb 1-1: Product: a
> usb 1-1: Manufacturer: a
> usb 1-1: SerialNumber: a
> gadgetfs: configuration #8
> input: a a as /devices/platform/dummy_hcd.0/usb1/1-1/1-1:8.0/input/input8
> evbug: Connected device: input8 (a a at usb-dummy_hcd.0-1/input0)
> kworker/0:0: page allocation failure: order:0,
> mode:0x1280020(GFP_ATOMIC|__GFP_NOTRACK), nodemask=(null)
> kworker/0:0 cpuset=/ mems_allowed=0

It seems you are running out of memory.

> Swap cache stats: add 0, delete 0, find 0/0
> Free swap  = 0kB
> Total swap = 0kB

And no swap. I think you need to give the box a bit more memory (or
there might be a leak somewhere).

Thanks.

-- 
Dmitry
--
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

[PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Dmitry Torokhov]

From: Benson Leung <ble...@chromium.org>

usbhid->intf->needs_remote_wakeup is set when a device is opened, and is
cleared when a device is closed.

In usbhid_open, usb_autopm_get_interface is called before setting the
needs_remote_wakeup flag, and usb_autopm_put_interface is called after
hid_start_in. However, when the device is closed in usbhid_close, we
simply reset the flag and the device stays awake even though it could be
suspended.

This patch adds calls to usb_autopm_get_interface and
usb_autopm_put_interface when we reset usbhid->intf->needs_remote_wakeup
flag, giving the system chance to put the device to sleep.

Signed-off-by: Benson Leung <ble...@chromium.org>
Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---
 drivers/hid/usbhid/hid-core.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8a9a21..174d87f0e3e6 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -729,6 +729,7 @@ static int usbhid_open(struct hid_device *hid)
 static void usbhid_close(struct hid_device *hid)
 {
struct usbhid_device *usbhid = hid->driver_data;
+   int autopm_error;
 
/*
 * Make sure we don't restart data acquisition due to
@@ -745,8 +746,14 @@ static void usbhid_close(struct hid_device *hid)
return;
 
hid_cancel_delayed_stuff(usbhid);
+
+   autopm_error = usb_autopm_get_interface(usbhid->intf);
+
usb_kill_urb(usbhid->urbin);
usbhid->intf->needs_remote_wakeup = 0;
+
+   if (!autopm_error)
+   usb_autopm_put_interface(usbhid->intf);
 }
 
 /*
@@ -1176,13 +1183,18 @@ static int usbhid_start(struct hid_device *hid)
 static void usbhid_stop(struct hid_device *hid)
 {
struct usbhid_device *usbhid = hid->driver_data;
+   int autopm_error;
 
if (WARN_ON(!usbhid))
return;
 
if (hid->quirks & HID_QUIRK_ALWAYS_POLL) {
clear_bit(HID_IN_POLLING, >iofl);
+
+   autopm_error = usb_autopm_get_interface(usbhid->intf);
usbhid->intf->needs_remote_wakeup = 0;
+   if (!autopm_error)
+   usb_autopm_put_interface(usbhid->intf);
}
 
clear_bit(HID_STARTED, >iofl);
-- 
2.14.1.581.gf28d330327-goog


-- 
Dmitry
--
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

[PATCH] HID: usbhid: fix "always poll" quirk

[Dmitry Torokhov]

Even though the IO for devices with "always poll" quirk is already running,
we still need to set HID_OPENED bit in usbhid->iofl so the interrupt
handler does not ignore the data coming from the device.

Reported-by: Olof Johansson <o...@lixom.net>
Tested-by: Olof Johansson <o...@lixom.net>
Fixes: e399396a6b0 ("HID: usbhid: remove custom locking from usbhid_open...")
Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---
 drivers/hid/usbhid/hid-core.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 76013eb5cb7f..c008847e0b20 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -680,18 +680,21 @@ static int usbhid_open(struct hid_device *hid)
struct usbhid_device *usbhid = hid->driver_data;
int res;
 
+   set_bit(HID_OPENED, >iofl);
+
if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
return 0;
 
res = usb_autopm_get_interface(usbhid->intf);
/* the device must be awake to reliably request remote wakeup */
-   if (res < 0)
+   if (res < 0) {
+   clear_bit(HID_OPENED, >iofl);
return -EIO;
+   }
 
usbhid->intf->needs_remote_wakeup = 1;
 
set_bit(HID_RESUME_RUNNING, >iofl);
-   set_bit(HID_OPENED, >iofl);
set_bit(HID_IN_POLLING, >iofl);
 
res = hid_start_in(hid);
@@ -727,19 +730,20 @@ static void usbhid_close(struct hid_device *hid)
 {
struct usbhid_device *usbhid = hid->driver_data;
 
-   if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
-   return;
-
/*
 * Make sure we don't restart data acquisition due to
 * a resumption we no longer care about by avoiding racing
 * with hid_start_in().
 */
spin_lock_irq(>lock);
-   clear_bit(HID_IN_POLLING, >iofl);
clear_bit(HID_OPENED, >iofl);
+   if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL))
+   clear_bit(HID_IN_POLLING, >iofl);
spin_unlock_irq(>lock);
 
+   if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
+   return;
+
hid_cancel_delayed_stuff(usbhid);
usb_kill_urb(usbhid->urbin);
usbhid->intf->needs_remote_wakeup = 0;
-- 
2.13.2.932.g7449e964c-goog


-- 
Dmitry
--
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 3/3] mfd: twl: move header file out of I2C realm

[Dmitry Torokhov]

On Mon, May 22, 2017 at 12:02:10AM +0200, Wolfram Sang wrote:
> include/linux/i2c is not for client devices. Move the header file to a
> more appropriate location.
> 
> Signed-off-by: Wolfram Sang <w...@the-dreams.de>
> ---
>  arch/arm/mach-omap2/common.h| 2 +-
>  arch/arm/mach-omap2/omap_twl.c  | 2 +-
>  drivers/gpio/gpio-twl4030.c | 2 +-
>  drivers/iio/adc/twl4030-madc.c  | 2 +-
>  drivers/iio/adc/twl6030-gpadc.c | 2 +-
>  drivers/input/keyboard/twl4030_keypad.c | 2 +-
>  drivers/input/misc/twl4030-pwrbutton.c  | 2 +-
>  drivers/input/misc/twl4030-vibra.c  | 2 +-

Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

>  drivers/mfd/twl-core.c  | 6 +++---
>  drivers/mfd/twl4030-audio.c | 2 +-
>  drivers/mfd/twl4030-irq.c   | 2 +-
>  drivers/mfd/twl4030-power.c | 2 +-
>  drivers/mfd/twl6030-irq.c   | 2 +-
>  drivers/phy/phy-twl4030-usb.c   | 2 +-
>  drivers/power/supply/twl4030_charger.c  | 2 +-
>  drivers/pwm/pwm-twl-led.c   | 2 +-
>  drivers/pwm/pwm-twl.c   | 2 +-
>  drivers/regulator/twl-regulator.c   | 2 +-
>  drivers/regulator/twl6030-regulator.c   | 2 +-
>  drivers/rtc/rtc-twl.c   | 2 +-
>  drivers/usb/phy/phy-twl6030-usb.c   | 2 +-
>  drivers/video/backlight/pandora_bl.c| 2 +-
>  drivers/watchdog/twl4030_wdt.c  | 2 +-
>  include/linux/{i2c => mfd}/twl.h| 0
>  sound/soc/codecs/twl4030.c  | 2 +-
>  25 files changed, 26 insertions(+), 26 deletions(-)
>  rename include/linux/{i2c => mfd}/twl.h (100%)
> 
> diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> index 8cc6338fcb1288..b5ad7fcb80ed24 100644
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -29,7 +29,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/arch/arm/mach-omap2/omap_twl.c b/arch/arm/mach-omap2/omap_twl.c
> index 1346b3ab34a5e3..295124b248ae3f 100644
> --- a/arch/arm/mach-omap2/omap_twl.c
> +++ b/arch/arm/mach-omap2/omap_twl.c
> @@ -16,7 +16,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  
>  #include "soc.h"
>  #include "voltage.h"
> diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
> index 24f388ed46d4c4..9b511df5450eb6 100644
> --- a/drivers/gpio/gpio-twl4030.c
> +++ b/drivers/gpio/gpio-twl4030.c
> @@ -35,7 +35,7 @@
>  #include 
>  #include 
>  
> -#include 
> +#include 
>  
>  /*
>   * The GPIO "subchip" supports 18 GPIOs which can be configured as
> diff --git a/drivers/iio/adc/twl4030-madc.c b/drivers/iio/adc/twl4030-madc.c
> index 0c74869a540ad3..5a64eda1652061 100644
> --- a/drivers/iio/adc/twl4030-madc.c
> +++ b/drivers/iio/adc/twl4030-madc.c
> @@ -35,7 +35,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c
> index becbb0aef232b9..bc0e60b9da452e 100644
> --- a/drivers/iio/adc/twl6030-gpadc.c
> +++ b/drivers/iio/adc/twl6030-gpadc.c
> @@ -33,7 +33,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  
> diff --git a/drivers/input/keyboard/twl4030_keypad.c 
> b/drivers/input/keyboard/twl4030_keypad.c
> index 39e72b3219d8a4..f9f98ef1d98e3f 100644
> --- a/drivers/input/keyboard/twl4030_keypad.c
> +++ b/drivers/input/keyboard/twl4030_keypad.c
> @@ -30,7 +30,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  
> diff --git a/drivers/input/misc/twl4030-pwrbutton.c 
> b/drivers/input/misc/twl4030-pwrbutton.c
> index 1c13005b228fa7..b307cca1702226 100644
> --- a/drivers/input/misc/twl4030-pwrbutton.c
> +++ b/drivers/input/misc/twl4030-pwrbutton.c
> @@ -27,7 +27,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  
>  #define PWR_PWRON_IRQ (1 << 0)
>  
> diff --git a/drivers/input/misc/twl4030-vibra.c 
> b/drivers/input/misc/twl4030-vibra.c
> index caa5a62c42fbe0..6c51d404874bbd 100644
> --- a/drivers/input/misc/twl4030-vibra.c
> +++ b/drivers/input/misc/twl4030-vibra.c
> @@ -28,7 +28,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index c64615dca2bd33..2a09dde4ca6efc 100644
> --- a/drivers/mfd/twl-core.c
>

Re: [PATCH 0/7] Input: fix NULL-derefs at probe

[Dmitry Torokhov]

On Fri, Mar 17, 2017 at 11:53:37AM +0100, Johan Hovold wrote:
> On Thu, Mar 16, 2017 at 03:37:28PM -0700, Dmitry Torokhov wrote:
> > On Mon, Mar 13, 2017 at 04:45:52PM +0100, Johan Hovold wrote:
> > > On Mon, Mar 13, 2017 at 04:15:18PM +0100, Oliver Neukum wrote:
> > > > Am Montag, den 13.03.2017, 13:35 +0100 schrieb Johan Hovold:
> > > > > This series fixes a number of NULL-pointer dereferences due to
> > > > > missing endpoint sanity checks that can be triggered by a
> > > > > malicious USB device.
> 
> > Applied the lot.
> 
> I noticed you dropped the Fixes tag from the patches that fix bugs which
> predate git. While this is probably not much of an issue in this case, I
> think it's generally a bad idea since we're loosing information this
> way, and this specifically makes it harder for the stable maintainers to
> figure out which tree to backport a fix to.

As far as I know the rule is: if no special markings then stable patch
should be applied as far as it can go.

There is no reason to say specify 2.6.12 commit, as in fact the
offending change is likely to be even earlier, so the annotation would
be effectively wrong.

Thanks.

-- 
Dmitry
--
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 0/7] Input: fix NULL-derefs at probe

[Dmitry Torokhov]

On Mon, Mar 13, 2017 at 04:45:52PM +0100, Johan Hovold wrote:
> On Mon, Mar 13, 2017 at 04:15:18PM +0100, Oliver Neukum wrote:
> > Am Montag, den 13.03.2017, 13:35 +0100 schrieb Johan Hovold:
> > > This series fixes a number of NULL-pointer dereferences due to
> > > missing
> > > endpoint sanity checks that can be triggered by a malicious USB
> > > device.
> >
> > At the risk of repeating myself, doesn't the sheer number of fixes
> > demonstrate the need for a more centralized check?
> 
> No, I don't think that follows. These are plain bugs that needs to be
> fixed (cf. not checking for allocation failures or whatever) and
> backported to the stable trees.
> 
> I think I may have surveyed just about every USB driver this last week,
> and there is no single pattern for how endpoints are verified and
> retrieved that could easily be refactored into USB core.
> 
> Now there are certain patterns that could benefit from a few helpers,
> and some obvious bugs could then be caught by declaring those helpers as
> __must_check. But specifically, you'd still be checking the return
> value from the helpers.
> 
> Then verifying the endpoint counts before calling driver probe,
> typically only saves a bit of time while probing *malicious* devices
> (and the occasional odd interface which cannot be matched on other
> attributes).
> 
> That being said, we could still add a centralised sanity check for a
> large class of drivers (e.g. that do not use altsettings and only need
> minimum constraints) but it's not going to obviate the need for careful
> driver implementations.
> 
> I'll be posting some more patches related to this shortly.

There were some discussions about making and that would allow drivers
declare endpoints they want and have USB core ether fill them or not
even try to bind, but nothing concrete.

Anyway, I do not think we should be blocking this patch series on it; if
we come up with something clever we can always switch over.

Applied the lot.

Thanks.

-- 
Dmitry
--
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] hid: usbhid: usbkbd: fix checkpatch.pl issues

[Dmitry Torokhov]

On Wed, Mar 01, 2017 at 09:59:31PM +0200, Avraham Shukron wrote:
> > 
> > This kind of change is definitely not helpful. The original table was
> > Nx16, you converted it to Nx14. Why do you think original table used 16
> > columns?
> > 
> > Regardless, it's a very old driver, just let it be.
> > 
> > Thanks.
> > 
> 
> I can make it Nx8 :)

Or you can leave it as is.

> 
> Seriously now - I don't understand what is so wrong with checkpatch fixes?

Checkpatch is a tool to make sure new code follows standard conversions,
not reshuffling old working code.

> I'm a new to kernel development, and the natural place to start is to do some
> coding style fixes.
> I thought fixing a driver that I actually use daily will be more satisfying.

You are not using this driver daily, pretty much nobody does. What you
are using is usbhid + hid-input + probably some hardware-specific hid
driver that twiddles the behavior of your keyboard.

> Why driver being old is a good reason to ignore the coding style conventions?

Since there is no active development nor use it is easy to introduce
bugs that won't be caught until much later. Checkpatch fixes are usually
welcome when there are additional fixes to the same driver.

Thanks.

-- 
Dmitry
--
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] hid: usbhid: usbkbd: fix checkpatch.pl issues

[Dmitry Torokhov]

Hi Avraham,

On Tue, Feb 21, 2017 at 07:26:50PM +0200, Avraham Shukron wrote:
> - Broke long lines
> - Added spaces where needed
> - Removed unnecessary / trailing whitespaces
> - Extracted assignments outside of 'if' statements
> 
> Signed-off-by: Avraham Shukron 
> ---
>  drivers/hid/usbhid/usbkbd.c | 121 
> ++--
>  1 file changed, 72 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/usbkbd.c b/drivers/hid/usbhid/usbkbd.c
> index 7fb2d1e..ae40b0d 100644
> --- a/drivers/hid/usbhid/usbkbd.c
> +++ b/drivers/hid/usbhid/usbkbd.c
> @@ -45,22 +45,24 @@ MODULE_DESCRIPTION(DRIVER_DESC);
>  MODULE_LICENSE("GPL");
> 
>  static const unsigned char usb_kbd_keycode[256] = {
> -   0,  0,  0,  0, 30, 48, 46, 32, 18, 33, 34, 35, 23, 36, 37, 38,
> -  50, 49, 24, 25, 16, 19, 31, 20, 22, 47, 17, 45, 21, 44,  2,  3,
> -   4,  5,  6,  7,  8,  9, 10, 11, 28,  1, 14, 15, 57, 12, 13, 26,
> -  27, 43, 43, 39, 40, 41, 51, 52, 53, 58, 59, 60, 61, 62, 63, 64,
> -  65, 66, 67, 68, 87, 88, 99, 70,119,110,102,104,111,107,109,106,
> - 105,108,103, 69, 98, 55, 74, 78, 96, 79, 80, 81, 75, 76, 77, 71,
> -  72, 73, 82, 83, 86,127,116,117,183,184,185,186,187,188,189,190,
> - 191,192,193,194,134,138,130,132,128,129,131,137,133,135,136,113,
> - 115,114,  0,  0,  0,121,  0, 89, 93,124, 92, 94, 95,  0,  0,  0,
> - 122,123, 90, 91, 85,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -   0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -   0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -   0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -   0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
> -  29, 42, 56,125, 97, 54,100,126,164,166,165,163,161,115,114,113,
> - 150,158,159,128,136,177,178,176,142,152,173,140
> +   0,   0,   0,   0,  30,  48,  46,  32,  18,  33,  34,  35,  23,  36,
> +  37,  38,  50,  49,  24,  25,  16,  19,  31,  20,  22,  47,  17,  45,
> +  21,  44,   2,   3,   4,   5,   6,   7,   8,   9,  10,  11,  28,   1,
> +  14,  15,  57,  12,  13,  26,  27,  43,  43,  39,  40,  41,  51,  52,
> +  53,  58,  59,  60,  61,  62,  63,  64,  65,  66,  67,  68,  87,  88,
> +  99,  70, 119, 110, 102, 104, 111, 107, 109, 106, 105, 108, 103,  69,
> +  98,  55,  74,  78,  96,  79,  80,  81,  75,  76,  77,  71,  72,  73,
> +  82,  83,  86, 127, 116, 117, 183, 184, 185, 186, 187, 188, 189, 190,
> + 191, 192, 193, 194, 134, 138, 130, 132, 128, 129, 131, 137, 133, 135,
> + 136, 113, 115, 114,   0,   0,   0, 121,   0,  89,  93, 124,  92,  94,
> +  95,   0,   0,   0, 122, 123,  90,  91,  85,   0,   0,   0,   0,   0,
> +   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,
> +  29,  42,  56, 125,  97,  54, 100, 126, 164, 166, 165, 163, 161, 115,
> + 114, 113, 150, 158, 159, 128, 136, 177, 178, 176, 142, 152, 173, 140
>  };

This kind of change is definitely not helpful. The original table was
Nx16, you converted it to Nx14. Why do you think original table used 16
columns?

Regardless, it's a very old driver, just let it be.

Thanks.

-- 
Dmitry
--
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: [RFC PATCH] usb: core: correct usb_get_dev() documentation

[Dmitry Torokhov]

On Thu, Oct 27, 2016 at 03:02:30PM -0700, Brian Norris wrote:
> In reading through a USB interface driver, I noticed that it called
> usb_{get,put}_dev() in its probe() and disconnect() methods. This seemed
> unnecessary, but a look at the comments here matched the usage.
> 
> USB interface devices seem to be well covered by the parent/child
> relationship of the device model, and so it should be unnecessary for a
> child device to grab a refcount on its parent device.
> 
> Signed-off-by: Brian Norris <computersforpe...@gmail.com>

Yes, usb_device is parent of usb_interface and device core does "parent
= get_device(dev->parent);" as part of device_add() when registering new
interfaces.

Reviewed-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

> ---
> This reflects my understanding (and testing), as well as the majority of usage
> -- there are *very* few interface drivers that actually call usb_get_dev(). If
> I'm wrong, please feel free to tell me so! But I thought patching the
> documentation would be the best way to solicit a response :)
> 
>  drivers/usb/core/usb.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 592151461017..0ba7e070f04e 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -539,9 +539,9 @@ EXPORT_SYMBOL_GPL(usb_alloc_dev);
>   *
>   * Each live reference to a device should be refcounted.
>   *
> - * Drivers for USB interfaces should normally record such references in
> - * their probe() methods, when they bind to an interface, and release
> - * them by calling usb_put_dev(), in their disconnect() methods.
> + * The device driver core automatically handles this refcounting for USB
> + * interface drivers, but this API can be used for non-parent/child
> + * relationships.
>   *
>   * Return: A pointer to the device with the incremented reference counter.
>   */
> -- 
> 2.8.0.rc3.226.g39d4020
> 

-- 
Dmitry
--
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 v2] input: tablet: pegasus_notetaker: USB PM fixes

[Dmitry Torokhov]

On Tue, Jun 28, 2016 at 06:17:13PM +0200, Martin Kepplinger wrote:
> Am 2016-06-23 um 19:18 schrieb Dmitry Torokhov:
> > Hi Martin,
> > 
> > On Tue, Jun 14, 2016 at 01:20:15PM +0200, Martin Kepplinger wrote:
> >>  static int pegasus_reset_resume(struct usb_interface *intf)
> >>  {
> >> +  struct pegasus *pegasus = usb_get_intfdata(intf);
> >> +
> >> +  if (pegasus->dev->users)
> >> +  pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> >> +
> >>return pegasus_resume(intf);
> > 
> > Hmm, we need to take input mutex when using pegasus->dev->users, how
> > about the version below instead?
> > 
> > Thanks.
> > 
> 
> Sorry for the delay, give me a few more days to test and confirm this or
> come up with a final patch.

Martin, did you have time to try out this version of the patch?

Thanks!

-- 
Dmitry
--
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 v2] input: tablet: pegasus_notetaker: USB PM fixes

[Dmitry Torokhov]

Hi Martin,

On Tue, Jun 14, 2016 at 01:20:15PM +0200, Martin Kepplinger wrote:
>  static int pegasus_reset_resume(struct usb_interface *intf)
>  {
> + struct pegasus *pegasus = usb_get_intfdata(intf);
> +
> + if (pegasus->dev->users)
> + pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> +
>   return pegasus_resume(intf);

Hmm, we need to take input mutex when using pegasus->dev->users, how
about the version below instead?

Thanks.

-- 
Dmitry

Input: pegasus_notetake - USB PM fixes

From: Martin Kepplinger <mart...@posteo.de>

* Fix usb_autopm calls to be balanced
 * In reset_resume() we need to set the device mode
 * In suspend() we must cancel the workqueue's work

Signed-off-by: Martin Kepplinger <mart...@posteo.de>
Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---
 drivers/input/tablet/pegasus_notetaker.c |   46 --
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/input/tablet/pegasus_notetaker.c 
b/drivers/input/tablet/pegasus_notetaker.c
index 805afe3..d20ea06 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -80,7 +80,7 @@ struct pegasus {
struct work_struct init;
 };
 
-static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
+static int pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
 {
const int sizeof_buf = len + 2;
int result;
@@ -88,7 +88,7 @@ static void pegasus_control_msg(struct pegasus *pegasus, u8 
*data, int len)
 
cmd_buf = kmalloc(sizeof_buf, GFP_KERNEL);
if (!cmd_buf)
-   return;
+   return -ENOMEM;
 
cmd_buf[0] = NOTETAKER_REPORT_ID;
cmd_buf[1] = len;
@@ -101,17 +101,23 @@ static void pegasus_control_msg(struct pegasus *pegasus, 
u8 *data, int len)
 0, 0, cmd_buf, sizeof_buf,
 USB_CTRL_SET_TIMEOUT);
 
-   if (result != sizeof_buf)
-   dev_err(>usbdev->dev, "control msg error\n");
+   if (result != sizeof_buf) {
+   if (result >= 0)
+   result = -EIO;
+   dev_err(>usbdev->dev, "control msg error: %d\n",
+   result);
+   }
 
kfree(cmd_buf);
+
+   return result;
 }
 
-static void pegasus_set_mode(struct pegasus *pegasus, u8 mode, u8 led)
+static int pegasus_set_mode(struct pegasus *pegasus, u8 mode, u8 led)
 {
u8 cmd[] = { NOTETAKER_SET_CMD, NOTETAKER_SET_MODE, led, mode };
 
-   pegasus_control_msg(pegasus, cmd, sizeof(cmd));
+   return pegasus_control_msg(pegasus, cmd, sizeof(cmd));
 }
 
 static void pegasus_parse_packet(struct pegasus *pegasus)
@@ -205,27 +211,24 @@ static int pegasus_open(struct input_dev *dev)
return retval;
 
pegasus->irq->dev = pegasus->usbdev;
-   if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
+   if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
retval = -EIO;
+   goto out;
+   }
 
-   pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
+   retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
 
+out:
usb_autopm_put_interface(pegasus->intf);
-
return retval;
 }
 
 static void pegasus_close(struct input_dev *dev)
 {
struct pegasus *pegasus = input_get_drvdata(dev);
-   int autopm_error;
 
-   autopm_error = usb_autopm_get_interface(pegasus->intf);
usb_kill_urb(pegasus->irq);
cancel_work_sync(>init);
-
-   if (!autopm_error)
-   usb_autopm_put_interface(pegasus->intf);
 }
 
 static int pegasus_probe(struct usb_interface *intf,
@@ -373,6 +376,7 @@ static int pegasus_suspend(struct usb_interface *intf, 
pm_message_t message)
 
mutex_lock(>dev->mutex);
usb_kill_urb(pegasus->irq);
+   cancel_work_sync(>init);
mutex_unlock(>dev->mutex);
 
return 0;
@@ -393,7 +397,19 @@ static int pegasus_resume(struct usb_interface *intf)
 
 static int pegasus_reset_resume(struct usb_interface *intf)
 {
-   return pegasus_resume(intf);
+   struct pegasus *pegasus = usb_get_intfdata(intf);
+   int retval = 0;
+
+   mutex_lock(>dev->mutex);
+   if (pegasus->dev->users) {
+   retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
+ NOTETAKER_LED_MOUSE);
+   if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
+   retval = -EIO;
+   }
+   mutex_unlock(>dev->mutex);
+
+   return retval;
 }
 
 static const struct usb_device_id pegasus_ids[] = {
--
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 v7] input: tablet: add Pegasus Notetaker tablet driver

[Dmitry Torokhov]

Hi Martin,

On Wed, Jun 01, 2016 at 02:55:03PM +0200, Martin Kepplinger wrote:
> This adds a driver for the Pegasus Notetaker Pen. When connected,
> this uses the Pen as an input tablet.
> 
> This device was sold in various different brandings, for example
>   "Pegasus Mobile Notetaker M210",
>   "Genie e-note The Notetaker",
>   "Staedtler Digital ballpoint pen 990 01",
>   "IRISnotes Express" or
>   "NEWLink Digital Note Taker".
> 
> Here's an example, so that you know what we are talking about:
> http://www.genie-online.de/genie-e-note-2/
> 
> https://pegatech.blogspot.com/ seems to be a remaining official resource.
> 
> This device can also transfer saved (offline recorded handwritten) data and
> there are userspace programs that do this, see https://launchpad.net/m210
> (Well, alternatively there are really fast scanners out there :)
> 
> It's *really* fun to use as an input tablet though! So let's support this
> for everybody.
> 
> Signed-off-by: Martin Kepplinger 
> ---
> 
> I thought about PM again and I think this is how it's supposed to be, and
> how it's done in many other usb input drivers.
> 
> I'm running it and don't have any problems. It feels more finished now.
> 
> Any objections? thanks!

2 more tiny nits, and I think I'll be able to merge this.

> 
> revision history
> 
> v7 add usb_driver power management
> v6 minor cleanup (thanks Dmitry)
> v5 fix various bugs (thanks Oliver and Dmitry)
> v4 use normal work queue instead of a kernel thread (thanks to Oliver Neukum)
> v3 fix reporting low pen battery and add USB list to CC
> v2 minor cleanup (remove unnecessary variables)
> v1 initial release
> 
> 
>  drivers/input/tablet/Kconfig |  15 ++
>  drivers/input/tablet/Makefile|   1 +
>  drivers/input/tablet/pegasus_notetaker.c | 412 
> +++
>  3 files changed, 428 insertions(+)
>  create mode 100644 drivers/input/tablet/pegasus_notetaker.c
> 
> diff --git a/drivers/input/tablet/Kconfig b/drivers/input/tablet/Kconfig
> index 623bb9e..a2b9f97 100644
> --- a/drivers/input/tablet/Kconfig
> +++ b/drivers/input/tablet/Kconfig
> @@ -73,6 +73,21 @@ config TABLET_USB_KBTAB
> To compile this driver as a module, choose M here: the
> module will be called kbtab.
>  
> +config TABLET_USB_PEGASUS
> + tristate "Pegasus Mobile Notetaker Pen input tablet support"
> + depends on USB_ARCH_HAS_HCD
> + select USB
> + help
> +   Say Y here if you want to use the Pegasus Mobile Notetaker,
> +   also known as:
> +   Genie e-note The Notetaker,
> +   Staedtler Digital ballpoint pen 990 01,
> +   IRISnotes Express or
> +   NEWLink Digital Note Taker.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called pegasus_notetaker.
> +
>  config TABLET_SERIAL_WACOM4
>   tristate "Wacom protocol 4 serial tablet support"
>   select SERIO
> diff --git a/drivers/input/tablet/Makefile b/drivers/input/tablet/Makefile
> index 2e13010..200fc4e 100644
> --- a/drivers/input/tablet/Makefile
> +++ b/drivers/input/tablet/Makefile
> @@ -8,4 +8,5 @@ obj-$(CONFIG_TABLET_USB_AIPTEK)   += aiptek.o
>  obj-$(CONFIG_TABLET_USB_GTCO)+= gtco.o
>  obj-$(CONFIG_TABLET_USB_HANWANG) += hanwang.o
>  obj-$(CONFIG_TABLET_USB_KBTAB)   += kbtab.o
> +obj-$(CONFIG_TABLET_USB_PEGASUS) += pegasus_notetaker.o
>  obj-$(CONFIG_TABLET_SERIAL_WACOM4) += wacom_serial4.o
> diff --git a/drivers/input/tablet/pegasus_notetaker.c 
> b/drivers/input/tablet/pegasus_notetaker.c
> new file mode 100644
> index 000..f403494
> --- /dev/null
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -0,0 +1,412 @@
> +/*
> + * Pegasus Mobile Notetaker Pen input tablet driver
> + *
> + * Copyright (c) 2016 Martin Kepplinger 
> + */
> +
> +/*
> + * request packet (control endpoint):
> + * |-|
> + * | Report ID | Nr of bytes | command   |
> + * | (1 byte)  | (1 byte)| (n bytes) |
> + * |-|
> + * | 0x02  | n   |   |
> + * |-|
> + *
> + * data packet after set xy mode command, 0x80 0xb5 0x02 0x01
> + * and pen is in range:
> + *
> + * byte  byte name   value (bits)
> + * 
> + * 0 status  0 1 0 0 0 0 X X
> + * 1 color   0 0 0 0 H 0 S T
> + * 2 X low
> + * 3 X high
> + * 4 Y low
> + * 5 Y high
> + *
> + * X X   battery state:
> + *   no state reported   0x00
> + *   battery low 0x01
> + *   battery good0x02
> + *
> + * H Hovering
> + * S Switch 1 (pen button)
> + * T Tip
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* USB HID defines */
> +#define USB_REQ_GET_REPORT   0x01
> +#define USB_REQ_SET_REPORT   0x09
> +
> +#define USB_VENDOR_ID_PEGASUSTECH  

Re: [PATCH v5] input: tablet: add Pegasus Notetaker tablet driver

[Dmitry Torokhov]

Hi Martin,

On Fri, May 27, 2016 at 11:46:21AM +0200, Martin Kepplinger wrote:
> This adds a driver for the Pegasus Notetaker Pen. When connected,
> this uses the Pen as an input tablet.
> 
> This device was sold in various different brandings, for example
>   "Pegasus Mobile Notetaker M210",
>   "Genie e-note The Notetaker",
>   "Staedtler Digital ballpoint pen 990 01",
>   "IRISnotes Express" or
>   "NEWLink Digital Note Taker".
> 
> Here's an example, so that you know what we are talking about:
> http://www.staedtler.com/en/products/ink-writing-instruments/ballpoint-pens/digital-pen-990-01-digital-ballpoint-pen
> 
> http://pegatech.blogspot.com/ seems to be a remaining official resource.
> 
> This device can also transfer saved (offline recorded handwritten) data and
> there are userspace programs that do this, see https://launchpad.net/m210
> (Well, alternatively there are really fast scanners out there :)
> 
> It's *really* fun to use as an input tablet though! So let's support this
> for everybody.
> 
> There's no way to disable the device. When the pen is out of range, we just
> don't get any URBs and don't do anything.
> Like all other mouses or input tablets, we don't use runtime PM.
> 
> Signed-off-by: Martin Kepplinger 
> ---
> 
> Thanks for reviewing! Dmitry's and Oliver's changes to v4 made it even
> smaller now. All is tested again and should apply to any recent tree.
> If not, please just complain.

Couple of minor comments:

> +
> +static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
> +{
> + const int sizeof_buf = len * sizeof(u8) + 2;

Just do "len + 2".

> +static void pegasus_parse_packet(struct pegasus *pegasus)
> +{
> + unsigned char *data = pegasus->data;
> + struct input_dev *dev = pegasus->dev;
> + u16 x, y;
> +
> + switch (data[0]) {
> + case SPECIAL_COMMAND:
> + /* device button pressed */
> + if (data[1] == BUTTON_PRESSED)
> + schedule_work(>init);
> +
> + break;
> + /* xy data */
> + case BATTERY_LOW:
> + dev_warn_once(>dev, "Pen battery low\n");
> + case BATTERY_NO_REPORT:
> + case BATTERY_GOOD:
> + x = le16_to_cpup((__le16 *)[2]);
> + y = le16_to_cpup((__le16 *)[4]);
> +
> + /* ignore pen up events */
> + if (x == 0 && y == 0)

Why are we ignoring pen-up events? I'd at least send
EV_KEY/BTN_TOOL_PEN/0 and maybe the rest of BTN* events.

> + break;
> +
> + input_report_key(dev, BTN_TOUCH, data[1] & PEN_TIP);
> + input_report_key(dev, BTN_RIGHT, data[1] & PEN_BUTTON_PRESSED);
> + input_report_key(dev, BTN_TOOL_PEN, 1);
> + input_report_abs(dev, ABS_X, (s16)x);
> + input_report_abs(dev, ABS_Y, y);
> +
> + input_sync(dev);
> + break;
> + default:
> + dev_warn_once(>usbdev->dev,
> +   "unknown answer from device\n");
> + }
> +}
> +
> +static void pegasus_irq(struct urb *urb)
> +{
> + struct pegasus *pegasus = urb->context;
> + struct usb_device *dev = pegasus->usbdev;
> + int retval;
> +
> + switch (urb->status) {
> + case 0:
> + pegasus_parse_packet(pegasus);
> + usb_mark_last_busy(pegasus->usbdev);

Since the driver does not use runtime-PM I do not think you need to call
usb_mark_last_busy().

> +static int pegasus_probe(struct usb_interface *intf,
> +  const struct usb_device_id *id)
> +{
> + struct usb_device *dev = interface_to_usbdev(intf);
> + struct usb_endpoint_descriptor *endpoint;
> + struct pegasus *pegasus;
> + struct input_dev *input_dev;
> + int error;
> + int pipe, maxp;
> +
> + /* we control interface 0 */
> + if (intf->cur_altsetting->desc.bInterfaceNumber == 1)
> + return -ENODEV;

Please add:

/* Sanity check that the device has an endpoint */
if (usbinterface->altsetting[0].desc.bNumEndpoints < 1) {
dev_err(>dev, "Invalid number of endpoints\n");
return -EINVAL;
}

Thanks.

-- 
Dmitry
--
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: [Linux-v4.6-rc2] atkbd serio0: Unknown key pressed (translated set 2, code 0xa8 on isa0060/serio0).

[Dmitry Torokhov]

On Wed, Apr 6, 2016 at 3:27 AM, Sedat Dilek  wrote:
> On Wed, Apr 6, 2016 at 11:35 AM, Sedat Dilek  wrote:
>> On Wed, Apr 6, 2016 at 11:27 AM, Sedat Dilek  wrote:
>>> Hi,
>>>
>>> I cannot use cursor up and down on my notebook and see this in my dmesg.
>>>
>>> [  685.425634] atkbd serio0: Unknown key pressed (translated set 2,
>>> code 0xa8 on isa0060/serio0).
>>> [  685.425648] atkbd serio0: Use 'setkeycodes e028 ' to make it 
>>> known.
>>>
>>> Known issue?
>>>
>>> Do you need more input?
>>>
>>> Linux-config and dmesg-output attached.
>>>
>>> Regards,
>>> - Sedat Dilek -
>>
>> Attached is output of...
>>
>>$ xmodmap -pke > /tmp/xmodmap-pke.txt
>>
>
> [ CC (USB)HID folks ]
>
> I forgot to tell that I have some patches on top of vanilla Linux v4.6-rc2.
>
> Dropping the patches from  makes
> the issue go away.
> Unsure what is the cause.

Hmm, this is quite curious, none of the recent patches in
for-4.6/upstream-fixes touches atkbd. I'd be very interested in which
one exactly causes this.

Thanks.

-- 
Dmitry
--
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 1/4] acecad: stop saving struct usb_device

[Dmitry Torokhov]

On Tue, Mar 22, 2016 at 04:04:54PM +0100, Oliver Neukum wrote:
>  static int usb_acecad_open(struct input_dev *dev)
>  {
>   struct usb_acecad *acecad = input_get_drvdata(dev);
>  
> - acecad->irq->dev = acecad->usbdev;
> + acecad->irq->dev = interface_to_usbdev(acecad->intf);

By the way, do we still need to do this assignment before submitting
urb?

>   if (usb_submit_urb(acecad->irq, GFP_KERNEL))
>   return -EIO;
>  

Thanks.

-- 
Dmitry
--
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 0/4] tablet: remove private copy to USB device

[Dmitry Torokhov]

On Tue, Mar 22, 2016 at 04:04:53PM +0100, Oliver Neukum wrote:
> We now have a macro to easily get to the USB device from the interface.
> So we are cleaning up all drivers to not store a private pointer.

Applied all 4, thank you.

> 
> Oliver Neukum (4):
>   acecad: stop saving struct usb_device
>   aiptek: stop saving struct usb_device
>   gtco: stop saving struct usb_device
>   kbtab: stop saving struct usb_device
> 
>  drivers/input/tablet/acecad.c | 12 ++--
>  drivers/input/tablet/aiptek.c | 18 +-
>  drivers/input/tablet/gtco.c   | 24 
>  drivers/input/tablet/kbtab.c  |  8 
>  4 files changed, 31 insertions(+), 31 deletions(-)
> 
> -- 
> 2.1.4
> 

-- 
Dmitry
--
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] Input: gtco: fix crash on detecting device without endpoints

[Dmitry Torokhov]

On Fri, Mar 18, 2016 at 07:35:00PM +0100, Vladis Dronov wrote:
> The gtco driver expects at least one valid endpoint. If given
> malicious descriptors that specify 0 for the number of endpoints,
> it will crash in the probe function. Ensure there is at least
> one endpoint on the interface before using it. Fix minor coding
> style issue.
> 
> The full report of this issue can be found here:
> http://seclists.org/bugtraq/2016/Mar/86
> 
> Reported-by: Ralf Spenneberg 
> Signed-off-by: Vladis Dronov 

Applied, thank you.

> ---
>  drivers/input/tablet/gtco.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/tablet/gtco.c b/drivers/input/tablet/gtco.c
> index 3a7f3a4..7c18249 100644
> --- a/drivers/input/tablet/gtco.c
> +++ b/drivers/input/tablet/gtco.c
> @@ -858,6 +858,14 @@ static int gtco_probe(struct usb_interface *usbinterface,
>   goto err_free_buf;
>   }
>  
> + /* Sanity check that a device has an endpoint */
> + if (usbinterface->altsetting[0].desc.bNumEndpoints < 1) {
> + dev_err(>dev,
> + "Invalid number of endpoints\n");
> + error = -EINVAL;
> + goto err_free_urb;
> + }
> +
>   /*
>* The endpoint is always altsetting 0, we know this since we know
>* this device only has one interrupt endpoint
> @@ -879,7 +887,7 @@ static int gtco_probe(struct usb_interface *usbinterface,
>* HID report descriptor
>*/
>   if (usb_get_extra_descriptor(usbinterface->cur_altsetting,
> -  HID_DEVICE_TYPE, _desc) != 0){
> +  HID_DEVICE_TYPE, _desc) != 0) {
>   dev_err(>dev,
>   "Can't retrieve exta USB descriptor to get hid report 
> descriptor length\n");
>   error = -EIO;
> -- 
> 2.5.0

-- 
Dmitry
--
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] ims-pcu: sanity check against missing interfaces

[Dmitry Torokhov]

On Thu, Mar 17, 2016 at 03:10:47PM +0100, Oliver Neukum wrote:
> A malicious device missing interface can make the driver oops.
> Add sanity checking.
> 
> Signed-off-by: Oliver Neukum 
> CC: sta...@vger.kernel.org

Applied, thank you.

> ---
>  drivers/input/misc/ims-pcu.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index ac1fa5f..9c0ea36 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1663,6 +1663,8 @@ static int ims_pcu_parse_cdc_data(struct usb_interface 
> *intf, struct ims_pcu *pc
>  
>   pcu->ctrl_intf = usb_ifnum_to_if(pcu->udev,
>union_desc->bMasterInterface0);
> + if (!pcu->ctrl_intf)
> + return -EINVAL;
>  
>   alt = pcu->ctrl_intf->cur_altsetting;
>   pcu->ep_ctrl = >endpoint[0].desc;
> @@ -1670,6 +1672,8 @@ static int ims_pcu_parse_cdc_data(struct usb_interface 
> *intf, struct ims_pcu *pc
>  
>   pcu->data_intf = usb_ifnum_to_if(pcu->udev,
>union_desc->bSlaveInterface0);
> + if (!pcu->data_intf)
> + return -EINVAL;
>  
>   alt = pcu->data_intf->cur_altsetting;
>   if (alt->desc.bNumEndpoints != 2) {
> -- 
> 2.1.4
> 

-- 
Dmitry
--
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] USB: input: powermate: fix oops with malicious USB descriptors

[Dmitry Torokhov]

On Mon, Mar 14, 2016 at 9:46 AM, Josh Boyer  wrote:
> On Mon, Mar 14, 2016 at 12:15 PM, Greg Kroah-Hartman
>  wrote:
>> On Mon, Mar 14, 2016 at 10:12:53AM -0400, Josh Boyer wrote:
>>> The powermate driver expects at least one valid USB endpoint in its
>>> probe function.  If given malicious descriptors that specify 0 for
>>> the number of endpoints, it will crash.  Validate the number of
>>> endpoints on the interface before using them.
>>>
>>> The full report for this issue can be found here:
>>> http://seclists.org/bugtraq/2016/Mar/85
>>>
>>> Reported-by: Ralf Spenneberg 
>>> Cc: stable 
>>> Signed-off-by: Josh Boyer 
>>> ---
>>>  drivers/input/misc/powermate.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>
>> I'll queue these up after 4.6-rc1 is out as the merge window is closed
>> right now, but we might want to think about a better way to handle this
>> type of thing in the USB core.  A way to keep from having to add checks
>> like this for every single driver, when the driver shouldn't even really
>> have their probe function called unless their expected endpoints are
>> going to be there.
>
> I thought this discussion came up a while ago, when something very
> similar was fixed in the whiteheat driver (commit cbb4be652d374).  I
> can't find the thread at the moment, but I thought someone said this
> had to be per-driver for some reason.  I'm more than happy to have a
> core subsystem fix if it's possible.
>
>> I'll think about that over the next few weeks...
>
> I have something around 8 drivers with issues like this.  I think
> Oliver (now CC'd) is working from the same set of bugs.  Should we
> hold off on submitting individual fixes until later, or should we go
> ahead and submit them?

I'll take input bits, there is no need to keep kernel oopsing while we
are working on a more general solution.

Thanks.

-- 
Dmitry
--
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] USB: input: powermate: fix oops with malicious USB descriptors

[Dmitry Torokhov]

On Mon, Mar 14, 2016 at 09:15:48AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Mar 14, 2016 at 10:12:53AM -0400, Josh Boyer wrote:
> > The powermate driver expects at least one valid USB endpoint in its
> > probe function.  If given malicious descriptors that specify 0 for
> > the number of endpoints, it will crash.  Validate the number of
> > endpoints on the interface before using them.
> > 
> > The full report for this issue can be found here:
> > http://seclists.org/bugtraq/2016/Mar/85
> > 
> > Reported-by: Ralf Spenneberg 
> > Cc: stable 
> > Signed-off-by: Josh Boyer 
> > ---
> >  drivers/input/misc/powermate.c | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> I'll queue these up after 4.6-rc1 is out as the merge window is closed
> right now, but we might want to think about a better way to handle this
> type of thing in the USB core.  A way to keep from having to add checks

I do not see any reason in holding it until after rc1, applied.

> like this for every single driver, when the driver shouldn't even really
> have their probe function called unless their expected endpoints are
> going to be there.

I had a patch where driver could declare minimal amount of endpoints it
expects to find, but you mentioned we need something more flexible...

Thanks.

-- 
Dmitry
--
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: [PATCHv3] Input: xpad - Fix double URB submission races

[Dmitry Torokhov]

Hi Laura,

On Mon, Nov 16, 2015 at 02:47:13PM -0800, Laura Abbott wrote:
> +static int __xpad_submit_urb(struct usb_xpad *xpad,
> + unsigned char odata[XPAD_PKT_LEN], int transfer_length,
> + int type, bool safe_submit)
> +{
> + int ret;
> +
> + if (safe_submit || xpad->submit_state == OUT_IRQ_AVAILABLE) {
> + memcpy(xpad->odata, odata, transfer_length);
> + xpad->irq_out->transfer_buffer_length = transfer_length;
> + ret = usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
> + xpad->submit_state = OUT_IRQ_QUEUE_EMPTY;
> + xpad->out_submitter = type;
> + } else {
> + /*
> +  * The goal here is to prevent starvation of any other type.
> +  * If this type matches what is being submitted and there is
> +  * another type in the queue, don't ovewrite it
> +  */
> + if (xpad->submit_state != OUT_IRQ_QUEUE_EMPTY &&
> + xpad->out_submitter == type &&
> + xpad->queue_submitter != type) {
> + ret = -EBUSY;
> + goto out;

No, we do not want to return "busy" here. We should save the most
up-to-date request of given type and re-submit it when URB is no longer
busy.

I CCed you on another patch addressing the same issue, please take a
look when you have a chance.

Thanks.

-- 
Dmitry
--
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] usb: interface: allow drivers declare number of endpoints they need

[Dmitry Torokhov]

On Tue, Dec 1, 2015 at 11:46 AM, Josh Boyer <jwbo...@fedoraproject.org> wrote:
> On Mon, Nov 30, 2015 at 7:47 PM, Dmitry Torokhov
> <dmitry.torok...@gmail.com> wrote:
>> On Mon, Nov 30, 2015 at 3:36 PM, Greg Kroah-Hartman
>> <gre...@linuxfoundation.org> wrote:
>>> On Mon, Nov 30, 2015 at 02:56:09PM -0800, Dmitry Torokhov wrote:
>>>> On Mon, Nov 30, 2015 at 2:18 PM, Greg Kroah-Hartman
>>>> <gre...@linuxfoundation.org> wrote:
>>>> > On Mon, Nov 30, 2015 at 01:11:50PM -0800, Dmitry Torokhov wrote:
>>>> >> USB interface drivers need to check number of endpoints before trying to
>>>> >> access/use them. Quite a few drivers only use the default setting
>>>> >> (altsetting 0), so let's allow them to declare number of endpoints in
>>>> >> altsetting 0 they require to operate and have USB core check it for us
>>>> >> instead of having every driver implement check manually.
>>>> >>
>>>> >> For compatibility, if driver does not specify number of endpoints (i.e.
>>>> >> number of endpoints is left at 0) we bypass the check in USB core and
>>>> >> expect the driver perform necessary checks on its own.
>>>> >>
>>>> >> Acked-by: Alan Stern <st...@rowland.harvard.edu>
>>>> >> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
>>>> >> ---
>>>> >>
>>>> >> Greg, if the patch is reasonable I wonder if I can take it through my
>>>> >> tree, as I have a few drivers that do not check number of endpoints
>>>> >> properly and will crash the kernel when specially crafted device is
>>>> >> plugged in, as reported by Vladis Dronov.
>>>> >>
>>>> >>  drivers/usb/core/driver.c | 9 +
>>>> >>  include/linux/usb.h   | 7 +++
>>>> >>  2 files changed, 16 insertions(+)
>>>> >>
>>>> >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>>>> >> index 6b5063e..d9f680d 100644
>>>> >> --- a/drivers/usb/core/driver.c
>>>> >> +++ b/drivers/usb/core/driver.c
>>>> >> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
>>>> >>
>>>> >>   dev_dbg(dev, "%s - got id\n", __func__);
>>>> >>
>>>> >> + if (driver->num_endpoints &&
>>>> >> + intf->altsetting[0].desc.bNumEndpoints < 
>>>> >> driver->num_endpoints) {
>>>> >> +
>>>> >
>>>> > Empty line :(
>>>> >
>>>> >> + dev_err(dev, "Not enough endpoints %d (want %d)\n",
>>>> >> + intf->altsetting[0].desc.bNumEndpoints,
>>>> >> + driver->num_endpoints);
>>>> >
>>>> > What can a user do with this?
>>>>
>>>> Report on the lists or throw such device into a bin.
>>>>
>>>> >
>>>> >> + return -EINVAL;
>>>> >> + }
>>>> >> +
>>>> >>   error = usb_autoresume_device(udev);
>>>> >>   if (error)
>>>> >>   return error;
>>>> >> diff --git a/include/linux/usb.h b/include/linux/usb.h
>>>> >> index 447fe29..93f8dfc 100644
>>>> >> --- a/include/linux/usb.h
>>>> >> +++ b/include/linux/usb.h
>>>> >> @@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
>>>> >>   * @id_table: USB drivers use ID table to support hotplugging.
>>>> >>   *   Export this with MODULE_DEVICE_TABLE(usb,...).  This must be set
>>>> >>   *   or your driver's probe function will never get called.
>>>> >> + * @num_endpoints: Number of endpoints that should be present in 
>>>> >> default
>>>> >> + *   setting (altsetting 0) the driver needs to operate properly.
>>>> >> + *   The probe will be aborted if actual number of endpoints is less
>>>> >> + *   than what the driver specified here. 0 means no check should be
>>>> >> + *   performed.
>>>> >
>>>> > I don't understand, a driver can do whatever it wants with the endpoints
>>>> > of the interface, why do we need to check/

[PATCH] usb: interface: allow drivers declare number of endpoints they need

[Dmitry Torokhov]

USB interface drivers need to check number of endpoints before trying to
access/use them. Quite a few drivers only use the default setting
(altsetting 0), so let's allow them to declare number of endpoints in
altsetting 0 they require to operate and have USB core check it for us
instead of having every driver implement check manually.

For compatibility, if driver does not specify number of endpoints (i.e.
number of endpoints is left at 0) we bypass the check in USB core and
expect the driver perform necessary checks on its own.

Acked-by: Alan Stern <st...@rowland.harvard.edu>
Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---

Greg, if the patch is reasonable I wonder if I can take it through my
tree, as I have a few drivers that do not check number of endpoints
properly and will crash the kernel when specially crafted device is
plugged in, as reported by Vladis Dronov.

 drivers/usb/core/driver.c | 9 +
 include/linux/usb.h   | 7 +++
 2 files changed, 16 insertions(+)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 6b5063e..d9f680d 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
 
dev_dbg(dev, "%s - got id\n", __func__);
 
+   if (driver->num_endpoints &&
+   intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
+
+   dev_err(dev, "Not enough endpoints %d (want %d)\n",
+   intf->altsetting[0].desc.bNumEndpoints,
+   driver->num_endpoints);
+   return -EINVAL;
+   }
+
error = usb_autoresume_device(udev);
if (error)
return error;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 447fe29..93f8dfc 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
  * @id_table: USB drivers use ID table to support hotplugging.
  * Export this with MODULE_DEVICE_TABLE(usb,...).  This must be set
  * or your driver's probe function will never get called.
+ * @num_endpoints: Number of endpoints that should be present in default
+ * setting (altsetting 0) the driver needs to operate properly.
+ * The probe will be aborted if actual number of endpoints is less
+ * than what the driver specified here. 0 means no check should be
+ * performed.
  * @dynids: used internally to hold the list of dynamically added device
  * ids for this driver.
  * @drvwrap: Driver-model core structure wrapper.
@@ -1099,6 +1104,8 @@ struct usb_driver {
 
const struct usb_device_id *id_table;
 
+   unsigned int num_endpoints;
+
struct usb_dynids dynids;
struct usbdrv_wrap drvwrap;
unsigned int no_dynamic_id:1;
-- 
2.6.0.rc2.230.g3dd15c0


-- 
Dmitry
--
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] usb: interface: allow drivers declare number of endpoints they need

[Dmitry Torokhov]

On Mon, Nov 30, 2015 at 03:39:43PM -0600, Felipe Balbi wrote:
> 
> Hi,
> 
> Dmitry Torokhov <dmitry.torok...@gmail.com> writes:
> > USB interface drivers need to check number of endpoints before trying to
> > access/use them. Quite a few drivers only use the default setting
> > (altsetting 0), so let's allow them to declare number of endpoints in
> > altsetting 0 they require to operate and have USB core check it for us
> > instead of having every driver implement check manually.
> >
> > For compatibility, if driver does not specify number of endpoints (i.e.
> > number of endpoints is left at 0) we bypass the check in USB core and
> > expect the driver perform necessary checks on its own.
> >
> > Acked-by: Alan Stern <st...@rowland.harvard.edu>
> > Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
> > ---
> >
> > Greg, if the patch is reasonable I wonder if I can take it through my
> > tree, as I have a few drivers that do not check number of endpoints
> > properly and will crash the kernel when specially crafted device is
> > plugged in, as reported by Vladis Dronov.
> >
> >  drivers/usb/core/driver.c | 9 +
> >  include/linux/usb.h   | 7 +++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index 6b5063e..d9f680d 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
> >  
> > dev_dbg(dev, "%s - got id\n", __func__);
> >  
> > +   if (driver->num_endpoints &&
> 
> this part of the check is pointless, right ?
> 
> > +   intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
> 
> bNumEndpoints will never be less than 0 and if it is, we're gonna have
> issues elsewhere anyway.

Fair enough, I'll drop it.

Thanks.

-- 
Dmitry
--
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] usb: interface: allow drivers declare number of endpoints they need

[Dmitry Torokhov]

On Mon, Nov 30, 2015 at 2:18 PM, Greg Kroah-Hartman
<gre...@linuxfoundation.org> wrote:
> On Mon, Nov 30, 2015 at 01:11:50PM -0800, Dmitry Torokhov wrote:
>> USB interface drivers need to check number of endpoints before trying to
>> access/use them. Quite a few drivers only use the default setting
>> (altsetting 0), so let's allow them to declare number of endpoints in
>> altsetting 0 they require to operate and have USB core check it for us
>> instead of having every driver implement check manually.
>>
>> For compatibility, if driver does not specify number of endpoints (i.e.
>> number of endpoints is left at 0) we bypass the check in USB core and
>> expect the driver perform necessary checks on its own.
>>
>> Acked-by: Alan Stern <st...@rowland.harvard.edu>
>> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
>> ---
>>
>> Greg, if the patch is reasonable I wonder if I can take it through my
>> tree, as I have a few drivers that do not check number of endpoints
>> properly and will crash the kernel when specially crafted device is
>> plugged in, as reported by Vladis Dronov.
>>
>>  drivers/usb/core/driver.c | 9 +
>>  include/linux/usb.h   | 7 +++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>> index 6b5063e..d9f680d 100644
>> --- a/drivers/usb/core/driver.c
>> +++ b/drivers/usb/core/driver.c
>> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
>>
>>   dev_dbg(dev, "%s - got id\n", __func__);
>>
>> + if (driver->num_endpoints &&
>> + intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) {
>> +
>
> Empty line :(
>
>> + dev_err(dev, "Not enough endpoints %d (want %d)\n",
>> + intf->altsetting[0].desc.bNumEndpoints,
>> + driver->num_endpoints);
>
> What can a user do with this?

Report on the lists or throw such device into a bin.

>
>> + return -EINVAL;
>> + }
>> +
>>   error = usb_autoresume_device(udev);
>>   if (error)
>>   return error;
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index 447fe29..93f8dfc 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
>>   * @id_table: USB drivers use ID table to support hotplugging.
>>   *   Export this with MODULE_DEVICE_TABLE(usb,...).  This must be set
>>   *   or your driver's probe function will never get called.
>> + * @num_endpoints: Number of endpoints that should be present in default
>> + *   setting (altsetting 0) the driver needs to operate properly.
>> + *   The probe will be aborted if actual number of endpoints is less
>> + *   than what the driver specified here. 0 means no check should be
>> + *   performed.
>
> I don't understand, a driver can do whatever it wants with the endpoints
> of the interface, why do we need to check/know this ahead of time?  What
> is crashing without this?

The kernel because some drivers do not verify that
intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing
intf->altsetting[0].endpoints[0].

>
> It's up to the driver to check this, if it cares about it.

Instead of duplicating the check in almost every driver is it more
efficient to allow USB core check it for them (if driver requests it
to do so).

>  How many
> drivers do you have that is going to care?

I saw at least 3 that did not check, that's from cursory glance. Plus
we have many that do check explicitly.

>  Why is this suddenly a new
> thing that we haven't run into in the past 15+ years?

We are less trusting now. Before we/some of the drivers believed that
if device has VID/PID that they recognize the rest of descriptors will
have the data we expect, but we can't rely on this anymore.

Thanks.

-- 
Dmitry
--
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] usb: interface: allow drivers declare number of endpoints they need

[Dmitry Torokhov]

On Mon, Nov 30, 2015 at 3:36 PM, Greg Kroah-Hartman
<gre...@linuxfoundation.org> wrote:
> On Mon, Nov 30, 2015 at 02:56:09PM -0800, Dmitry Torokhov wrote:
>> On Mon, Nov 30, 2015 at 2:18 PM, Greg Kroah-Hartman
>> <gre...@linuxfoundation.org> wrote:
>> > On Mon, Nov 30, 2015 at 01:11:50PM -0800, Dmitry Torokhov wrote:
>> >> USB interface drivers need to check number of endpoints before trying to
>> >> access/use them. Quite a few drivers only use the default setting
>> >> (altsetting 0), so let's allow them to declare number of endpoints in
>> >> altsetting 0 they require to operate and have USB core check it for us
>> >> instead of having every driver implement check manually.
>> >>
>> >> For compatibility, if driver does not specify number of endpoints (i.e.
>> >> number of endpoints is left at 0) we bypass the check in USB core and
>> >> expect the driver perform necessary checks on its own.
>> >>
>> >> Acked-by: Alan Stern <st...@rowland.harvard.edu>
>> >> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
>> >> ---
>> >>
>> >> Greg, if the patch is reasonable I wonder if I can take it through my
>> >> tree, as I have a few drivers that do not check number of endpoints
>> >> properly and will crash the kernel when specially crafted device is
>> >> plugged in, as reported by Vladis Dronov.
>> >>
>> >>  drivers/usb/core/driver.c | 9 +
>> >>  include/linux/usb.h   | 7 +++
>> >>  2 files changed, 16 insertions(+)
>> >>
>> >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>> >> index 6b5063e..d9f680d 100644
>> >> --- a/drivers/usb/core/driver.c
>> >> +++ b/drivers/usb/core/driver.c
>> >> @@ -306,6 +306,15 @@ static int usb_probe_interface(struct device *dev)
>> >>
>> >>   dev_dbg(dev, "%s - got id\n", __func__);
>> >>
>> >> + if (driver->num_endpoints &&
>> >> + intf->altsetting[0].desc.bNumEndpoints < driver->num_endpoints) 
>> >> {
>> >> +
>> >
>> > Empty line :(
>> >
>> >> + dev_err(dev, "Not enough endpoints %d (want %d)\n",
>> >> + intf->altsetting[0].desc.bNumEndpoints,
>> >> + driver->num_endpoints);
>> >
>> > What can a user do with this?
>>
>> Report on the lists or throw such device into a bin.
>>
>> >
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >>   error = usb_autoresume_device(udev);
>> >>   if (error)
>> >>   return error;
>> >> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> >> index 447fe29..93f8dfc 100644
>> >> --- a/include/linux/usb.h
>> >> +++ b/include/linux/usb.h
>> >> @@ -1051,6 +1051,11 @@ struct usbdrv_wrap {
>> >>   * @id_table: USB drivers use ID table to support hotplugging.
>> >>   *   Export this with MODULE_DEVICE_TABLE(usb,...).  This must be set
>> >>   *   or your driver's probe function will never get called.
>> >> + * @num_endpoints: Number of endpoints that should be present in default
>> >> + *   setting (altsetting 0) the driver needs to operate properly.
>> >> + *   The probe will be aborted if actual number of endpoints is less
>> >> + *   than what the driver specified here. 0 means no check should be
>> >> + *   performed.
>> >
>> > I don't understand, a driver can do whatever it wants with the endpoints
>> > of the interface, why do we need to check/know this ahead of time?  What
>> > is crashing without this?
>>
>> The kernel because some drivers do not verify that
>> intf->altsetting[0].desc.bNumEndpoints >= 1 before referencing
>> intf->altsetting[0].endpoints[0].
>
> The USB core does that?  Or just a driver, and if it's just a driver, we
> should fix that in the driver itself as there are lots of other
> validation checks the drivers should be doing becides just this one
> about endpoints, sizes, and directions that we can't catch in the core.
>
>> > It's up to the driver to check this, if it cares about it.
>>
>> Instead of duplicating the check in almost every driver is it more
>> efficient to allow USB core check it for them (if driver requests it

Re: First kernel patch (optimization)

[Dmitry Torokhov]

Hi Erik,

>
> Yeah, I'm still reading this email thread and learned lots from it.
> I'm working on something more meaningful, but it's not going to be
> ground breaking of course, there is a led on my capslock key on a new
> machine I won at work that does not switch off properly after it is
> switched on.

Is it Debian-derivative by any chance? Their capslock setup is wonky
because CapsLock key does no actually set up as a CapsLock but another
modifier. Also is it in X or is it on text console? Because X handles
led state on its own...

> I think it is something to do with the LED_CAPSL variable
> in here:
>
> drivers/hid/usbhid/usbkbd.c

I do not think you are using usbkbd driver - it is for keyboards in
"boot protocol" and barely anyone users them in such mode. You need to
look into drivers/hid/hid-input.c.

Thanks.

-- 
Dmitry
--
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: [PATCHv2] Input: xpad - Fix double URB submission races

[Dmitry Torokhov]

On Mon, Aug 24, 2015 at 9:22 PM, Laura Abbott labb...@redhat.com wrote:
 On 08/21/2015 09:50 AM, Dmitry Torokhov wrote:

 Hi Laura,

 On Mon, Aug 10, 2015 at 05:26:12PM -0700, Laura Abbott wrote:

 v2: Created a proper queue for events instead of just dropping them


 How long does it take for the queue to exhaust your memory if you keep
 bombarding the driver with requests?


 My script which changes the LEDs as fast as possible ran for 7+ hours on
 my machine with 16GB of RAM without exhausting all of it. This is also a
 very extreme case as almost any kind of delay between sending
 commands will drain the queue.

Hmm, that means the device is able to process requests pretty fast;
I'm impressed.



 I do not think you need a queue. I believe the nature of LEDs and rumble
 force feedback effect is such that you can discard all requests but the
 latest that arrived between the moment you submitted a request to the
 device and the moment you are ready submit a new one.


 So your suggestion is to only keep a single item in the queue?

That would not be a queue anymore, but essentially yes. Store pending
brightness and FF effect in the driver structure and simply replace it
with the latest requests until the device is ready to process next
request. You need to take care alternating serving LED vs FF requests
to make sure one does not starve another.

Thanks!

-- 
Dmitry
--
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: [PATCHv2] Input: xpad - Fix double URB submission races

[Dmitry Torokhov]

Hi Laura,

On Mon, Aug 10, 2015 at 05:26:12PM -0700, Laura Abbott wrote:
 v2: Created a proper queue for events instead of just dropping them

How long does it take for the queue to exhaust your memory if you keep
bombarding the driver with requests?

I do not think you need a queue. I believe the nature of LEDs and rumble
force feedback effect is such that you can discard all requests but the
latest that arrived between the moment you submitted a request to the
device and the moment you are ready submit a new one.

Thanks.

-- 
Dmitry
--
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] Input: xpad - Fix double URB submission races

[Dmitry Torokhov]

Hi Laura,

On Wed, Jul 29, 2015 at 02:27:23PM -0700, Laura Abbott wrote:
@@ -791,6 +796,9 @@ static int xpad_play_effect(struct input_dev *dev, void 
*data, struct ff_effect
  {
   struct usb_xpad *xpad = input_get_drvdata(dev);
  
 + if (test_and_set_bit(OUT_IRQ_SUBMITTED, xpad-odata_flags))
 + return 0;
 +

So this results in basically ignoring the request if urb is busy which
is not the best way of handling this. You need to note that there is
pending effect to be played and submit it after currently executing
request completes.

The same needs to be done for led toggling request.

Thanks.

-- 
Dmitry
--
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 00/27] Export I2C and OF module aliases in missing drivers

[Dmitry Torokhov]

On Thu, Jul 30, 2015 at 09:35:17AM -0700, Dmitry Torokhov wrote:
 Hi Javier,
 
 On Thu, Jul 30, 2015 at 06:18:25PM +0200, Javier Martinez Canillas wrote:
  Hello,
  
  Short version:
  
  This series add the missing MODULE_DEVICE_TABLE() for OF and I2C tables
  to export that information so modules have the correct aliases built-in
  and autoloading works correctly.
  
  Longer version:
  
  Currently it's mandatory for I2C drivers to have an I2C device ID table
  regardless if the device was registered using platform data or OF. This
  is because the I2C core needs an I2C device ID table for two reasons:
  
  1) Match the I2C client with a I2C device ID so a struct i2c_device_id
 is passed to the I2C driver probe() function.
  
  2) Export the module aliases from the I2C device ID table so userspace
 can auto-load the correct module. This is because i2c_device_uevent
 always reports a MODALIAS of the form i2c:client-name.
 
 Why are we not fixing this? We emit specially carved uevent for
 ACPI-based devices, why not the same for OF? Platform bus does this...

Ah, now I see the 27/27 patch. I think it is exactly what we need. And
probably for SPI bus as well.

Thanks.

-- 
Dmitry
--
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 00/27] Export I2C and OF module aliases in missing drivers

[Dmitry Torokhov]

Hi Javier,

On Thu, Jul 30, 2015 at 06:18:25PM +0200, Javier Martinez Canillas wrote:
 Hello,
 
 Short version:
 
 This series add the missing MODULE_DEVICE_TABLE() for OF and I2C tables
 to export that information so modules have the correct aliases built-in
 and autoloading works correctly.
 
 Longer version:
 
 Currently it's mandatory for I2C drivers to have an I2C device ID table
 regardless if the device was registered using platform data or OF. This
 is because the I2C core needs an I2C device ID table for two reasons:
 
 1) Match the I2C client with a I2C device ID so a struct i2c_device_id
is passed to the I2C driver probe() function.
 
 2) Export the module aliases from the I2C device ID table so userspace
can auto-load the correct module. This is because i2c_device_uevent
always reports a MODALIAS of the form i2c:client-name.

Why are we not fixing this? We emit specially carved uevent for
ACPI-based devices, why not the same for OF? Platform bus does this...

Thanks.

-- 
Dmitry
--
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: [RFC ebeam PATCH 2/2] input: misc: New USB eBeam input driver

[Dmitry Torokhov]

On Mon, Jul 20, 2015 at 02:59:56PM -0700, Greg KH wrote:
 On Mon, Jul 20, 2015 at 11:03:19PM +0200, Yann Cantin wrote:
  Signed-off-by: Yann Cantin yann.can...@laposte.net
 
  +
  +   /* sysfs setup */
  +   err = sysfs_create_group(intf-dev.kobj, ebeam_attr_group);
 
 Ick, you just added the sysfs files to the USB device, not your input
 device, are you sure you tested this?
 
 And there should be a race-free way to add an attribute group to an
 input device, as this is, you are adding them to the device _after_ it
 is created, so userspace will not see them at creation time, causing a
 race.

No, there are no driver-specific attributed on input devices themselves,
they belong to the actual hardware devices. The input devices only
export standard attributes applicable to every and all input devices
in the system.

Thanks.

-- 
Dmitry
--
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: [RFC ebeam PATCH 2/2] input: misc: New USB eBeam input driver

[Dmitry Torokhov]

On Mon, Jul 20, 2015 at 03:40:33PM -0700, Greg KH wrote:
 On Mon, Jul 20, 2015 at 03:26:40PM -0700, Dmitry Torokhov wrote:
  On Mon, Jul 20, 2015 at 02:59:56PM -0700, Greg KH wrote:
   On Mon, Jul 20, 2015 at 11:03:19PM +0200, Yann Cantin wrote:
Signed-off-by: Yann Cantin yann.can...@laposte.net
   
+
+   /* sysfs setup */
+   err = sysfs_create_group(intf-dev.kobj, ebeam_attr_group);
   
   Ick, you just added the sysfs files to the USB device, not your input
   device, are you sure you tested this?
   
   And there should be a race-free way to add an attribute group to an
   input device, as this is, you are adding them to the device _after_ it
   is created, so userspace will not see them at creation time, causing a
   race.
  
  No, there are no driver-specific attributed on input devices themselves,
  they belong to the actual hardware devices. The input devices only
  export standard attributes applicable to every and all input devices
  in the system.
 
 Then the Documentation in this patch better be fixed up, as it points to
 the input device as having these sysfs files :)
 
 But as these are input device attributes, and not USB device interface
 attributes, putting them on the USB interface doesn't make much sense,
 and as I pointed out, is racy.  Why can't input devices have
 driver-specific attributes?  Why does input devices add their own
 attributes sometimes (like in psmouse, which does so in a racy way) yet
 this driver shouldn't do that?
 
 Hm, nevermind about psmouse, that happens on the parent device too it
 seems (a serio device), not the input device, so it is consistent, but
 not something I really like...

I think it is historical as quite often they (attributes) affect the
hardware itself, not behavior of input interface. And so they were
always placed on hardware level, not input device level.

Anyway, I do not think you are going to win your war on sysfs attributes
created post-device creation. We just need to recognize that there are
attributes that are created by the driver when it is bound to the
device. Maybe we need to revisit the idea about emitting special uevent
when driver is fully bound to a device and interested userspace can
start using it.

Thanks.

-- 
Dmitry
--
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] ff-core: use new debug macros

[Dmitry Torokhov]

On Tue, Apr 14, 2015 at 02:06:10PM +0200, Oliver Neukum wrote:
 Replace old pr_* with dev_* debugging macros

Not so new anymore ;)

Applied, thank you.

 
 Signed-off-by: Oliver Neukum oneu...@suse.de
 ---
  drivers/input/ff-core.c | 10 --
  1 file changed, 4 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
 index f50f6dd..b81c88c 100644
 --- a/drivers/input/ff-core.c
 +++ b/drivers/input/ff-core.c
 @@ -23,8 +23,6 @@
  
  /* #define DEBUG */
  
 -#define pr_fmt(fmt) KBUILD_BASENAME :  fmt
 -
  #include linux/input.h
  #include linux/module.h
  #include linux/mutex.h
 @@ -116,7 +114,7 @@ int input_ff_upload(struct input_dev *dev, struct 
 ff_effect *effect,
  
   if (effect-type  FF_EFFECT_MIN || effect-type  FF_EFFECT_MAX ||
   !test_bit(effect-type, dev-ffbit)) {
 - pr_debug(invalid or not supported effect type in upload\n);
 + dev_dbg(dev-dev, invalid or not supported effect type in 
 upload\n);
   return -EINVAL;
   }
  
 @@ -124,7 +122,7 @@ int input_ff_upload(struct input_dev *dev, struct 
 ff_effect *effect,
   (effect-u.periodic.waveform  FF_WAVEFORM_MIN ||
effect-u.periodic.waveform  FF_WAVEFORM_MAX ||
!test_bit(effect-u.periodic.waveform, dev-ffbit))) {
 - pr_debug(invalid or not supported wave form in upload\n);
 + dev_dbg(dev-dev, invalid or not supported wave form in 
 upload\n);
   return -EINVAL;
   }
  
 @@ -246,7 +244,7 @@ static int flush_effects(struct input_dev *dev, struct 
 file *file)
   struct ff_device *ff = dev-ff;
   int i;
  
 - pr_debug(flushing now\n);
 + dev_dbg(dev-dev, flushing now\n);
  
   mutex_lock(ff-mutex);
  
 @@ -316,7 +314,7 @@ int input_ff_create(struct input_dev *dev, unsigned int 
 max_effects)
   int i;
  
   if (!max_effects) {
 - pr_err(cannot allocate device without any effects\n);
 + dev_err(dev-dev, cannot allocate device without any 
 effects\n);
   return -EINVAL;
   }
  
 -- 
 2.1.4
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-input in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Dmitry
--
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 v2 7/7] USB / PM: Allow USB devices to remain runtime-suspended when sleeping

[Dmitry Torokhov]

Hi Tomeu,

On Fri, Apr 03, 2015 at 02:57:56PM +0200, Tomeu Vizoso wrote:
 Have dev_pm_ops.prepare return 1 for USB devices, interfaces, endpoints
 and ports so that USB devices can remain runtime-suspended when the
 system goes to a sleep state, if their wakeup state is correct.
 
 Also enable runtime PM for endpoints, which is another requirement for
 the above to work.

After patching I think the 4th unrelated subsystem with stubs for
prepare() I think it is pretty clear that this approach is not the right
one.

If your driver does not care about any children hanging off it there is
dev-ignore_children flag that either already does what you want, or
maybe needs adjusted to support your use case.

Thanks.

 
 Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com
 ---
  drivers/usb/core/endpoint.c | 17 +
  drivers/usb/core/message.c  | 16 
  drivers/usb/core/port.c |  6 ++
  drivers/usb/core/usb.c  |  8 +++-
  4 files changed, 46 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/usb/core/endpoint.c b/drivers/usb/core/endpoint.c
 index 39a2402..7c82bb7 100644
 --- a/drivers/usb/core/endpoint.c
 +++ b/drivers/usb/core/endpoint.c
 @@ -160,6 +160,19 @@ static const struct attribute_group *ep_dev_groups[] = {
   NULL
  };
  
 +#ifdef   CONFIG_PM
 +
 +static int usb_ep_device_prepare(struct device *dev)
 +{
 + return 1;
 +}
 +
 +static const struct dev_pm_ops usb_ep_device_pm_ops = {
 + .prepare =  usb_ep_device_prepare,
 +};
 +
 +#endif   /* CONFIG_PM */
 +
  static void ep_device_release(struct device *dev)
  {
   struct ep_device *ep_dev = to_ep_device(dev);
 @@ -170,6 +183,9 @@ static void ep_device_release(struct device *dev)
  struct device_type usb_ep_device_type = {
   .name = usb_endpoint,
   .release = ep_device_release,
 +#ifdef CONFIG_PM
 + .pm =   usb_ep_device_pm_ops,
 +#endif
  };
  
  int usb_create_ep_devs(struct device *parent,
 @@ -197,6 +213,7 @@ int usb_create_ep_devs(struct device *parent,
   goto error_register;
  
   device_enable_async_suspend(ep_dev-dev);
 + pm_runtime_enable(ep_dev-dev);
   endpoint-ep_dev = ep_dev;
   return retval;
  
 diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
 index f368d20..9041aee 100644
 --- a/drivers/usb/core/message.c
 +++ b/drivers/usb/core/message.c
 @@ -1589,10 +1589,26 @@ static int usb_if_uevent(struct device *dev, struct 
 kobj_uevent_env *env)
   return 0;
  }
  
 +#ifdef   CONFIG_PM
 +
 +static int usb_if_prepare(struct device *dev)
 +{
 + return 1;
 +}
 +
 +static const struct dev_pm_ops usb_if_pm_ops = {
 + .prepare =  usb_if_prepare,
 +};
 +
 +#endif   /* CONFIG_PM */
 +
  struct device_type usb_if_device_type = {
   .name = usb_interface,
   .release =  usb_release_interface,
   .uevent =   usb_if_uevent,
 +#ifdef CONFIG_PM
 + .pm =   usb_if_pm_ops,
 +#endif
  };
  
  static struct usb_interface_assoc_descriptor *find_iad(struct usb_device 
 *dev,
 diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
 index 2106183..f49707d 100644
 --- a/drivers/usb/core/port.c
 +++ b/drivers/usb/core/port.c
 @@ -168,12 +168,18 @@ static int usb_port_runtime_suspend(struct device *dev)
  
   return retval;
  }
 +
 +static int usb_port_prepare(struct device *dev)
 +{
 + return 1;
 +}
  #endif
  
  static const struct dev_pm_ops usb_port_pm_ops = {
  #ifdef CONFIG_PM
   .runtime_suspend =  usb_port_runtime_suspend,
   .runtime_resume =   usb_port_runtime_resume,
 + .prepare =  usb_port_prepare,
  #endif
  };
  
 diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
 index 8d5b2f4..3a55c91 100644
 --- a/drivers/usb/core/usb.c
 +++ b/drivers/usb/core/usb.c
 @@ -316,7 +316,13 @@ static int usb_dev_uevent(struct device *dev, struct 
 kobj_uevent_env *env)
  
  static int usb_dev_prepare(struct device *dev)
  {
 - return 0;   /* Implement eventually? */
 + struct usb_device *udev = to_usb_device(dev);
 +
 + /* Return 0 if the current wakeup setting is wrong, otherwise 1 */
 + if (udev-do_remote_wakeup != device_may_wakeup(dev))
 + return 0;
 +
 + return 1;
  }
  
  static void usb_dev_complete(struct device *dev)
 -- 
 2.3.4
 
 --
 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

-- 
Dmitry
--
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 1/3] phy: core: Add devm_of_phy_get_by_index to phy-core

[Dmitry Torokhov]

On Wed, Mar 25, 2015 at 05:04:32PM -0700, Arun Ramamurthy wrote:
 
 
 On 15-03-25 03:03 PM, Kishon Vijay Abraham I wrote:
 Hi,
 
 On Saturday 21 March 2015 02:59 AM, Arun Ramamurthy wrote:
 
 
 On 15-03-20 02:26 PM, Dmitry Torokhov wrote:
 Hi Arun,
 
 On Fri, Mar 20, 2015 at 02:07:08PM -0700, Arun Ramamurthy wrote:
 Adding devm_of_phy_get_by_index to get phys by supplying an index
 and not a phy name when multiple phys are declared
 
 I think a bit more explanation on why get_by_index is needed here.
 Thanks Kison. Can you be more specific? I am unsure of what more I
 can explain here.

We just need to mention that some generic drivers, such as ehci, may use
multiple phys, and for such drivers referencing phy(s) by name(s) does
not make sense. Instead of inventing elaborate naming schemes and
producing custom code to iterate over names, such drivers are better of
using nameless phy bindings and using this newly introduced API to
iterate through them.

Thanks.

-- 
Dmitry
--
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 1/3] phy: core: Add devm_of_phy_get_by_index to phy-core

[Dmitry Torokhov]

Hi Arun,

On Fri, Mar 20, 2015 at 02:07:08PM -0700, Arun Ramamurthy wrote:
 Adding devm_of_phy_get_by_index to get phys by supplying an index
 and not a phy name when multiple phys are declared
 
 Reviewed-by: Ray Jui r...@broadcom.com
 Reviewed-by: Scott Branden sbran...@broadcom.com
 Signed-off-by: Arun Ramamurthy arun.ramamur...@broadcom.com
 ---
  drivers/phy/phy-core.c  | 30 ++
  include/linux/phy/phy.h |  2 ++
  2 files changed, 32 insertions(+)
 
 diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
 index a12d353..0c03876 100644
 --- a/drivers/phy/phy-core.c
 +++ b/drivers/phy/phy-core.c
 @@ -622,6 +622,36 @@ struct phy *devm_of_phy_get(struct device *dev, struct 
 device_node *np,
  EXPORT_SYMBOL_GPL(devm_of_phy_get);
  
  /**
 + * devm_of_phy_get_by_index() - lookup and obtain a reference to a phy by 
 index.
 + * @dev: device that requests this phy
 + * @np: node containing the phy
 + * @index: index of the phy
 + *
 + * Gets the phy using _of_phy_get(), and associates a device with it using
 + * devres. On driver detach, release function is invoked on the devres data,
 + * then, devres data is freed.
 + *
 + */
 +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node 
 *np,
 +  int index)
 +{
 + struct phy **ptr, *phy;
 +
 + ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
 + if (!ptr)
 + return ERR_PTR(-ENOMEM);
 +
 + phy = _of_phy_get(np, index);
 + if (!IS_ERR(phy)) {
 + *ptr = phy;
 + devres_add(dev, ptr);
 + } else {
 + devres_free(ptr);
 + }
 +
 + return phy;
 +}

You want EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index); here.

 +/**
   * phy_create() - create a new phy
   * @dev: device that is creating the new phy
   * @node: device node of the phy
 diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
 index a0197fa..ae2ffaf 100644
 --- a/include/linux/phy/phy.h
 +++ b/include/linux/phy/phy.h
 @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const char 
 *string);
  struct phy *devm_phy_optional_get(struct device *dev, const char *string);
  struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
   const char *con_id);
 +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node 
 *np,
 +  int index);
  void phy_put(struct phy *phy);
  void devm_phy_put(struct device *dev, struct phy *phy);
  struct phy *of_phy_get(struct device_node *np, const char *con_id);
 -- 
 2.3.2
 

-- 
Dmitry
--
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 2/3] usb: ehci-platform: Use devm_of_phy_get_by_index

[Dmitry Torokhov]

Hi Arun,

On Fri, Mar 20, 2015 at 02:07:09PM -0700, Arun Ramamurthy wrote:
 Getting phys by index instead of phy names so that the dt
 bindings phy-names remain consistent when multiple phys are present
 
 Reviewed-by: Ray Jui r...@broadcom.com
 Reviewed-by: Scott Branden sbran...@broadcom.com
 Signed-off-by: Arun Ramamurthy arun.ramamur...@broadcom.com
 ---
  drivers/usb/host/ehci-platform.c | 20 
  1 file changed, 4 insertions(+), 16 deletions(-)
 
 diff --git a/drivers/usb/host/ehci-platform.c 
 b/drivers/usb/host/ehci-platform.c
 index d8a75a5..8b0c7ae 100644
 --- a/drivers/usb/host/ehci-platform.c
 +++ b/drivers/usb/host/ehci-platform.c
 @@ -154,7 +154,6 @@ static int ehci_platform_probe(struct platform_device 
 *dev)
   struct usb_ehci_pdata *pdata = dev_get_platdata(dev-dev);
   struct ehci_platform_priv *priv;
   struct ehci_hcd *ehci;
 - const char *phy_name;
   int err, irq, phy_num, clk = 0;
  
   if (usb_disabled())
 @@ -212,21 +211,10 @@ static int ehci_platform_probe(struct platform_device 
 *dev)
   return -ENOMEM;
  
   for (phy_num = 0; phy_num  priv-num_phys; phy_num++) {
 - err = of_property_read_string_index(
 - dev-dev.of_node,
 - phy-names, phy_num,
 - phy_name);
 -
 - if (err  0) {
 - if (priv-num_phys  1) {
 - dev_err(dev-dev, phy-names 
 not provided);
 - goto err_put_hcd;
 - } else
 - phy_name = usb;
 - }
 -
 - priv-phys[phy_num] = devm_phy_get(dev-dev,
 - phy_name);
 + priv-phys[phy_num] =
 + devm_of_phy_get_by_index(dev-dev,
 + dev-dev.of_node,
 + phy_num);
   if (IS_ERR(priv-phys[phy_num])) {
   err = PTR_ERR(priv-phys[phy_num]);
   if ((priv-num_phys  1) ||

While you are fixing this can you please correct the wrong indentation
level and clean up that whole weird business of treating phy not
present in DT in a special way and, stuffing NULL pointer in priv-phys,
and checking it for NULL elsewhere?

Thanks!

-- 
Dmitry
--
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] Remove deprecated IRQF_DISABLED flag entirely

[Dmitry Torokhov]

On Thu, Mar 5, 2015 at 5:11 AM, Hannes Reinecke h...@suse.de wrote:
 On 03/05/2015 01:59 PM, Valentin Rothberg wrote:
 The IRQF_DISABLED is a NOOP and has been scheduled for removal since
 Linux v2.6.36 by commit 6932bf37bed4 (genirq: Remove IRQF_DISABLED from
 core code).

 According to commit e58aa3d2d0cc (genirq: Run irq handlers with
 interrupts disabled) running IRQ handlers with interrupts enabled can
 cause stack overflows when the interrupt line of the issuing device is
 still active.

 This patch ends the grace period for IRQF_DISABLED (i.e., SA_INTERRUPT
 in older versions of Linux) and removes the definition and all remaining
 usages of this flag.

 Signed-off-by: Valentin Rothberg valentinrothb...@gmail.com
 ---
 The bigger hunk in Documentation/scsi/ncr53c8xx.txt is removed entirely
 as IRQF_DISABLED is gone now; the usage in older kernel versions
 (including the old SA_INTERRUPT flag) should be discouraged.  The
 trouble of using IRQF_SHARED is a general problem and not specific to
 any driver.

 I left the reference in Documentation/PCI/MSI-HOWTO.txt untouched since
 it has already been removed in linux-next by commit b0e1ee8e1405
 (MSI-HOWTO.txt: remove reference on IRQF_DISABLED).

 All remaining references are changelogs that I suggest to keep.

 While you're at it: having '0x0' as a value for the irq flags looks
 a bit silly, and makes you wonder what the parameter is for.

 I would rather like to have

 #define IRQF_NONE 0x0

 and use it for these cases.
 That way the scope of that parameter is clear.

No, that would imply that IRQ never triggers whereas passing 0 means
we keep triggers that have been set by the platform.

Thanks.

-- 
Dmitry
--
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

[PATCH] usb: musb: blackfin: remove incorrect __exit_p()

[Dmitry Torokhov]

bfin_remove() is not (nor should it be) marked as __exit, so we should
not be using __exit_p() wrapper with it, otherwise unbinding through
sysfs does not work properly.

Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---

Not tested, sorry.

 drivers/usb/musb/blackfin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb/blackfin.c b/drivers/usb/musb/blackfin.c
index 1782501..6123b74 100644
--- a/drivers/usb/musb/blackfin.c
+++ b/drivers/usb/musb/blackfin.c
@@ -608,7 +608,7 @@ static SIMPLE_DEV_PM_OPS(bfin_pm_ops, bfin_suspend, 
bfin_resume);
 
 static struct platform_driver bfin_driver = {
.probe  = bfin_probe,
-   .remove = __exit_p(bfin_remove),
+   .remove = bfin_remove,
.driver = {
.name   = musb-blackfin,
.pm = bfin_pm_ops,
-- 
2.2.0.rc0.207.ga3a616c


-- 
Dmitry
--
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] Input: usbtouchscreen - separate report and transmit buffer size handling

[Dmitry Torokhov]

Hi Christian,

On Tue, Oct 15, 2013 at 04:50:00PM +0200, Christian Engelmayer wrote:
 This patch supports the separate handling of the USB transfer buffer length
 and the length of the buffer used for multi packet support. The USB transfer
 size can now be explicitly configured via the device_info record. Otherwise
 it defaults to the configured report packet size as before. For devices
 supporting multiple report or diagnostic packets, the USB transfer size is
 now reduced to the USB endpoints wMaxPacketSize if not explicitly set.
 
 This fixes an issue where event reporting can be delayed for an arbitrary
 time for multi packet devices. For instance the report size for eGalax devices
 is defined to the 16 byte maximum diagnostic packet size as opposed to the 5
 byte report packet size. In case the driver requests 16 byte from the USB
 interrupt endpoint, the USB host controller driver needs to split up the
 request into 2 accesses according to the endpoints wMaxPacketSize of 8 byte.
 When the first transfer is answered by the eGalax device with not less than
 the full 8 byte requested, the host controller has got no way of knowing
 whether the touch controller has got additional data queued and will issue
 the second transfer. If per example a liftoff event finishes at such a
 wMaxPacketSize boundary, the data will not be available to the usbtouch driver
 until a further event is triggered and transfered to the host. From user
 perspective the BTN_TOUCH release event in this case is stuck until the next
 touch down event.
 
 Signed-off-by: Christian Engelmayer christian.engelma...@frequentis.com
 ---
  drivers/input/touchscreen/usbtouchscreen.c |   16 
  1 files changed, 12 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/input/touchscreen/usbtouchscreen.c 
 b/drivers/input/touchscreen/usbtouchscreen.c
 index 721fdb3..aa1f6a7 100644
 --- a/drivers/input/touchscreen/usbtouchscreen.c
 +++ b/drivers/input/touchscreen/usbtouchscreen.c
 @@ -76,6 +76,7 @@ struct usbtouch_device_info {
   int min_yc, max_yc;
   int min_press, max_press;
   int rept_size;
 + int xmit_size;
  
   /*
* Always service the USB devices irq not just when the input device is
 @@ -1523,7 +1524,7 @@ static int usbtouch_reset_resume(struct usb_interface 
 *intf)
  static void usbtouch_free_buffers(struct usb_device *udev,
 struct usbtouch_usb *usbtouch)
  {
 - usb_free_coherent(udev, usbtouch-type-rept_size,
 + usb_free_coherent(udev, usbtouch-type-xmit_size,
 usbtouch-data, usbtouch-data_dma);
   kfree(usbtouch-buffer);
  }
 @@ -1567,8 +1568,15 @@ static int usbtouch_probe(struct usb_interface *intf,
   usbtouch-type = type;
   if (!type-process_pkt)
   type-process_pkt = usbtouch_process_pkt;
 + if (!type-xmit_size) {
 + if ((type-get_pkt_len) 
 + (type-rept_size  le16_to_cpu(endpoint-wMaxPacketSize)))
 + type-xmit_size = le16_to_cpu(endpoint-wMaxPacketSize);
 + else
 + type-xmit_size = type-rept_size;

'type' points to a shared data structure and should not be modified. It
looks like we already violating this so a cleanup patch would be
appreciated as well.

BTW, maybe we should do:

u16 wMaxPaxetSize = le16_to_cpu(endpoint-wMaxPacketSize);
xmit_size = min(type-rept_size, wMaxPaxetSize);

?

Thanks.

-- 
Dmitry
--
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 v1 26/49] input: cm109: prepare for enabling irq in complete()

[Dmitry Torokhov]

Hi Ming,

On Sun, Aug 18, 2013 at 12:24:51AM +0800, Ming Lei wrote:
 Complete() will be run with interrupt enabled, so change to
 spin_lock_irqsave().

I think cm109 needs some love in it's URB handling, but this patch does
not change anything, so:

Acked-by: Dmitry Torokhov dmitry.torok...@gmail.com

Or do you want me to pick it up for my tree?

Thanks.

-- 
Dmitry
--
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

[RESEND PATCH] USB: xhci - fix bit definitions for IMAN register

[Dmitry Torokhov]

According to XHCI specification (5.5.2.1) the IP is bit 0 and IE is bit 1
of IMAN register. Previously their definitions were reversed.

Even though there are no ill effects being observed from the swapped
definitions (because IMAN_IP is RW1C and in legacy PCI case we come in
with it already set to 1 so it was clearing itself even though we were
setting IMAN_IE instead of IMAN_IP), we should still correct the values.

Signed-off-by: Dmitry Torokhov d...@vmware.com
---

 drivers/usb/host/xhci.h |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f791bd0..2c510e4 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -206,8 +206,8 @@ struct xhci_op_regs {
 /* bits 12:31 are reserved (and should be preserved on writes). */
 
 /* IMAN - Interrupt Management Register */
-#define IMAN_IP(1  1)
-#define IMAN_IE(1  0)
+#define IMAN_IE(1  1)
+#define IMAN_IP(1  0)
 
 /* USBSTS - USB status - status bitmasks */
 /* HC not running - set to 1 when run/stop bit is cleared. */
--
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

[PATCH] USB: remove incorrect __exit markups

[Dmitry Torokhov]

Even if bus is not hot-pluggable, the devices can be unbound from the
driver via sysfs, so we should not be using __exit annotations on
remove() methods. The only exception is drivers registered with
platform_driver_probe() which specifically disables sysfs bind/unbind
attributes.

Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
 drivers/usb/host/ehci-mxc.c| 2 +-
 drivers/usb/host/ehci-orion.c  | 4 ++--
 drivers/usb/host/ehci-sh.c | 4 ++--
 drivers/usb/otg/isp1301_omap.c | 4 ++--
 drivers/usb/otg/twl4030-usb.c  | 4 ++--
 drivers/usb/otg/twl6030-usb.c  | 4 ++--
 drivers/usb/phy/mv_u3d_phy.c   | 2 +-
 7 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
index dedb80b..85c99c3 100644
--- a/drivers/usb/host/ehci-mxc.c
+++ b/drivers/usb/host/ehci-mxc.c
@@ -199,7 +199,7 @@ err_alloc:
return ret;
 }
 
-static int __exit ehci_mxc_drv_remove(struct platform_device *pdev)
+static int ehci_mxc_drv_remove(struct platform_device *pdev)
 {
struct mxc_usbh_platform_data *pdata = pdev-dev.platform_data;
struct usb_hcd *hcd = platform_get_drvdata(pdev);
diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index 914a3ec..38c45fb 100644
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -305,7 +305,7 @@ err1:
return err;
 }
 
-static int __exit ehci_orion_drv_remove(struct platform_device *pdev)
+static int ehci_orion_drv_remove(struct platform_device *pdev)
 {
struct usb_hcd *hcd = platform_get_drvdata(pdev);
struct clk *clk;
@@ -333,7 +333,7 @@ MODULE_DEVICE_TABLE(of, ehci_orion_dt_ids);
 
 static struct platform_driver ehci_orion_driver = {
.probe  = ehci_orion_drv_probe,
-   .remove = __exit_p(ehci_orion_drv_remove),
+   .remove = ehci_orion_drv_remove,
.shutdown   = usb_hcd_platform_shutdown,
.driver = {
.name   = orion-ehci,
diff --git a/drivers/usb/host/ehci-sh.c b/drivers/usb/host/ehci-sh.c
index 0c90a24..2deef81 100644
--- a/drivers/usb/host/ehci-sh.c
+++ b/drivers/usb/host/ehci-sh.c
@@ -171,7 +171,7 @@ fail_create_hcd:
return ret;
 }
 
-static int __exit ehci_hcd_sh_remove(struct platform_device *pdev)
+static int ehci_hcd_sh_remove(struct platform_device *pdev)
 {
struct ehci_sh_priv *priv = platform_get_drvdata(pdev);
struct usb_hcd *hcd = priv-hcd;
@@ -197,7 +197,7 @@ static void ehci_hcd_sh_shutdown(struct platform_device 
*pdev)
 
 static struct platform_driver ehci_hcd_sh_driver = {
.probe  = ehci_hcd_sh_probe,
-   .remove = __exit_p(ehci_hcd_sh_remove),
+   .remove = ehci_hcd_sh_remove
.shutdown   = ehci_hcd_sh_shutdown,
.driver = {
.name   = sh_ehci,
diff --git a/drivers/usb/otg/isp1301_omap.c b/drivers/usb/otg/isp1301_omap.c
index af9cb11..8b9de95 100644
--- a/drivers/usb/otg/isp1301_omap.c
+++ b/drivers/usb/otg/isp1301_omap.c
@@ -1212,7 +1212,7 @@ static void isp1301_release(struct device *dev)
 
 static struct isp1301 *the_transceiver;
 
-static int __exit isp1301_remove(struct i2c_client *i2c)
+static int isp1301_remove(struct i2c_client *i2c)
 {
struct isp1301  *isp;
 
@@ -1634,7 +1634,7 @@ static struct i2c_driver isp1301_driver = {
.name   = isp1301_omap,
},
.probe  = isp1301_probe,
-   .remove = __exit_p(isp1301_remove),
+   .remove = isp1301_remove,
.id_table   = isp1301_id,
 };
 
diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
index 0a70193..4e04579 100644
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -657,7 +657,7 @@ static int twl4030_usb_probe(struct platform_device *pdev)
return 0;
 }
 
-static int __exit twl4030_usb_remove(struct platform_device *pdev)
+static int twl4030_usb_remove(struct platform_device *pdev)
 {
struct twl4030_usb *twl = platform_get_drvdata(pdev);
int val;
@@ -701,7 +701,7 @@ MODULE_DEVICE_TABLE(of, twl4030_usb_id_table);
 
 static struct platform_driver twl4030_usb_driver = {
.probe  = twl4030_usb_probe,
-   .remove = __exit_p(twl4030_usb_remove),
+   .remove = twl4030_usb_remove,
.driver = {
.name   = twl4030_usb,
.owner  = THIS_MODULE,
diff --git a/drivers/usb/otg/twl6030-usb.c b/drivers/usb/otg/twl6030-usb.c
index 8cd6cf4..7f3c5b0 100644
--- a/drivers/usb/otg/twl6030-usb.c
+++ b/drivers/usb/otg/twl6030-usb.c
@@ -393,7 +393,7 @@ static int twl6030_usb_probe(struct platform_device *pdev)
return 0;
 }
 
-static int __exit twl6030_usb_remove(struct platform_device *pdev)
+static int twl6030_usb_remove(struct platform_device *pdev)
 {
struct twl6030_usb *twl = platform_get_drvdata(pdev);
 
@@ -420,7 +420,7 @@ MODULE_DEVICE_TABLE

[PATCH 0/3] Introduce driver for IMS PCU devices

[Dmitry Torokhov]

IMS, an In Flight Entertainment System provider, has a number of seat displays
Linux. To interact with the user IMS uses a passenger Control Unit (PCU), which
communicates with Rave Display Unit via a USB interface.

Originally large part of the PCU handling was done from userspace and so it
presents itself as two distinct USB devices: one is a standard HID mouse and
another is a CDC-ACM modem-like device. The latter one was used by IMS
userspace software to get the status of all PCU buttons and perform other
operations. However the fact that with the custom userspace handling the device
resides outside of the standard input framework hinders use of the device
elsewhere in the stack and this patch series attempts to fix this issue by
creating a proper input driver for the device. If the device was purely input
device it would also be possible to use cdc-acm + userspace solution and loop
the input events back into the kernel via uinput, however the device also
allows control its key backlight, which exported as a standard LED device, and
this functionality is not available through uinput (nor should it be).
Firmware update is also implemented via the standard request_firmware
mechanism.

The patch series consists of 3 parts:

- a patch adding new keycodes needed for PCU;
- the driver itself;
- a patch to cdc-acm driver to ignore PCU devices when ims-pcu driver is
  enabled.

Thanks.

-- 
Dmitry
--
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

[PATCH 2/3] Input: add IMS Passenger Control Unit driver

[Dmitry Torokhov]

The PCU is a device installed in the armrest of a plane seat and
is connected to IMS Rave Entertainment System. It has a set of control
buttons (Volume Up/Down, Attendant, Lights, etc) on one side and
gamepad-like controls on the other side.

Originally the device was handled from userspace and because of that
it presents itself on USB bus as a CDC-ACM modem device that however
can not make calls. However the custom handling is not as convenient
as using standard input subsystem facilities. If it was pure input
device it would be possible to continue using userspace solution
(moving it over to uinput), but the device also has backlighted keys
which can not be supported via uinput.

Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
 drivers/input/misc/Kconfig   |   10 +
 drivers/input/misc/Makefile  |1 +
 drivers/input/misc/ims-pcu.c | 1907 ++
 3 files changed, 1918 insertions(+)
 create mode 100644 drivers/input/misc/ims-pcu.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 798ba29..9d32716 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -606,6 +606,16 @@ config INPUT_ADXL34X_SPI
  To compile this driver as a module, choose M here: the
  module will be called adxl34x-spi.
 
+config INPUT_IMS_PCU
+   tristate IMS Passenger Control Unit driver
+   depends on USB
+   depends on LEDS_CLASS
+   help
+ Say Y here if you have system with IMS Rave Passenger Control Unit.
+
+ To compile this driver as a module, choose M here: the module will be
+ called ims_pcu.
+
 config INPUT_CMA3000
tristate VTI CMA3000 Tri-axis accelerometer
help
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index ae5d9fd..33085784 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_INPUT_GP2A)  += gp2ap002a00f.o
 obj-$(CONFIG_INPUT_GPIO_BEEPER)+= gpio-beeper.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)   += gpio_tilt_polled.o
 obj-$(CONFIG_HP_SDC_RTC)   += hp_sdc_rtc.o
+obj-$(CONFIG_INPUT_IMS_PCU)+= ims-pcu.o
 obj-$(CONFIG_INPUT_IXP4XX_BEEPER)  += ixp4xx-beeper.o
 obj-$(CONFIG_INPUT_KEYSPAN_REMOTE) += keyspan_remote.o
 obj-$(CONFIG_INPUT_KXTJ9)  += kxtj9.o
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
new file mode 100644
index 000..10bd73c
--- /dev/null
+++ b/drivers/input/misc/ims-pcu.c
@@ -0,0 +1,1907 @@
+/*
+ * Driver for IMS Passenger Control Unit Devices
+ *
+ * Copyright (C) 2013 The IMS Company
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation.
+ */
+
+#include linux/completion.h
+#include linux/device.h
+#include linux/firmware.h
+#include linux/ihex.h
+#include linux/input.h
+#include linux/kernel.h
+#include linux/leds.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/types.h
+#include linux/usb/input.h
+#include linux/usb/cdc.h
+#include asm/unaligned.h
+
+#define IMS_PCU_KEYMAP_LEN 32
+
+struct ims_pcu_buttons {
+   struct input_dev *input;
+   char name[32];
+   char phys[32];
+   unsigned short keymap[IMS_PCU_KEYMAP_LEN];
+};
+
+struct ims_pcu_gamepad {
+   struct input_dev *input;
+   char name[32];
+   char phys[32];
+};
+
+struct ims_pcu_backlight {
+   struct led_classdev cdev;
+   struct work_struct work;
+   enum led_brightness desired_brightness;
+   char name[32];
+};
+
+#define IMS_PCU_PART_NUMBER_LEN15
+#define IMS_PCU_SERIAL_NUMBER_LEN  8
+#define IMS_PCU_DOM_LEN8
+#define IMS_PCU_FW_VERSION_LEN (9 + 1)
+#define IMS_PCU_BL_VERSION_LEN (9 + 1)
+#define IMS_PCU_BL_RESET_REASON_LEN(2 + 1)
+
+#define IMS_PCU_BUF_SIZE   128
+
+struct ims_pcu {
+   struct usb_device *udev;
+   struct device *dev; /* control interface's device, used for logging */
+
+   unsigned int device_no;
+
+   bool bootloader_mode;
+
+   char part_number[IMS_PCU_PART_NUMBER_LEN];
+   char serial_number[IMS_PCU_SERIAL_NUMBER_LEN];
+   char date_of_manufacturing[IMS_PCU_DOM_LEN];
+   char fw_version[IMS_PCU_FW_VERSION_LEN];
+   char bl_version[IMS_PCU_BL_VERSION_LEN];
+   char reset_reason[IMS_PCU_BL_RESET_REASON_LEN];
+   int update_firmware_status;
+
+   struct usb_interface *ctrl_intf;
+
+   struct usb_endpoint_descriptor *ep_ctrl;
+   struct urb *urb_ctrl;
+   u8 *urb_ctrl_buf;
+   dma_addr_t ctrl_dma;
+   size_t max_ctrl_size;
+
+   struct usb_interface *data_intf;
+
+   struct usb_endpoint_descriptor *ep_in;
+   struct urb *urb_in;
+   u8 *urb_in_buf;
+   dma_addr_t read_dma;
+   size_t max_in_size;
+
+   struct

[PATCH 3/3] USB: cdc-acm - blacklist IMS PCU device

[Dmitry Torokhov]

The IMS PCU (Passenger Control Unit) device used custom protocol over serial
line, so it is presenting itself as CDC ACM device.

Now that we have proper in-kernel driver for it we need to black-list the
device in cdc-acm driver.

Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
 drivers/usb/class/cdc-acm.c | 13 +
 drivers/usb/class/cdc-acm.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 2d92cce..3c8473d 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -987,6 +987,10 @@ static int acm_probe(struct usb_interface *intf,
 
/* normal quirks */
quirks = (unsigned long)id-driver_info;
+
+   if (quirks == IGNORE_DEVICE)
+   return -ENODEV;
+
num_rx_buf = (quirks == SINGLE_RX_URB) ? 1 : ACM_NR;
 
/* handle quirks deadly to normal probing*/
@@ -1691,6 +1695,15 @@ static const struct usb_device_id acm_ids[] = {
.driver_info = NO_DATA_INTERFACE,
},
 
+#if IS_ENABLED(CONFIG_INPUT_IMS_PCU)
+   { USB_DEVICE(0x04d8, 0x0082),   /* Application mode */
+   .driver_info = IGNORE_DEVICE,
+   },
+   { USB_DEVICE(0x04d8, 0x0083),   /* Bootloader mode */
+   .driver_info = IGNORE_DEVICE,
+   },
+#endif
+
/* control interfaces without any protocol set */
{ USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_ACM,
USB_CDC_PROTO_NONE) },
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index 35ef887..0f76e4a 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -128,3 +128,4 @@ struct acm {
 #define NO_CAP_LINE4
 #define NOT_A_MODEM8
 #define NO_DATA_INTERFACE  16
+#define IGNORE_DEVICE  32
-- 
1.7.11.7

--
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 5/5] kfifo: log based kfifo API

[Dmitry Torokhov]

Hi Yuanhan,

On Tue, Jan 08, 2013 at 10:57:53PM +0800, Yuanhan Liu wrote:
 The current kfifo API take the kfifo size as input, while it rounds
  _down_ the size to power of 2 at __kfifo_alloc. This may introduce
 potential issue.
 
 Take the code at drivers/hid/hid-logitech-dj.c as example:
 
   if (kfifo_alloc(djrcv_dev-notif_fifo,
DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct dj_report),
GFP_KERNEL)) {
 
 Where, DJ_MAX_NUMBER_NOTIFICATIONS is 8, and sizeo of(struct dj_report)
 is 15.
 
 Which means it wants to allocate a kfifo buffer which can store 8
 dj_report entries at once. The expected kfifo buffer size would be
 8 * 15 = 120 then. While, in the end, __kfifo_alloc will turn the
 size to rounddown_power_of_2(120) =  64, and then allocate a buf
 with 64 bytes, which I don't think this is the original author want.
 
 With the new log API, we can do like following:
 
   int kfifo_size_order = order_base_2(DJ_MAX_NUMBER_NOTIFICATIONS *
   sizeof(struct dj_report));
 
   if (kfifo_alloc(djrcv_dev-notif_fifo, kfifo_size_order, GFP_KERNEL)) {
 
 This make sure we will allocate enough kfifo buffer for holding
 DJ_MAX_NUMBER_NOTIFICATIONS dj_report entries.

Why don't you simply change __kfifo_alloc to round the allocation up
instead of down?

Thanks.

-- 
Dmitry
--
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

[PATCH] USB: xhci - fix bit definitions for IMAN register

[Dmitry Torokhov]

According to XHCI specification (5.5.2.1) the IP is bit 0 and IE is bit 1
of IMAN register. Previously their definitions were reversed.

Signed-off-by: Dmitry Torokhov d...@vmware.com
---

Sarah,

I did not see any ill effects from using the old definitions (I think
because IMAN_IP is RW1C and in legacy PCI case we come in with it
already set to 1 so it was clearing itself even though we were setting
IMAN_IE instead of IMAN_IP), and the new ones seem to be working as
well.

Thanks,
Dmitry

 drivers/usb/host/xhci.h |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f791bd0..2c510e4 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -206,8 +206,8 @@ struct xhci_op_regs {
 /* bits 12:31 are reserved (and should be preserved on writes). */
 
 /* IMAN - Interrupt Management Register */
-#define IMAN_IP(1  1)
-#define IMAN_IE(1  0)
+#define IMAN_IE(1  1)
+#define IMAN_IP(1  0)
 
 /* USBSTS - USB status - status bitmasks */
 /* HC not running - set to 1 when run/stop bit is cleared. */
--
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 v2 0/1] Input: xpad - Implement wireless controller LED setting and fix connect time LED setting

[Dmitry Torokhov]

On Fri, Nov 30, 2012 at 08:13:29PM -0800, Chris Moeller wrote:
 On Fri, 30 Nov 2012 14:30:23 -0800
 Dmitry Torokhov dmitry.torok...@gmail.com wrote:
 
  Hi Chris,
  
  On Friday, November 30, 2012 01:54:06 PM Chris Moeller wrote:
   I've submitted versions of this with prior patch sets, and this part
   was never accepted, possibly because it depended on other patches to
   work, or possibly because it wasn't so cleanly organized. This time,
   I've split the LED setting command off into its own static function,
   then call that on controller connect and optionally on LED setting
   commands. The static function does not include any locking, because
   locking inside the function which receives the incoming packets
   deadlocks the driver, and does not appear to be necessary anyway.
   
   It also removes all traces of the original bulk out function which
   was designed for the purpose of setting the LED status on connect,
   as I found that it actually does not work at all. It appears to try
   to send the data, but nothing actually happens to the controller
   LEDs.
  
  Attached is a reply I sent to on 7/4/11 to which you unfortunately
  never responded. The issue is that of force feedback (rumble) and LED
  share the same URB then access to that URB should be arbitrated. The
  attached message contains a patch that attempts to implement that
  arbitration, could you please try it out and see what changes are
  needed to make it work?
  
  Thanks.
  
 
 So sorry I missed your reply. That's what I get for filtering the
 mailing list messages past my inbox, then never following up on my
 filter/folder set for replies to my own messages. I recall you
 mentioned that potential race condition when I first submitted, but I
 forgot to do anything about it. I'm glad at least one of us has our
 stuff together. It seems to work just fine, but there may be a force
 feedback issue with the following test program, where it leaves the
 effect playing indefinitely after the program terminates, and then the
 controller itself ceases to respond until the module is removed and
 reloaded.

Just to confirm, you see this problem only with the patch being
discussed and do not see it without it, right?

-- 
Dmitry
--
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 v2 0/1] Input: xpad - Implement wireless controller LED setting and fix connect time LED setting

[Dmitry Torokhov]

Hi Chris,

On Friday, November 30, 2012 01:54:06 PM Chris Moeller wrote:
 I've submitted versions of this with prior patch sets, and this part
 was never accepted, possibly because it depended on other patches to
 work, or possibly because it wasn't so cleanly organized. This time,
 I've split the LED setting command off into its own static function,
 then call that on controller connect and optionally on LED setting
 commands. The static function does not include any locking, because
 locking inside the function which receives the incoming packets
 deadlocks the driver, and does not appear to be necessary anyway.
 
 It also removes all traces of the original bulk out function which was
 designed for the purpose of setting the LED status on connect, as I
 found that it actually does not work at all. It appears to try to send
 the data, but nothing actually happens to the controller LEDs.

Attached is a reply I sent to on 7/4/11 to which you unfortunately never
responded. The issue is that of force feedback (rumble) and LED share the
same URB then access to that URB should be arbitrated. The attached
message contains a patch that attempts to implement that arbitration,
could you please try it out and see what changes are needed to make it
work?

Thanks.

-- 
Dmitry---BeginMessage---
On Sun, Jun 12, 2011 at 05:49:49PM -0700, Chris Moeller wrote:
 This patch removes the non-functional bulk output URB method for setting
 XBox360 Wireless Controller player number indicators on controller
 activation, and replaces it with a functional IRQ output URB method. It
 also implements the LED command control for these devices.
 
 Signed-off-by: Chris Moeller kod...@gmail.com
 
 ---
 
 I chose to duplicate the LED command setting function in the
 xpad360w_process_packet function, as the other LED setting function is
 designed to require mutex locking, which I found to deadlock the driver
 when used in that manner. I will consider adding a lock, as testing with
 a rumble flooding application collided with the LED control and
 prevented it from setting the player number on connect. I'm not even
 sure how the mutex could be deadlocking in the input packet handler, or
 even what good it would do in that case, since the rumble setting
 functions don't lock it. In fact, only the LED setting function locks
 it.

If 2 functions share the same URB then we need to arbitrate access to
URB data buffers, etc, etc. I believe the patch below could be used as a
starting point.

Thanks.

-- 
Dmitry

Input: xpad - wireless LED setting

From: Chris Moeller kod...@gmail.com

This patch removes the non-functional bulk output URB method for setting
XBox360 Wireless Controller player number indicators on controller
activation, and replaces it with a functional IRQ output URB method. It
also implements the LED command control for these devices.

Signed-off-by: Chris Moeller kod...@gmail.com
Signed-off-by: Dmitry Torokhov d...@mail.ru
---

 drivers/input/joystick/xpad.c |  699 ++---
 1 files changed, 379 insertions(+), 320 deletions(-)


diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index d728875..e2dbe54 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -253,23 +253,28 @@ struct usb_xpad {
struct input_dev *dev;  /* input device interface */
struct usb_device *udev;/* usb device */
 
-   int pad_present;
+   int interface_number;
 
struct urb *irq_in; /* urb for interrupt in report */
unsigned char *idata;   /* input data */
dma_addr_t idata_dma;
 
-   struct urb *bulk_out;
-   unsigned char *bdata;
-
 #if defined(CONFIG_JOYSTICK_XPAD_FF) || defined(CONFIG_JOYSTICK_XPAD_LEDS)
struct urb *irq_out;/* urb for interrupt out report */
unsigned char *odata;   /* output data */
dma_addr_t odata_dma;
-   struct mutex odata_mutex;
+   spinlock_t odata_lock;
+   bool irq_out_pending;
+
+   bool led_pending;
+   int led_command;
+
+   bool ff_pending;
+   u16 rumble_strong;
+   u16 rumble_weak;
 #endif
 
-#if defined(CONFIG_JOYSTICK_XPAD_LEDS)
+#ifdef CONFIG_JOYSTICK_XPAD_LEDS
struct xpad_led *led;
 #endif
 
@@ -279,6 +284,369 @@ struct usb_xpad {
int xtype;  /* type of xbox device */
 };
 
+#ifdef CONFIG_JOYSTICK_XPAD_FF
+static bool xpad_format_rumble(struct usb_xpad *xpad, u16 strong, u16 weak)
+{
+   switch (xpad-xtype) {
+
+   case XTYPE_XBOX:
+   xpad-odata[0] = 0x00;
+   xpad-odata[1] = 0x06;
+   xpad-odata[2] = 0x00;
+   xpad-odata[3] = strong / 256;  /* left actuator */
+   xpad-odata[4] = 0x00;
+   xpad-odata[5] = weak / 256;/* right actuator */
+   xpad-irq_out-transfer_buffer_length = 6;
+
+   return true;
+
+   case XTYPE_XBOX360

Re: [PATCH 02/10] arm: at91: move platfarm_data to include/linux/platform_data/atmel.h

[Dmitry Torokhov]

On Wed, Nov 07, 2012 at 01:20:41PM +0100, Marc Kleine-Budde wrote:
 On 11/07/2012 12:22 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
  Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD plagn...@jcrosoft.com
  Cc: Nicolas Ferre nicolas.fe...@atmel.com
  Cc: linux-...@vger.kernel.org
  Cc: linux-in...@vger.kernel.org
  Cc: linux-...@vger.kernel.org
  Cc: linux-...@vger.kernel.org
  Cc: net...@vger.kernel.org
  Cc: linux-pcm...@lists.infradead.org
  Cc: rtc-li...@googlegroups.com
  Cc: spi-devel-gene...@lists.sourceforge.net
  Cc: linux-ser...@vger.kernel.org
  Cc: linux-usb@vger.kernel.org
  Cc: linux-fb...@vger.kernel.org
  ---
  HI all,
  
  If it's ok with everyone this will go via at91
  with the patch serie than clean up the include/mach
 
 Fine with me.
 
  For preparation to switch to arm multiarch kernel
 
 Acked-by: Marc Kleine-Budde m...@pengutronix.de (for the CAN related 
 changes)

Acked-by: Dmitry Torokhov dmitry.torok...@gmail.com

for input piece.

-- 
Dmitry
--
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: [ebeam PATCH v2 1/2] hid: Blacklist eBeam devices

[Dmitry Torokhov]

Jiri,

Are you OK with this change?

Yann,

Is the device usable at all with generic HID driver? If it isn't then
maybe we should blacklist it unconditionally?

Thanks.

On Sat, Oct 06, 2012 at 03:14:46PM +0200, Yann Cantin wrote:
 
 Signed-off-by: Yann Cantin yann.can...@laposte.net
 ---
  drivers/hid/hid-core.c |3 +++
  drivers/hid/hid-ids.h  |3 +++
  2 files changed, 6 insertions(+)
 
 diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
 index bd3971b..59ffaa2 100644
 --- a/drivers/hid/hid-core.c
 +++ b/drivers/hid/hid-core.c
 @@ -1937,6 +1937,9 @@ static const struct hid_device_id hid_ignore_list[] = {
   { HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20) 
 },
   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x0004) },
   { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x000a) },
 +#if defined(CONFIG_INPUT_EBEAM_USB)
 + { HID_USB_DEVICE(USB_VENDOR_ID_EFI, USB_DEVICE_ID_EFI_EBEAM) },
 +#endif
   { HID_USB_DEVICE(USB_VENDOR_ID_ESSENTIAL_REALITY, 
 USB_DEVICE_ID_ESSENTIAL_REALITY_P5) },
   { HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC5UH) },
   { HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC4UM) },
 diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
 index 269b509..df826b8 100644
 --- a/drivers/hid/hid-ids.h
 +++ b/drivers/hid/hid-ids.h
 @@ -274,6 +274,9 @@
  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72D0  0x72d0
  #define USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_72C4  0x72c4
  
 +#define USB_VENDOR_ID_EFI0x2650
 +#define USB_DEVICE_ID_EFI_EBEAM  0x1311
 +
  #define USB_VENDOR_ID_ELECOM 0x056e
  #define USB_DEVICE_ID_ELECOM_BM084   0x0061
  
 -- 
 1.7.10
 

-- 
Dmitry
--
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] drivers: phy: add generic PHY framework

[Dmitry Torokhov]

On Wednesday, September 26, 2012 09:57:57 AM Joe Perches wrote:
 On Wed, 2012-09-26 at 20:31 +0530, Kishon Vijay Abraham I wrote:
  The PHY framework provides a set of API's for the PHY drivers to
  create/destroy a PHY and API's
 
 Just some trivial notes.
 
  diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
 
 []
 
  @@ -0,0 +1,445 @@
 
 []
 
  +static void devm_phy_release(struct device *dev, void *res)
  +{
  +   struct phy *phy = *(struct phy **)res;
 
 That's a bit twisted isn't it?
 Perhaps
   struct phy *phy = res;

No, because you really need to dereference ptr, not simply cast.

...

  +
  +   ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
 
 Is this the right size?
 
 Because ptr is **, perhaps sizeof(struct phy) is clearer.

But incorrect.

Thanks.

-- 
Dmitry
--
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 resend2] Input: usbtouchscreen - initialize eGalax devices

[Dmitry Torokhov]

On Mon, Sep 03, 2012 at 01:33:50PM -0400, Forest Bond wrote:
 From: Forest Bond forest.b...@rapidrollout.com
 
 Certain eGalax devices expose an interface with class HID and protocol
 None.  Some work with usbhid and some work with usbtouchscreen, but
 there is no easy way to differentiate.  Sending an eGalax diagnostic
 packet seems to kick them all into using the right protocol for
 usbtouchscreen, so we can continue to bind them all there (as opposed to
 handing some off to usbhid).
 
 This fixes a regression for devices that were claimed by (and worked
 with) usbhid prior to commit 139ebe8dc80dd74cb2ac9f5603d18fbf5cff049f
 (Input: usbtouchscreen - fix eGalax HID ignoring), which made
 usbtouchscreen claim them instead.  With this patch they will still be
 claimed by usbtouchscreen, but they will actually report events
 usbtouchscreen can understand.  Note that these devices will be limited
 to the usbtouchscreen feature set so e.g. dual touch features are not
 supported.
 
 I have the distinct pleasure of needing to support devices of both types
 and have tested accordingly.
 
 Signed-off-by: Forest Bond forest.b...@rapidrollout.com

Applied, thank you Forest.

 ---
  drivers/input/touchscreen/usbtouchscreen.c |   39 
 
  1 files changed, 39 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/input/touchscreen/usbtouchscreen.c 
 b/drivers/input/touchscreen/usbtouchscreen.c
 index e32709e..c5f4dc0 100644
 --- a/drivers/input/touchscreen/usbtouchscreen.c
 +++ b/drivers/input/touchscreen/usbtouchscreen.c
 @@ -304,6 +304,44 @@ static int e2i_read_data(struct usbtouch_usb *dev, 
 unsigned char *pkt)
  #define EGALAX_PKT_TYPE_REPT 0x80
  #define EGALAX_PKT_TYPE_DIAG 0x0A
  
 +static int egalax_init(struct usbtouch_usb *usbtouch)
 +{
 + int ret, i;
 + unsigned char *buf;
 + struct usb_device *udev = interface_to_usbdev(usbtouch-interface);
 +
 + /* An eGalax diagnostic packet kicks the device into using the right
 +  * protocol.  We send a check active packet.  The response will be
 +  * read later and ignored.
 +  */
 +
 + buf = kmalloc(3, GFP_KERNEL);
 + if (!buf)
 + return -ENOMEM;
 +
 + buf[0] = EGALAX_PKT_TYPE_DIAG;
 + buf[1] = 1; /* length */
 + buf[2] = 'A';   /* command - check active */
 +
 + for (i = 0; i  3; i++) {
 + ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
 +   0,
 +   USB_DIR_OUT | USB_TYPE_VENDOR | 
 USB_RECIP_DEVICE,
 +   0, 0, buf, 3,
 +   USB_CTRL_SET_TIMEOUT);
 + if (ret = 0) {
 + ret = 0;
 + break;
 + }
 + if (ret != -EPIPE)
 + break;
 + }
 +
 + kfree(buf);
 +
 + return ret;
 +}
 +
  static int egalax_read_data(struct usbtouch_usb *dev, unsigned char *pkt)
  {
   if ((pkt[0]  EGALAX_PKT_TYPE_MASK) != EGALAX_PKT_TYPE_REPT)
 @@ -1056,6 +1094,7 @@ static struct usbtouch_device_info usbtouch_dev_info[] 
 = {
   .process_pkt= usbtouch_process_multi,
   .get_pkt_len= egalax_get_pkt_len,
   .read_data  = egalax_read_data,
 + .init   = egalax_init,
   },
  #endif
  
 -- 
 1.7.0.4

-- 
Dmitry
--
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] Input: usbtouchscreen - initialize eGalax devices

[Dmitry Torokhov]

On Fri, Aug 31, 2012 at 09:56:58AM -0400, Forest Bond wrote:
 From: Forest Bond forest.b...@rapidrollout.com
 
 Certain eGalax devices expose an interface with class HID and protocol
 None.  Some work with usbhid and some work with usbtouchscreen, but
 there is no easy way to differentiate.  Sending an eGalax diagnostic
 packet seems to kick them all into using the right protocol for
 usbtouchscreen, so we can continue to bind them all there (as opposed to
 handing some off to usbhid).
 
 This fixes a regression for devices that were claimed by (and worked
 with) usbhid prior to commit 139ebe8dc80dd74cb2ac9f5603d18fbf5cff049f,
 which made usbtouchscreen claim them instead.  With this patch they will
 still be claimed by usbtouchscreen, but they will actually report events
 usbtouchscreen can understand.  Note that these devices will be limited
 to the usbtouchscreen feature set so e.g. dual touch features are not
 supported.
 
 I have the distinct pleasure of needing to support devices of both types
 and have tested accordingly.
 
 Signed-off-by: Forest Bond forest.b...@rapidrollout.com

Applied, thanks Forest.

 ---
  drivers/input/touchscreen/usbtouchscreen.c |   25 +
  1 files changed, 25 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/input/touchscreen/usbtouchscreen.c 
 b/drivers/input/touchscreen/usbtouchscreen.c
 index e32709e..2ce5308 100644
 --- a/drivers/input/touchscreen/usbtouchscreen.c
 +++ b/drivers/input/touchscreen/usbtouchscreen.c
 @@ -304,6 +304,30 @@ static int e2i_read_data(struct usbtouch_usb *dev, 
 unsigned char *pkt)
  #define EGALAX_PKT_TYPE_REPT 0x80
  #define EGALAX_PKT_TYPE_DIAG 0x0A
  
 +static int egalax_init(struct usbtouch_usb *usbtouch)
 +{
 + int ret, i;
 + struct usb_device *udev = interface_to_usbdev(usbtouch-interface);
 +
 + /* An eGalax diagnostic packet kicks the device into using the right
 +  * protocol. */
 + for (i = 0; i  3; i++) {
 + /* Send a check active packet. The response will be read
 +  * later and ignored. */
 + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 +   0,
 +   USB_DIR_OUT | USB_TYPE_VENDOR | 
 USB_RECIP_DEVICE,
 +   0, 0, \x0A\x01A, 0,
 +   USB_CTRL_SET_TIMEOUT);
 + if (ret = 0)
 + break;
 + if (ret != -EPIPE)
 + return ret;
 + }
 +
 + return 0;
 +}
 +
  static int egalax_read_data(struct usbtouch_usb *dev, unsigned char *pkt)
  {
   if ((pkt[0]  EGALAX_PKT_TYPE_MASK) != EGALAX_PKT_TYPE_REPT)
 @@ -1056,6 +1080,7 @@ static struct usbtouch_device_info usbtouch_dev_info[] 
 = {
   .process_pkt= usbtouch_process_multi,
   .get_pkt_len= egalax_get_pkt_len,
   .read_data  = egalax_read_data,
 + .init   = egalax_init,
   },
  #endif
  
 -- 
 1.7.0.4

-- 
Dmitry
--
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] Input: usbtouchscreen - initialize eGalax devices

[Dmitry Torokhov]

On Fri, Aug 31, 2012 at 10:50:38PM +0400, Sergei Shtylyov wrote:
 Hello.
 
 On 08/31/2012 05:56 PM, Forest Bond wrote:
 
  From: Forest Bond forest.b...@rapidrollout.com
 
  Certain eGalax devices expose an interface with class HID and protocol
  None.  Some work with usbhid and some work with usbtouchscreen, but
  there is no easy way to differentiate.  Sending an eGalax diagnostic
  packet seems to kick them all into using the right protocol for
  usbtouchscreen, so we can continue to bind them all there (as opposed to
  handing some off to usbhid).
 
  This fixes a regression for devices that were claimed by (and worked
  with) usbhid prior to commit 139ebe8dc80dd74cb2ac9f5603d18fbf5cff049f,
 
Please also specify that commit's summary ion parens.
 
  which made usbtouchscreen claim them instead.  With this patch they will
  still be claimed by usbtouchscreen, but they will actually report events
  usbtouchscreen can understand.  Note that these devices will be limited
  to the usbtouchscreen feature set so e.g. dual touch features are not
  supported.
 
  I have the distinct pleasure of needing to support devices of both types
  and have tested accordingly.
 
  Signed-off-by: Forest Bond forest.b...@rapidrollout.com
  ---
   drivers/input/touchscreen/usbtouchscreen.c |   25 +
   1 files changed, 25 insertions(+), 0 deletions(-)
 
  diff --git a/drivers/input/touchscreen/usbtouchscreen.c 
  b/drivers/input/touchscreen/usbtouchscreen.c
  index e32709e..2ce5308 100644
  --- a/drivers/input/touchscreen/usbtouchscreen.c
  +++ b/drivers/input/touchscreen/usbtouchscreen.c
  @@ -304,6 +304,30 @@ static int e2i_read_data(struct usbtouch_usb *dev, 
  unsigned char *pkt)
   #define EGALAX_PKT_TYPE_REPT   0x80
   #define EGALAX_PKT_TYPE_DIAG   0x0A
   
  +static int egalax_init(struct usbtouch_usb *usbtouch)
  +{
  +   int ret, i;
  +   struct usb_device *udev = interface_to_usbdev(usbtouch-interface);
  +
  +   /* An eGalax diagnostic packet kicks the device into using the right
  +* protocol. */
 
The preferred multi-line comment style is:
 
 /*
  * bla
  * bla
  */
 
  +   for (i = 0; i  3; i++) {
  +   /* Send a check active packet. The response will be read
  +* later and ignored. */
  +   ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
  + 0,
  + USB_DIR_OUT | USB_TYPE_VENDOR | 
  USB_RECIP_DEVICE,
  + 0, 0, \x0A\x01A, 0,
 
You probably can't send data from the .const section (as well as off the
 stack) -- they can be DMA'ed and there'll be issues with cache consistency on
 non-x86 arches. You should allocate the data with kmalloc().
Although, on the second thought, maybe I'm wrong in this case... not really
 sure about sending -- receiving (to the .data section) could certainly be 
 harmful...

Hmm, do we actually send anything here? The size passed to
usb_control_msg() is 0 so I don't think we use that data at all...

Thanks.

-- 
Dmitry
--
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] Input: usbtouchscreen - initialize eGalax devices

[Dmitry Torokhov]

On Fri, Aug 31, 2012 at 06:53:53PM -0400, Forest Bond wrote:
 Hi,
 
 On Fri, Aug 31, 2012 at 04:04:58PM -0400, Alan Stern wrote:
  On Fri, 31 Aug 2012, Dmitry Torokhov wrote:
  
 + /* Send a check active packet. The response will be 
 read
 +  * later and ignored. */
 + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 +   0,
 +   USB_DIR_OUT | USB_TYPE_VENDOR | 
 USB_RECIP_DEVICE,
 +   0, 0, \x0A\x01A, 0,

   You probably can't send data from the .const section (as well as off 
the
stack) -- they can be DMA'ed and there'll be issues with cache 
consistency on
non-x86 arches. You should allocate the data with kmalloc().
   Although, on the second thought, maybe I'm wrong in this case... not 
really
sure about sending -- receiving (to the .data section) could certainly 
be harmful...
   
   Hmm, do we actually send anything here? The size passed to
   usb_control_msg() is 0 so I don't think we use that data at all...
  
  Good point.  Perhaps the 0 is a typo, in which case data does get sent
  and the buffer must be kmalloc'ed.  If the 0 is correct then the buffer
  should be NULL, not \x0A\x01A (and what's the purpose of the leading
  '0' in the second byte?).
  
  In addition, although the bRequestType specifies USB_DIR_OUT, the pipe
  value is usb_rcvctrlpipe.  Is this transfer meant to be IN or OUT?
 
 Thanks again to all for the review.  My theory for why the previous patch 
 worked
 in spite of its wrongness is that the device actually switches modes when it
 receives a control message with USB_TYPE_VENDOR even though the documentation
 suggests an actual diagnostic packet must be received.
 
 Does this (untested) patch look more reasonable?

Yes, but we still need it tested, please.

 
 
 diff --git a/drivers/input/touchscreen/usbtouchscreen.c 
 b/drivers/input/touchscreen/usbtouchscreen.c
 index e32709e..64b4b17 100644
 --- a/drivers/input/touchscreen/usbtouchscreen.c
 +++ b/drivers/input/touchscreen/usbtouchscreen.c
 @@ -304,6 +304,41 @@ static int e2i_read_data(struct usbtouch_usb *dev, 
 unsigned char *pkt)
  #define EGALAX_PKT_TYPE_REPT 0x80
  #define EGALAX_PKT_TYPE_DIAG 0x0A
  
 +static int egalax_init(struct usbtouch_usb *usbtouch)
 +{
 + int ret, i;
 + unsigned char *buf;
 + struct usb_device *udev = interface_to_usbdev(usbtouch-interface);
 +
 + /* An eGalax diagnostic packet kicks the device into using the right
 +  * protocol.  We send a check active packet.  The response will be
 +  * read later and ignored.
 +  */
 +
 + buf = kmalloc(3, GFP_KERNEL);
 + buf[0] = EGALAX_PKT_TYPE_DIAG;
 + buf[1] = 1; /* length */
 + buf[2] = 'A';   /* command - check active */
 +
 + for (i = 0; i  3; i++) {
 + ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
 +   0,
 +   USB_DIR_OUT | USB_TYPE_VENDOR | 
 USB_RECIP_DEVICE,
 +   0, 0, buf, 3,
 +   USB_CTRL_SET_TIMEOUT);
 + if (ret = 0) {
 + ret = 0;
 + break;
 + }
 + if (ret != -EPIPE)
 + break;
 + }
 +
 + kfree(buf);
 +
 + return ret;
 +}
 +
  static int egalax_read_data(struct usbtouch_usb *dev, unsigned char *pkt)
  {
   if ((pkt[0]  EGALAX_PKT_TYPE_MASK) != EGALAX_PKT_TYPE_REPT)
 @@ -1056,6 +1091,7 @@ static struct usbtouch_device_info usbtouch_dev_info[] 
 = {
   .process_pkt= usbtouch_process_multi,
   .get_pkt_len= egalax_get_pkt_len,
   .read_data  = egalax_read_data,
 + .init   = egalax_init,
   },
  #endif
  
 
 Thanks,
 Forest
 -- 
 Forest Bond
 http://www.alittletooquiet.net
 http://www.rapidrollout.com



-- 
Dmitry
--
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: [RFC ebeam PATCH v3 1/2] hid: Blacklist new eBeam classic device

[Dmitry Torokhov]

On Monday, August 06, 2012 02:43:40 PM Greg KH wrote:
 On Mon, Aug 06, 2012 at 11:21:43PM +0200, Yann Cantin wrote:
  Signed-off-by: Yann Cantin yann.can...@laposte.net
  ---
  
   drivers/hid/hid-core.c |3 +++
   drivers/hid/hid-ids.h  |3 +++
   2 files changed, 6 insertions(+)
  
  diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
  index 60ea284..b1ed8ee 100644
  --- a/drivers/hid/hid-core.c
  +++ b/drivers/hid/hid-core.c
  @@ -1908,6 +1908,9 @@ static const struct hid_device_id hid_ignore_list[]
  = { 
  { HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20)
  },
  { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x0004) },
  { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x000a) },
  
  +#if defined(CONFIG_INPUT_EBEAM_USB)
  +   { HID_USB_DEVICE(USB_VENDOR_ID_EFI, USB_DEVICE_ID_EFI_CLASSIC) },
  +#endif
 
 Why is this #if in here?  Just always do it, how could it not be
 defined?

User might disable the driver and CONFIG_INPUT_EBEAM_USB will not be
set. But I agree, since the device is unusable with generic HID driver
there is no point in doing this conditionally.

-- 
Dmitry
--
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: [RFC ebeam PATCH v3 0/2]

[Dmitry Torokhov]

On Monday, August 06, 2012 02:44:23 PM Greg KH wrote:
 On Mon, Aug 06, 2012 at 11:21:42PM +0200, Yann Cantin wrote:
  Hi,
  
  New USB input driver for eBeam devices.
  
  Currently, only the Luidia eBeam classic projection model is supported.
  Edge model and a NEC interactive video-projector support planned for the
  end of the mounth.
  
  Patch 1 to blacklist the device for hid generic-usb.
  
  Patch 2 is the actual driver.
  
  Changes from previous :
  - switch to div64_s64 for portable 64/64-bits divisions

Do you really need this much precision? It will be slower on 32 bits..

  - some cosmetics in device name
  - unused include and def removed
  - variables name changes for readability
  
  Pending issues :
  
  - sysfs custom files : need to pass 13 parameters for calibration :
choice is between lots of simply-handled, or few with a big sscanf.
 
 sysfs is one value per file, so use lots of different files please.

This is kind of a one value though - it is a transformation matrix.
Maybe switch it to binary - 9 s32?

-- 
Dmitry
--
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: [RFC ebeam PATCH v3 2/2] input: misc: New USB eBeam input driver.

[Dmitry Torokhov]

On Tue, Aug 07, 2012 at 02:56:40AM +0200, Yann Cantin wrote:
 Hi,
 
 Le 06/08/2012 23:43, Greg KH a écrit :
  On Mon, Aug 06, 2012 at 11:21:44PM +0200, Yann Cantin wrote:
 
  Signed-off-by: Yann Cantin yann.can...@laposte.net
  ---
   drivers/input/misc/ebeam.c |  764 
  
   1 file changed, 764 insertions(+)
   create mode 100644 drivers/input/misc/ebeam.c
  
  What adds this file to the build?
  
 Sorry, i don't get it : what do you mean ?

Greg meant that you forgot to include Makefile and Kconfig changes with
this patch.

-- 
Dmitry
--
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: [RFC ebeam PATCH v3 1/2] hid: Blacklist new eBeam classic device

[Dmitry Torokhov]

On Tue, Aug 07, 2012 at 03:21:45AM +0200, Yann Cantin wrote:
 Le 07/08/2012 00:07, Dmitry Torokhov a écrit :
  On Monday, August 06, 2012 02:43:40 PM Greg KH wrote:
  On Mon, Aug 06, 2012 at 11:21:43PM +0200, Yann Cantin wrote:
  Signed-off-by: Yann Cantin yann.can...@laposte.net
  ---
 
   drivers/hid/hid-core.c |3 +++
   drivers/hid/hid-ids.h  |3 +++
   2 files changed, 6 insertions(+)
 
  diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
  index 60ea284..b1ed8ee 100644
  --- a/drivers/hid/hid-core.c
  +++ b/drivers/hid/hid-core.c
  @@ -1908,6 +1908,9 @@ static const struct hid_device_id hid_ignore_list[]
  = { 
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20)
},
{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x0004) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, 0x000a) },
 
  +#if defined(CONFIG_INPUT_EBEAM_USB)
  + { HID_USB_DEVICE(USB_VENDOR_ID_EFI, USB_DEVICE_ID_EFI_CLASSIC) },
  +#endif
 
  Why is this #if in here?  Just always do it, how could it not be
  defined?
  
  User might disable the driver and CONFIG_INPUT_EBEAM_USB will not be
  set. But I agree, since the device is unusable with generic HID driver
  there is no point in doing this conditionally.
 
 There's a closed-source user-space stack (libusb based daemon + xorg driver
 + wine apps) provided for some distro (Ubuntu 10.04, works on mandriva 2010,
 maybe others but break on recent xorg).
 
 I don't know exactly what to do : i don't want to break hypothetical support,
 even proprietary.
 Leaving the choice at kernel compile time seems to be safer, no ?

If they are using libusb that means that they use userspace solution and
do not require HID or any other in-kernel driver. They should still be
able to claim the port even if your driver is in use.

Thanks.

-- 
Dmitry
--
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: [RFC ebeam PATCH 3/3] input: misc: New USB eBeam input driver.

[Dmitry Torokhov]

Hi Yann,

On Sat, Jul 28, 2012 at 02:02:34AM +0200, Yann Cantin wrote:
 
 Signed-off-by: Yann Cantin yann.can...@laposte.net
 ---
  drivers/input/misc/Kconfig  |   21 +
  drivers/input/misc/Makefile |1 +
  drivers/input/misc/ebeam.c  |  895 
 +++
  3 files changed, 917 insertions(+)
  create mode 100644 drivers/input/misc/ebeam.c
 
 diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
 index 7faf4a7..0e798cb 100644
 --- a/drivers/input/misc/Kconfig
 +++ b/drivers/input/misc/Kconfig
 @@ -73,6 +73,27 @@ config INPUT_BMA150
 To compile this driver as a module, choose M here: the
 module will be called bma150.
  
 +config INPUT_EBEAM_USB
 + tristate USB eBeam driver
 + depends on USB_ARCH_HAS_HCD
 + select USB
 + help
 +   Say Y here if you have a USB eBeam pointing device and want to
 +   use it without any proprietary user space tools.
 +
 +   Have a look at http://sourceforge.net/projects/ebeam/ for
 +   a usage description and the required user-space tools.
 +
 +   Currently, only the Classic Projection model is supported.
 +
 +   To compile this driver as a module, choose M here: the
 +   module will be called ebeam.
 +
 +config INPUT_EBEAM_USB_CLASSIC
 + bool eBeam Classic Projection support
 + depends on INPUT_EBEAM_USB
 + default y

Will there be support for other eBean devices (are there any)? If there
will how soon? How different are they? If not the we probably do not
need this INPUT_EBEAM_USB_CLASSIC selector.

 +
  config INPUT_PCSPKR
   tristate PC Speaker support
   depends on PCSPKR_PLATFORM
 diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
 index f55cdf4..4b5e4a9 100644
 --- a/drivers/input/misc/Makefile
 +++ b/drivers/input/misc/Makefile
 @@ -23,6 +23,7 @@ obj-$(CONFIG_INPUT_CMA3000_I2C) += 
 cma3000_d0x_i2c.o
  obj-$(CONFIG_INPUT_COBALT_BTNS)  += cobalt_btns.o
  obj-$(CONFIG_INPUT_DA9052_ONKEY) += da9052_onkey.o
  obj-$(CONFIG_INPUT_DM355EVM) += dm355evm_keys.o
 +obj-$(CONFIG_INPUT_EBEAM_USB)+= ebeam.o
  obj-$(CONFIG_INPUT_GP2A) += gp2ap002a00f.o
  obj-$(CONFIG_INPUT_GPIO_TILT_POLLED) += gpio_tilt_polled.o
  obj-$(CONFIG_HP_SDC_RTC) += hp_sdc_rtc.o
 diff --git a/drivers/input/misc/ebeam.c b/drivers/input/misc/ebeam.c
 new file mode 100644
 index 000..a18615a
 --- /dev/null
 +++ b/drivers/input/misc/ebeam.c
 @@ -0,0 +1,895 @@
 +/**
 + *
 + * eBeam driver
 + *
 + * Copyright (C) 2012 Yann Cantin (yann.can...@laposte.net)
 + *
 + *   This program is free software; you can redistribute it and/or
 + *   modify it under the terms of the GNU General Public License as
 + *   published by the Free Software Foundation; either version 2 of the
 + *   License, or (at your option) any later version.
 + *
 + *  based on
 + *
 + *   usbtouchscreen.c by Daniel Ritz daniel.r...@gmx.ch
 + *   aiptek.c (sysfs/settings) by Chris Atenasio ch...@crud.net
 + *Bryan W. Headley bwhead...@earthlink.net
 + *
 + 
 */
 +
 +#define DEBUG

I do not think leaving DEBUG on is good idea for production code.

 +
 +#include linux/kernel.h
 +#include linux/slab.h
 +#include linux/input.h
 +#include linux/module.h
 +#include linux/init.h
 +#include linux/usb.h
 +#include linux/usb/input.h
 +#include linux/hid.h
 +
 +#define DRIVER_VERSION   v0.5
 +#define DRIVER_AUTHORYann Cantin yann.can...@laposte.net
 +#define DRIVER_DESC  USB eBeam Driver
 +
 +#define USB_VENDOR_ID_EFI   0x2650   /* Electronics For Imaging, Inc   */
 +#define USB_DEVICE_ID_EFI_CLASSIC  0x1311   /* Classic projection La 
 banane */
 +
 +#define EBEAM_BTN_TIP0x1  /* tip*/
 +#define EBEAM_BTN_LIT0x2  /* little */
 +#define EBEAM_BTN_BIG0x4  /* big*/
 +
 +/* until KConfig */
 +#define CONFIG_INPUT_EBEAM_USB_CLASSIC

Huh?

 +
 +/* device specifc data/functions */
 +struct ebeam_device;
 +struct ebeam_device_info {
 + int min_X;
 + int max_X;
 + int min_Y;
 + int max_Y;
 +
 + /*
 +  * TODO : Check if it's really necessary, waiting for other device info.
 +  * Always service the USB devices irq not just when the input device is
 +  * open. This is useful when devices have a watchdog which prevents us
 +  * from periodically polling the device. Leave this unset unless your
 +  * ebeam device requires it, as it does consume more of the USB
 +  * bandwidth.
 +  */
 + bool irq_always;

Does you device need this?

 +
 + int rept_size;
 +
 + /* optional, generic exist */
 + void (*process_pkt)  (struct ebeam_device *ebeam,
 +   unsigned char *pkt,
 +   

Re: [PATCH v3 1/1] Input: xpad - Handle all variations of Mad Catz Beat Pad

[Dmitry Torokhov]

Hi Yuri,

On Wed, Jul 11, 2012 at 12:33:22AM +0700, Yuri Khan wrote:
 * Add this device to usbhid ignore list

Please do not forget your Signed-off-by:  so that I can apply the
patch.

Thanks.

 ---
  drivers/hid/hid-core.c|1 +
  drivers/hid/hid-ids.h |3 +++
  drivers/input/joystick/xpad.c |1 +
  3 files changed, 5 insertions(+)
 
 diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
 index 6ac0286..1540934 100644
 --- a/drivers/hid/hid-core.c
 +++ b/drivers/hid/hid-core.c
 @@ -1995,6 +1995,7 @@ static const struct hid_device_id hid_ignore_list[] = {
   { HID_USB_DEVICE(USB_VENDOR_ID_LD, USB_DEVICE_ID_LD_MCT) },
   { HID_USB_DEVICE(USB_VENDOR_ID_LD, USB_DEVICE_ID_LD_HYBRID) },
   { HID_USB_DEVICE(USB_VENDOR_ID_LD, USB_DEVICE_ID_LD_HEATCONTROL) },
 + { HID_USB_DEVICE(USB_VENDOR_ID_MADCATZ, USB_DEVICE_ID_MADCATZ_BEATPAD) 
 },
   { HID_USB_DEVICE(USB_VENDOR_ID_MCC, USB_DEVICE_ID_MCC_PMD1024LS) },
   { HID_USB_DEVICE(USB_VENDOR_ID_MCC, USB_DEVICE_ID_MCC_PMD1208LS) },
   { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICKIT1) },
 diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
 index d1cdd2d..43c3d75 100644
 --- a/drivers/hid/hid-ids.h
 +++ b/drivers/hid/hid-ids.h
 @@ -518,6 +518,9 @@
  #define USB_DEVICE_ID_CRYSTALTOUCH   0x0006
  #define USB_DEVICE_ID_CRYSTALTOUCH_DUAL  0x0007
  
 +#define USB_VENDOR_ID_MADCATZ0x0738
 +#define USB_DEVICE_ID_MADCATZ_BEATPAD0x4540
 +
  #define USB_VENDOR_ID_MCC0x09db
  #define USB_DEVICE_ID_MCC_PMD1024LS  0x0076
  #define USB_DEVICE_ID_MCC_PMD1208LS  0x007a
 diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
 index ee16fb6..16974ef 100644
 --- a/drivers/input/joystick/xpad.c
 +++ b/drivers/input/joystick/xpad.c
 @@ -238,6 +238,7 @@ static struct usb_device_id xpad_table [] = {
   XPAD_XBOX360_VENDOR(0x045e),/* Microsoft X-Box 360 
 controllers */
   XPAD_XBOX360_VENDOR(0x046d),/* Logitech X-Box 360 style 
 controllers */
   XPAD_XBOX360_VENDOR(0x0738),/* Mad Catz X-Box 360 
 controllers */
 + { USB_DEVICE(0x0738, 0x4540) }, /* Mad Catz Beat Pad */
   XPAD_XBOX360_VENDOR(0x0e6f),/* 0x0e6f X-Box 360 controllers 
 */
   XPAD_XBOX360_VENDOR(0x12ab),/* X-Box 360 dance pads */
   XPAD_XBOX360_VENDOR(0x1430),/* RedOctane X-Box 360 
 controllers */
 -- 
 1.7.9.5
 

-- 
Dmitry
--
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 0/1] Input: xpad - Add a variation of Mad Catz Beat Pad

[Dmitry Torokhov]

On Fri, Jul 06, 2012 at 11:57:44PM +0700, Yuri Khan wrote:
 On Fri, Jul 6, 2012 at 11:32 PM, Yuri Khan yurivk...@gmail.com wrote:
 
  When I add a usbhid option quirks=0x0738:0x4540:0x4 (so that usbhid does
  not attempt to handle this device) and rebuild the xpad module with the
  following patch, the device works as expected. Dmitry Torokhov, the
  current maintainer of input drivers, suggested that I include a change
  to add the usbhid quirk in my patch.
 
 Of course the good idea only ever comes after the fact. If I change
 usbhid to ignore this vendor:device unconditionally, then xpad should
 also always handle it regardless of interface class/subclass/protocol,
 right?

Yes.

-- 
Dmitry
--
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