| Message ID | 20150207154859.89a7e4e3.cand@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi Lauri, On Sat, Feb 7, 2015 at 8:48 AM, Lauri Kasanen <cand@gmx.com> wrote: > Without this, my "Gasia Co.,Ltd PS(R) Gamepad" would not send > any events. Now everything works including the leds. > > Based on work by Andrew Haines and Antonio Ospite. > > cc: Antonio Ospite <ao2@ao2.it> > cc: Andrew Haines <AndrewD207@aol.com> > Signed-off-by: Lauri Kasanen <cand@gmx.com> > --- > drivers/hid/hid-sony.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > Antonio's original approach was not enough; it enabled the events, > but only for a few seconds, then the controller timed out and sent > no more. Andrew's did more than was necessary. This is a combination > of the two, against Linus' git. > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index 31e9d25..de93386 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -36,6 +36,7 @@ > #include <linux/list.h> > #include <linux/idr.h> > #include <linux/input/mt.h> > +#include <linux/usb/input.h> Please don't. HID should be transport agnostic, so please refrain from directly call usb. > > #include "hid-ids.h" > > @@ -1130,8 +1131,12 @@ static void sony_input_configured(struct hid_device *hdev, > */ > static int sixaxis_set_operational_usb(struct hid_device *hdev) > { > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > + struct usb_device *dev = interface_to_usbdev(intf); > int ret; > - char *buf = kmalloc(18, GFP_KERNEL); > + char *buf = kmalloc(65, GFP_KERNEL); 18 or 65 should be retrieved by the description of the device, not hardcoded. > + unsigned char buf2[] = { 0x00 }; > + int transfered; > > if (!buf) > return -ENOMEM; > @@ -1140,7 +1145,24 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev) > HID_REQ_GET_REPORT); > > if (ret < 0) > - hid_err(hdev, "can't set operational mode\n"); > + hid_err(hdev, "can't set operational mode on the control EP\n"); > + > + /* > + * Some compatible controllers like the Speedlink Strike FX and > + * Gasia need another query plus an USB interrupt to get operational. > + */ > + ret = hid_hw_raw_request(hdev, 0xf5, buf, 64, HID_FEATURE_REPORT, > + HID_REQ_GET_REPORT); 0xf5 should not be hardcoded. You have to retrieve it from the description of the device or at least put a special case for your specific game controller. > + > + if (ret < 0) > + hid_err(hdev, "can't set operational mode on the interrupt EP\n"); > + > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > + buf2, sizeof(buf2), > + &transfered, USB_CTRL_SET_TIMEOUT); Can't you simply use a hid_hw_output_report request instead of hard coding the device specific endpoint? And I'd also prefer it to be guarded against your specific controller. Cheers, Benjamin > + > + if (ret < 0) > + hid_err(hdev, "can't set operational mode on the interrupt EP\n"); > > kfree(buf); > > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 7 Feb 2015 10:56:49 -0500 Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > Hi Lauri, > > On Sat, Feb 7, 2015 at 8:48 AM, Lauri Kasanen <cand@gmx.com> wrote: > > Without this, my "Gasia Co.,Ltd PS(R) Gamepad" would not send > > any events. Now everything works including the leds. > > > > Based on work by Andrew Haines and Antonio Ospite. > > > > cc: Antonio Ospite <ao2@ao2.it> > > cc: Andrew Haines <AndrewD207@aol.com> > > Signed-off-by: Lauri Kasanen <cand@gmx.com> > > --- > > drivers/hid/hid-sony.c | 26 ++++++++++++++++++++++++-- > > 1 file changed, 24 insertions(+), 2 deletions(-) > > > > Antonio's original approach was not enough; it enabled the events, > > but only for a few seconds, then the controller timed out and sent > > no more. Andrew's did more than was necessary. This is a combination > > of the two, against Linus' git. > > > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > > index 31e9d25..de93386 100644 > > --- a/drivers/hid/hid-sony.c > > +++ b/drivers/hid/hid-sony.c > > @@ -36,6 +36,7 @@ > > #include <linux/list.h> > > #include <linux/idr.h> > > #include <linux/input/mt.h> > > +#include <linux/usb/input.h> > > Please don't. > HID should be transport agnostic, so please refrain from directly call usb. > I agree with Benjamin here. > > > > #include "hid-ids.h" > > > > @@ -1130,8 +1131,12 @@ static void sony_input_configured(struct hid_device *hdev, > > */ > > static int sixaxis_set_operational_usb(struct hid_device *hdev) > > { > > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > + struct usb_device *dev = interface_to_usbdev(intf); > > int ret; > > - char *buf = kmalloc(18, GFP_KERNEL); > > + char *buf = kmalloc(65, GFP_KERNEL); > > 18 or 65 should be retrieved by the description of the device, not hardcoded. > However I don't mind about hardcoding these values here. The device report descriptor does not describe all the feature reports, it never has and it's not really worth doing that IMHO. > > + unsigned char buf2[] = { 0x00 }; > > + int transfered; > > > > if (!buf) > > return -ENOMEM; > > @@ -1140,7 +1145,24 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev) > > HID_REQ_GET_REPORT); > > > > if (ret < 0) > > - hid_err(hdev, "can't set operational mode\n"); > > + hid_err(hdev, "can't set operational mode on the control EP\n"); > > + > > + /* > > + * Some compatible controllers like the Speedlink Strike FX and > > + * Gasia need another query plus an USB interrupt to get operational. > > + */ > > + ret = hid_hw_raw_request(hdev, 0xf5, buf, 64, HID_FEATURE_REPORT, > > + HID_REQ_GET_REPORT); > > 0xf5 should not be hardcoded. You have to retrieve it from the > description of the device or at least put a special case for your > specific game controller. > I find this acceptable, more so considering that the current code does exactly the same for feature report 0xf2. The operations are just ping-of-life tricks, nothing we want to expose. > > + > > + if (ret < 0) > > + hid_err(hdev, "can't set operational mode on the interrupt EP\n"); The error message is not accurate in this case. You could use something like "can't set operational mode: step 2" to differentiate between the errors and call "step 1" the previous one. > > + > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > + buf2, sizeof(buf2), > > + &transfered, USB_CTRL_SET_TIMEOUT); > > Can't you simply use a hid_hw_output_report request instead of hard > coding the device specific endpoint? > And I'd also prefer it to be guarded against your specific controller. > usb_interrupt_msg() is called in usbhid_output_report() indeed, so it should be possible to use the generic HID interface. > > + > > + if (ret < 0) > > + hid_err(hdev, "can't set operational mode on the interrupt EP\n"); And adjust this message accordingly. Call it "step 3" perhaps? > > > > kfree(buf); > > Thanks, Antonio
Hi, On Sat, 7 Feb 2015 17:31:33 +0100 Antonio Ospite <ao2@ao2.it> wrote: > > > +#include <linux/usb/input.h> > > > > Please don't. > > HID should be transport agnostic, so please refrain from directly call usb. > > > > I agree with Benjamin here. > > > > + > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > + buf2, sizeof(buf2), > > > + &transfered, USB_CTRL_SET_TIMEOUT); > > > > Can't you simply use a hid_hw_output_report request instead of hard > > coding the device specific endpoint? > > And I'd also prefer it to be guarded against your specific controller. > > > > usb_interrupt_msg() is called in usbhid_output_report() indeed, so it > should be possible to use the generic HID interface. Regarding the USB. This is a comment from Andrew's patch: // doesn't work. gets sent as a SET_REPORT Request intstead of a // URB_INTERRUPT out. I guess usbhid->urbout is null //if ( hdev->hid_output_raw_report(hdev, buf2, sizeof(buf2), I took his word for it, and did not attempt it. Do you think it would work with the current kernel? If so, I can test later. Had quite an enjoyable evening playing some FF7 once I had the controller working ;) Antonio, others, can you test with official Sony controllers that the current patch did not break them? I only have the Gasia. > > > + if (ret < 0) > > > + hid_err(hdev, "can't set operational mode on the interrupt EP\n"); > > And adjust this message accordingly. Call it "step 3" perhaps? Sure, will edit the error messages. - Lauri -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2015-02-07 at 15:48 +0200, Lauri Kasanen wrote: > Without this, my "Gasia Co.,Ltd PS(R) Gamepad" would not send > any events. Now everything works including the leds. > > Based on work by Andrew Haines and Antonio Ospite. > > cc: Antonio Ospite <ao2@ao2.it> > cc: Andrew Haines <AndrewD207@aol.com> > Signed-off-by: Lauri Kasanen <cand@gmx.com> > --- > drivers/hid/hid-sony.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > Antonio's original approach was not enough; it enabled the events, > but only for a few seconds, then the controller timed out and sent > no more. Andrew's did more than was necessary. This is a combination > of the two, against Linus' git. > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index 31e9d25..de93386 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -36,6 +36,7 @@ > #include <linux/list.h> > #include <linux/idr.h> > #include <linux/input/mt.h> > +#include <linux/usb/input.h> > > #include "hid-ids.h" > > @@ -1130,8 +1131,12 @@ static void sony_input_configured(struct hid_device *hdev, > */ > static int sixaxis_set_operational_usb(struct hid_device *hdev) > { > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > + struct usb_device *dev = interface_to_usbdev(intf); > int ret; > - char *buf = kmalloc(18, GFP_KERNEL); > + char *buf = kmalloc(65, GFP_KERNEL); > + unsigned char buf2[] = { 0x00 }; > + int transfered; > > if (!buf) > return -ENOMEM; > @@ -1140,7 +1145,24 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev) > HID_REQ_GET_REPORT); > > if (ret < 0) > - hid_err(hdev, "can't set operational mode\n"); > + hid_err(hdev, "can't set operational mode on the control EP\n"); > + > + /* > + * Some compatible controllers like the Speedlink Strike FX and > + * Gasia need another query plus an USB interrupt to get operational. > + */ > + ret = hid_hw_raw_request(hdev, 0xf5, buf, 64, HID_FEATURE_REPORT, > + HID_REQ_GET_REPORT); > + > + if (ret < 0) > + hid_err(hdev, "can't set operational mode on the interrupt EP\n"); > + > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > + buf2, sizeof(buf2), > + &transfered, USB_CTRL_SET_TIMEOUT); You cannot do this. Even for a single byte DMA on the stack is wrong. Not on all architectures it works at all and you violate the DMA constrainsts. You must use kmalloc(). Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 09 Feb 2015 11:08:01 +0100 Oliver Neukum <oneukum@suse.de> wrote: > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > + buf2, sizeof(buf2), > > + &transfered, USB_CTRL_SET_TIMEOUT); > > You cannot do this. Even for a single byte DMA on the stack is > wrong. Not on all architectures it works at all and you violate > the DMA constrainsts. You must use kmalloc(). Hi Oliver, Does this still apply when using hid_hw_output_report? - Lauri -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-02-09 at 20:44 +0200, Lauri Kasanen wrote: > On Mon, 09 Feb 2015 11:08:01 +0100 > Oliver Neukum <oneukum@suse.de> wrote: > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > + buf2, sizeof(buf2), > > > + &transfered, USB_CTRL_SET_TIMEOUT); > > > > You cannot do this. Even for a single byte DMA on the stack is > > wrong. Not on all architectures it works at all and you violate > > the DMA constrainsts. You must use kmalloc(). > > Hi Oliver, > > Does this still apply when using hid_hw_output_report? Yes. For USB devices hid_hw_output_report() goes to usbhid_output_report(). That goes to usb_interrupt_msg(), which passes the buffer pointer. It will then be mapped for DMA. You must not do that on the stack. HTH Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 2015-02-10 09:14:36, Oliver Neukum wrote: > On Mon, 2015-02-09 at 20:44 +0200, Lauri Kasanen wrote: > > On Mon, 09 Feb 2015 11:08:01 +0100 > > Oliver Neukum <oneukum@suse.de> wrote: > > > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > > + buf2, sizeof(buf2), > > > > + &transfered, USB_CTRL_SET_TIMEOUT); > > > > > > You cannot do this. Even for a single byte DMA on the stack is > > > wrong. Not on all architectures it works at all and you violate > > > the DMA constrainsts. You must use kmalloc(). > > > > Hi Oliver, > > > > Does this still apply when using hid_hw_output_report? > > Yes. For USB devices hid_hw_output_report() goes to > usbhid_output_report(). That goes to usb_interrupt_msg(), > which passes the buffer pointer. It will then be mapped > for DMA. You must not do that on the stack. Should we have some kind of runtime test for this ...? Because this is very very easy to get wrong... and I bet we do get it wrong at > 1 place... Pavel
On Mon, 16 Mar 2015, Pavel Machek wrote: > > > Oliver Neukum <oneukum@suse.de> wrote: > > > > > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > > > + buf2, sizeof(buf2), > > > > > + &transfered, USB_CTRL_SET_TIMEOUT); > > > > > > > > You cannot do this. Even for a single byte DMA on the stack is > > > > wrong. Not on all architectures it works at all and you violate > > > > the DMA constrainsts. You must use kmalloc(). > > > > > > Hi Oliver, > > > > > > Does this still apply when using hid_hw_output_report? > > > > Yes. For USB devices hid_hw_output_report() goes to > > usbhid_output_report(). That goes to usb_interrupt_msg(), > > which passes the buffer pointer. It will then be mapped > > for DMA. You must not do that on the stack. > > Should we have some kind of runtime test for this ...? Because this is > very very easy to get wrong... and I bet we do get it wrong at > 1 > place... Are you sure CONFIG_DMA_API_DEBUG wouldn't warn here?
On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote: > On Mon, 16 Mar 2015, Pavel Machek wrote: > > > > > Oliver Neukum <oneukum@suse.de> wrote: > > > > > > > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > > > > + buf2, sizeof(buf2), > > > > > > + &transfered, USB_CTRL_SET_TIMEOUT); > > > > > > > > > > You cannot do this. Even for a single byte DMA on the stack is > > > > > wrong. Not on all architectures it works at all and you violate > > > > > the DMA constrainsts. You must use kmalloc(). > > > > > > > > Hi Oliver, > > > > > > > > Does this still apply when using hid_hw_output_report? > > > > > > Yes. For USB devices hid_hw_output_report() goes to > > > usbhid_output_report(). That goes to usb_interrupt_msg(), > > > which passes the buffer pointer. It will then be mapped > > > for DMA. You must not do that on the stack. > > > > Should we have some kind of runtime test for this ...? Because this is > > very very easy to get wrong... and I bet we do get it wrong at > 1 > > place... > > Are you sure CONFIG_DMA_API_DEBUG wouldn't warn here? As far as I can tell, it will not warn. The problem is not in the mapping itself. That is usually legitimate. The problem arises because the buffer doesn't have a cacheline of its own. Thus the memory corruption happens after the IO operation has started. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 2015-03-19 10:14:21, Oliver Neukum wrote: > On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote: > > On Mon, 16 Mar 2015, Pavel Machek wrote: > > > > > > > Oliver Neukum <oneukum@suse.de> wrote: > > > > > > > > > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > > > > > + buf2, sizeof(buf2), > > > > > > > + &transfered, USB_CTRL_SET_TIMEOUT); > > > > > > > > > > > > You cannot do this. Even for a single byte DMA on the stack is > > > > > > wrong. Not on all architectures it works at all and you violate > > > > > > the DMA constrainsts. You must use kmalloc(). > > > > > > > > > > Hi Oliver, > > > > > > > > > > Does this still apply when using hid_hw_output_report? > > > > > > > > Yes. For USB devices hid_hw_output_report() goes to > > > > usbhid_output_report(). That goes to usb_interrupt_msg(), > > > > which passes the buffer pointer. It will then be mapped > > > > for DMA. You must not do that on the stack. > > > > > > Should we have some kind of runtime test for this ...? Because this is > > > very very easy to get wrong... and I bet we do get it wrong at > 1 > > > place... > > > > Are you sure CONFIG_DMA_API_DEBUG wouldn't warn here? > > As far as I can tell, it will not warn. The problem is not in the > mapping itself. That is usually legitimate. The problem arises > because the buffer doesn't have a cacheline of its own. Thus the > memory corruption happens after the IO operation has started. Nasty. Would WARN_ON(buffer & CACHELINE_SIZE-1) do at least part of the trick? Alternatively, could we call ksize() on the object, and fail if it is not big enough? Alternatively, we could create "allocate_for_usb" function, and only take pointers allocated by that function in usb functions. That would also teach people the problem exists... Pavel
On Thu, 2015-03-19 at 10:38 +0100, Pavel Machek wrote: > On Thu 2015-03-19 10:14:21, Oliver Neukum wrote: > > On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote: > > > Are you sure CONFIG_DMA_API_DEBUG wouldn't warn here? > > > > As far as I can tell, it will not warn. The problem is not in the > > mapping itself. That is usually legitimate. The problem arises > > because the buffer doesn't have a cacheline of its own. Thus the > > memory corruption happens after the IO operation has started. > > Nasty. Would WARN_ON(buffer & CACHELINE_SIZE-1) do at least part of No. It is perfectly legitimate to put your buffer at an offset or to combine buffers provided you don't use them at the same time. > the trick? Alternatively, could we call ksize() on the object, and > fail if it is not big enough? What object? We have a pointer to a memory location. > Alternatively, we could create "allocate_for_usb" function, and only > take pointers allocated by that function in usb functions. That would > also teach people the problem exists... No, this problem is not limited to USB. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 2015-03-19 10:54:22, Oliver Neukum wrote: > On Thu, 2015-03-19 at 10:38 +0100, Pavel Machek wrote: > > On Thu 2015-03-19 10:14:21, Oliver Neukum wrote: > > > On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote: > > > > > Are you sure CONFIG_DMA_API_DEBUG wouldn't warn here? > > > > > > As far as I can tell, it will not warn. The problem is not in the > > > mapping itself. That is usually legitimate. The problem arises > > > because the buffer doesn't have a cacheline of its own. Thus the > > > memory corruption happens after the IO operation has started. > > > > Nasty. Would WARN_ON(buffer & CACHELINE_SIZE-1) do at least part of > > No. It is perfectly legitimate to put your buffer at an offset > or to combine buffers provided you don't use them at the same > time. Legitimate: yes. Is anyone doing it? And will not they see exactly the same data corruption with the aliasing data? > > Alternatively, we could create "allocate_for_usb" function, and only > > take pointers allocated by that function in usb functions. That would > > also teach people the problem exists... > > No, this problem is not limited to USB. Well.. Recognize that just because you have a pointer does not mean you can pass it to certain functions. Maybe those functions should not be taking pointers in the first place.... Pavel
Hi On Sat, Feb 7, 2015 at 10:48 PM, Lauri Kasanen <cand@gmx.com> wrote: > Hi, > > On Sat, 7 Feb 2015 17:31:33 +0100 > Antonio Ospite <ao2@ao2.it> wrote: >> > > +#include <linux/usb/input.h> >> > >> > Please don't. >> > HID should be transport agnostic, so please refrain from directly call usb. >> > >> >> I agree with Benjamin here. >> >> > > + >> > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), >> > > + buf2, sizeof(buf2), >> > > + &transfered, USB_CTRL_SET_TIMEOUT); >> > >> > Can't you simply use a hid_hw_output_report request instead of hard >> > coding the device specific endpoint? >> > And I'd also prefer it to be guarded against your specific controller. >> > >> >> usb_interrupt_msg() is called in usbhid_output_report() indeed, so it >> should be possible to use the generic HID interface. > > Regarding the USB. This is a comment from Andrew's patch: > > // doesn't work. gets sent as a SET_REPORT Request intstead of a > // URB_INTERRUPT out. I guess usbhid->urbout is null > //if ( hdev->hid_output_raw_report(hdev, buf2, sizeof(buf2), > > I took his word for it, and did not attempt it. Do you think it would > work with the current kernel? If so, I can test later. Had quite an > enjoyable evening playing some FF7 once I had the controller working ;) Yes, hid_hw_request() and hid_hw_raw_request() send SET_REPORT/GET_REPORT in the ctrl line. If you want output-reports on the intr line, you have to use hid_hw_output_report(). So according to your quote, hid_hw_output_report() should work. Background: Documentation/hid/hid-transport.txt Thanks David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-03-19 at 11:12 +0100, Pavel Machek wrote: > On Thu 2015-03-19 10:54:22, Oliver Neukum wrote: > > On Thu, 2015-03-19 at 10:38 +0100, Pavel Machek wrote: > > > On Thu 2015-03-19 10:14:21, Oliver Neukum wrote: > > > > On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote: > > > > > > > Are you sure CONFIG_DMA_API_DEBUG wouldn't warn here? > > > > > > > > As far as I can tell, it will not warn. The problem is not in the > > > > mapping itself. That is usually legitimate. The problem arises > > > > because the buffer doesn't have a cacheline of its own. Thus the > > > > memory corruption happens after the IO operation has started. > > > > > > Nasty. Would WARN_ON(buffer & CACHELINE_SIZE-1) do at least part of > > > > No. It is perfectly legitimate to put your buffer at an offset > > or to combine buffers provided you don't use them at the same > > time. > > Legitimate: yes. Is anyone doing it? And will not they see exactly the For this particular function probably not. In the general case, yes. That's how you continue after an error. > same data corruption with the aliasing data? No, the error happens by touching another part of the cacheline from the CPU thus loading stale content into the cache. You can even have simultaneous DMA to two buffers in the same cacheline provided you touch neither until the last DMA has finished. > > > Alternatively, we could create "allocate_for_usb" function, and only > > > take pointers allocated by that function in usb functions. That would > > > also teach people the problem exists... > > > > No, this problem is not limited to USB. > > Well.. Recognize that just because you have a pointer does not mean > you can pass it to certain functions. > > Maybe those functions should not be taking pointers in the first > place.... What else would it take? Should we force people to allocate a new buffer every time? That would make the API for reading and writing asymmetric. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index 31e9d25..de93386 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c @@ -36,6 +36,7 @@ #include <linux/list.h> #include <linux/idr.h> #include <linux/input/mt.h> +#include <linux/usb/input.h> #include "hid-ids.h" @@ -1130,8 +1131,12 @@ static void sony_input_configured(struct hid_device *hdev, */ static int sixaxis_set_operational_usb(struct hid_device *hdev) { + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); + struct usb_device *dev = interface_to_usbdev(intf); int ret; - char *buf = kmalloc(18, GFP_KERNEL); + char *buf = kmalloc(65, GFP_KERNEL); + unsigned char buf2[] = { 0x00 }; + int transfered; if (!buf) return -ENOMEM; @@ -1140,7 +1145,24 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev) HID_REQ_GET_REPORT); if (ret < 0) - hid_err(hdev, "can't set operational mode\n"); + hid_err(hdev, "can't set operational mode on the control EP\n"); + + /* + * Some compatible controllers like the Speedlink Strike FX and + * Gasia need another query plus an USB interrupt to get operational. + */ + ret = hid_hw_raw_request(hdev, 0xf5, buf, 64, HID_FEATURE_REPORT, + HID_REQ_GET_REPORT); + + if (ret < 0) + hid_err(hdev, "can't set operational mode on the interrupt EP\n"); + + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), + buf2, sizeof(buf2), + &transfered, USB_CTRL_SET_TIMEOUT); + + if (ret < 0) + hid_err(hdev, "can't set operational mode on the interrupt EP\n"); kfree(buf);
Without this, my "Gasia Co.,Ltd PS(R) Gamepad" would not send any events. Now everything works including the leds. Based on work by Andrew Haines and Antonio Ospite. cc: Antonio Ospite <ao2@ao2.it> cc: Andrew Haines <AndrewD207@aol.com> Signed-off-by: Lauri Kasanen <cand@gmx.com> --- drivers/hid/hid-sony.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) Antonio's original approach was not enough; it enabled the events, but only for a few seconds, then the controller timed out and sent no more. Andrew's did more than was necessary. This is a combination of the two, against Linus' git.