Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup
On Mon, Sep 11, 2017 at 4:29 PM, Benson Leungwrote: > 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
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
On Mon, Sep 11, 2017 at 8:15 AM, Andrey Konovalovwrote: > 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
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
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
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
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
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
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
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
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
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
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
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
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).
On Wed, Apr 6, 2016 at 3:27 AM, Sedat Dilekwrote: > 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
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
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
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
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
On Mon, Mar 14, 2016 at 9:46 AM, Josh Boyerwrote: > 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
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
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
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
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
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
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
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)
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
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
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
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
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
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
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
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
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
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
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
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
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
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()
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
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()
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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]
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.
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
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.
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
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
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