[PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Lauri Kasanen]

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);
+
+	if (ret < 0)
+		hid_err(hdev, "can't set operational mode on the interrupt EP\n");
 
 	kfree(buf);
 
-- 
1.8.3.1

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Benjamin Tissoires]

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Antonio Ospite]

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

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Lauri Kasanen]

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[David Herrmann]

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Lauri Kasanen]

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Jiri Kosina]

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?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Pavel Machek]

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

Re: [PATCH] HID: sony: Enable Gasia third-party PS3 controllers

[Oliver Neukum]

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