Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
Hi On Sat, Feb 7, 2015 at 10:48 PM, Lauri Kasanen wrote: > Hi, > > On Sat, 7 Feb 2015 17:31:33 +0100 > Antonio Ospite wrote: >> > > +#include >> > >> > 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), >> > > + , 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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 wrote: > > > > > > > > > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > > > > > + buf2, sizeof(buf2), > > > > > > > + , 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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote: > On Mon, 16 Mar 2015, Pavel Machek wrote: > > > > > Oliver Neukum wrote: > > > > > > > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > > > > + buf2, sizeof(buf2), > > > > > > + , 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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 oneu...@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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Mon, 2015-03-16 at 22:37 +0100, Jiri Kosina wrote: On Mon, 16 Mar 2015, Pavel Machek wrote: Oliver Neukum oneu...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
Hi On Sat, Feb 7, 2015 at 10:48 PM, Lauri Kasanen c...@gmx.com wrote: Hi, On Sat, 7 Feb 2015 17:31:33 +0100 Antonio Ospite a...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Mon, 16 Mar 2015, Pavel Machek wrote: > > > Oliver Neukum wrote: > > > > > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > > > + buf2, sizeof(buf2), > > > > > + , 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? -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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 wrote: > > > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > > + buf2, sizeof(buf2), > > > > + , 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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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 oneu...@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 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Mon, 16 Mar 2015, Pavel Machek wrote: Oliver Neukum oneu...@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? -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers, v2
Hi Antonio, > These annotations about the history of a patch are usually added > after the '---' marker right before sending the patch, not in the commit > message. > > They are useful for reviewers, but not really interesting anymore after > the code is validated and merged. > > In the subject you can mark a v2 like [PATCHv2], the prefix will be > stripped by git am, do not put the version of the patch in the short > commit message itself. Thanks, communities differ wrt these, I'm not often around the kernel. > Moreover, a following cleanup patch could make this __u8 *buf which > would be the correct type. > > Another follow-up patch could indeed use SIXAXIS_REPORT_0xF2_SIZE and > also define SIXAXIS_REPORT_0xF5_SIZE. > > I can do these latter if you want. Yes, please do. You have more experience around these parts and can get it done faster. > Maybe you can add a "goto out" here and skip the other steps if a > previous one fails. Or is some slack actually required to support > compatible controllers? They all succeed on mine, so will add the goto. - Lauri -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers, v2
Hi Lauri,
On Sun, 8 Feb 2015 11:11:38 +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.
>
> v2:
> - edited error messages
> - use output_report
These annotations about the history of a patch are usually added
after the '---' marker right before sending the patch, not in the commit
message.
They are useful for reviewers, but not really interesting anymore after
the code is validated and merged.
In the subject you can mark a v2 like [PATCHv2], the prefix will be
stripped by git am, do not put the version of the patch in the short
commit message itself.
Some nitpicking below.
> cc: Antonio Ospite
> cc: Andrew Haines
> Signed-off-by: Lauri Kasanen
> ---
> drivers/hid/hid-sony.c | 20 ++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> Despite Andrew's report, using output_report worked fine.
Good! :)
I also tested the patch on an original controller and it still works
fine.
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 31e9d25..2661227 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1131,7 +1131,8 @@ static void sony_input_configured(struct hid_device
> *hdev,
> static int sixaxis_set_operational_usb(struct hid_device *hdev)
> {
> int ret;
> - char *buf = kmalloc(18, GFP_KERNEL);
> + char *buf = kmalloc(65, GFP_KERNEL);
> + unsigned char buf2[] = { 0x00 };
>
The argument of hid_hw_output_report() is of the same type of
hid_hw_raw_request() so make it all the same here.
But even better, can't you just reuse buf? It's a dummy buffer anyways.
Moreover, a following cleanup patch could make this __u8 *buf which
would be the correct type.
Another follow-up patch could indeed use SIXAXIS_REPORT_0xF2_SIZE and
also define SIXAXIS_REPORT_0xF5_SIZE.
I can do these latter if you want.
> if (!buf)
> return -ENOMEM;
> @@ -1140,7 +1141,22 @@ 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: step 1\n");
> +
Maybe you can add a "goto out" here and skip the other steps if a
previous one fails. Or is some slack actually required to support
compatible controllers?
> + /*
> + * 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: step 2\n");
> +
goto out;
}
> + ret = hid_hw_output_report(hdev, buf2, sizeof(buf2));
This could be:
ret = hid_hw_output_report(hdev, buf, 1);
> +
> + if (ret < 0)
> + hid_err(hdev, "can't set operational mode: step 3\n");
>
out:
> kfree(buf);
>
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Mon, 2015-02-09 at 20:44 +0200, Lauri Kasanen wrote: > On Mon, 09 Feb 2015 11:08:01 +0100 > Oliver Neukum wrote: > > > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > > + buf2, sizeof(buf2), > > > + , 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Mon, 2015-02-09 at 20:44 +0200, Lauri Kasanen wrote: On Mon, 09 Feb 2015 11:08:01 +0100 Oliver Neukum oneu...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers, v2
Hi Lauri,
On Sun, 8 Feb 2015 11:11:38 +0200
Lauri Kasanen c...@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.
v2:
- edited error messages
- use output_report
These annotations about the history of a patch are usually added
after the '---' marker right before sending the patch, not in the commit
message.
They are useful for reviewers, but not really interesting anymore after
the code is validated and merged.
In the subject you can mark a v2 like [PATCHv2], the prefix will be
stripped by git am, do not put the version of the patch in the short
commit message itself.
Some nitpicking below.
cc: Antonio Ospite a...@ao2.it
cc: Andrew Haines andrewd...@aol.com
Signed-off-by: Lauri Kasanen c...@gmx.com
---
drivers/hid/hid-sony.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
Despite Andrew's report, using output_report worked fine.
Good! :)
I also tested the patch on an original controller and it still works
fine.
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 31e9d25..2661227 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1131,7 +1131,8 @@ static void sony_input_configured(struct hid_device
*hdev,
static int sixaxis_set_operational_usb(struct hid_device *hdev)
{
int ret;
- char *buf = kmalloc(18, GFP_KERNEL);
+ char *buf = kmalloc(65, GFP_KERNEL);
+ unsigned char buf2[] = { 0x00 };
The argument of hid_hw_output_report() is of the same type of
hid_hw_raw_request() so make it all the same here.
But even better, can't you just reuse buf? It's a dummy buffer anyways.
Moreover, a following cleanup patch could make this __u8 *buf which
would be the correct type.
Another follow-up patch could indeed use SIXAXIS_REPORT_0xF2_SIZE and
also define SIXAXIS_REPORT_0xF5_SIZE.
I can do these latter if you want.
if (!buf)
return -ENOMEM;
@@ -1140,7 +1141,22 @@ 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: step 1\n);
+
Maybe you can add a goto out here and skip the other steps if a
previous one fails. Or is some slack actually required to support
compatible controllers?
+ /*
+ * 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: step 2\n);
+
goto out;
}
+ ret = hid_hw_output_report(hdev, buf2, sizeof(buf2));
This could be:
ret = hid_hw_output_report(hdev, buf, 1);
+
+ if (ret 0)
+ hid_err(hdev, can't set operational mode: step 3\n);
out:
kfree(buf);
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers, v2
Hi Antonio, These annotations about the history of a patch are usually added after the '---' marker right before sending the patch, not in the commit message. They are useful for reviewers, but not really interesting anymore after the code is validated and merged. In the subject you can mark a v2 like [PATCHv2], the prefix will be stripped by git am, do not put the version of the patch in the short commit message itself. Thanks, communities differ wrt these, I'm not often around the kernel. Moreover, a following cleanup patch could make this __u8 *buf which would be the correct type. Another follow-up patch could indeed use SIXAXIS_REPORT_0xF2_SIZE and also define SIXAXIS_REPORT_0xF5_SIZE. I can do these latter if you want. Yes, please do. You have more experience around these parts and can get it done faster. Maybe you can add a goto out here and skip the other steps if a previous one fails. Or is some slack actually required to support compatible controllers? They all succeed on mine, so will add the goto. - Lauri -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Mon, 09 Feb 2015 11:08:01 +0100 Oliver Neukum wrote: > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02), > > + buf2, sizeof(buf2), > > + , 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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
> cc: Andrew Haines
> Signed-off-by: Lauri Kasanen
> ---
> 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
> #include
> #include
> +#include
>
> #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),
> + , 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Mon, 09 Feb 2015 11:08:01 +0100 Oliver Neukum oneu...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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 a...@ao2.it
cc: Andrew Haines andrewd...@aol.com
Signed-off-by: Lauri Kasanen c...@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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] HID: sony: Enable Gasia third-party PS3 controllers, v2
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.
v2:
- edited error messages
- use output_report
cc: Antonio Ospite
cc: Andrew Haines
Signed-off-by: Lauri Kasanen
---
drivers/hid/hid-sony.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
Despite Andrew's report, using output_report worked fine.
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 31e9d25..2661227 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1131,7 +1131,8 @@ static void sony_input_configured(struct hid_device *hdev,
static int sixaxis_set_operational_usb(struct hid_device *hdev)
{
int ret;
- char *buf = kmalloc(18, GFP_KERNEL);
+ char *buf = kmalloc(65, GFP_KERNEL);
+ unsigned char buf2[] = { 0x00 };
if (!buf)
return -ENOMEM;
@@ -1140,7 +1141,22 @@ 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: step 1\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: step 2\n");
+
+ ret = hid_hw_output_report(hdev, buf2, sizeof(buf2));
+
+ if (ret < 0)
+ hid_err(hdev, "can't set operational mode: step 3\n");
kfree(buf);
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] HID: sony: Enable Gasia third-party PS3 controllers, v2
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.
v2:
- edited error messages
- use output_report
cc: Antonio Ospite a...@ao2.it
cc: Andrew Haines andrewd...@aol.com
Signed-off-by: Lauri Kasanen c...@gmx.com
---
drivers/hid/hid-sony.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
Despite Andrew's report, using output_report worked fine.
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 31e9d25..2661227 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1131,7 +1131,8 @@ static void sony_input_configured(struct hid_device *hdev,
static int sixaxis_set_operational_usb(struct hid_device *hdev)
{
int ret;
- char *buf = kmalloc(18, GFP_KERNEL);
+ char *buf = kmalloc(65, GFP_KERNEL);
+ unsigned char buf2[] = { 0x00 };
if (!buf)
return -ENOMEM;
@@ -1140,7 +1141,22 @@ 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: step 1\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: step 2\n);
+
+ ret = hid_hw_output_report(hdev, buf2, sizeof(buf2));
+
+ if (ret 0)
+ hid_err(hdev, can't set operational mode: step 3\n);
kfree(buf);
--
1.8.3.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
Hi, On Sat, 7 Feb 2015 17:31:33 +0100 Antonio Ospite wrote: > > > +#include > > > > 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), > > > + , 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Sat, 7 Feb 2015 10:56:49 -0500
Benjamin Tissoires wrote:
> Hi Lauri,
>
> On Sat, Feb 7, 2015 at 8:48 AM, 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
> > cc: Andrew Haines
> > Signed-off-by: Lauri Kasanen
> > ---
> > 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
> > #include
> > #include
> > +#include
>
> 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),
> > + , 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
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
Hi Lauri,
On Sat, Feb 7, 2015 at 8:48 AM, 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
> cc: Andrew Haines
> Signed-off-by: Lauri Kasanen
> ---
> 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
> #include
> #include
> +#include
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),
> + , 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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
cc: Andrew Haines
Signed-off-by: Lauri Kasanen
---
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
#include
#include
+#include
#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),
+ , USB_CTRL_SET_TIMEOUT);
+
+ 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
Hi Lauri,
On Sat, Feb 7, 2015 at 8:48 AM, Lauri Kasanen c...@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 a...@ao2.it
cc: Andrew Haines andrewd...@aol.com
Signed-off-by: Lauri Kasanen c...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] HID: sony: Enable Gasia third-party PS3 controllers
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 a...@ao2.it
cc: Andrew Haines andrewd...@aol.com
Signed-off-by: Lauri Kasanen c...@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);
+
+ 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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
On Sat, 7 Feb 2015 10:56:49 -0500
Benjamin Tissoires benjamin.tissoi...@gmail.com wrote:
Hi Lauri,
On Sat, Feb 7, 2015 at 8:48 AM, Lauri Kasanen c...@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 a...@ao2.it
cc: Andrew Haines andrewd...@aol.com
Signed-off-by: Lauri Kasanen c...@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
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers
Hi, On Sat, 7 Feb 2015 17:31:33 +0100 Antonio Ospite a...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
