[PATCH] HID: Sony: Add support for Gasia controllers

[PATCH] HID: Sony: Add support for Gasia controllers

[Martyn Welch]

There seems to be a number of subtly different firmwares for the
Playstation controllers made by "Gasia Co.,Ltd". Whilst such controllers
are easily detectable when attached via USB that is not always the case
via Bluetooth. Some controllers are named "PLAYSTATION(R)3 Controller"
where as the official Sony controllers are named
"Sony PLAYSTATION(R)3 Controller", however some versions of firmware use
the exact name used by the official controllers. The only way I've been
able to distinguish these versions of the controller (when connected via
Bluetooth) is that the Bluetooth Class of Device incorrectly reports the
controller as a keyboard rather than a gamepad. I've so far failed to work
out how to access this information from a HID driver.

The Gasia controllers need output reports to be configured in the same way
as the Shanwan controllers. In order to ensure both types of Gasia firmware
will work, this patch adds a quirk for those devices it can detect and
reworks `sixaxis_send_output_report()` to attempt `hid_hw_output_report()`
should `hid_hw_raw_request()` be known to be the wrong option (as is the
case with the Shanwan controllers) or fails.

This has got all the controllers I have working, with the slight
anoyance that the Gasia controllers that don't currently get marked with
a quirk require the call to `hid_hw_raw_request()` to fail before the
controller finishes initialising (which adds a significant extra delay
before the controller is ready).

This patch is based on the following patch by Conn O'Griofa:

https://github.com/RetroPie/RetroPie-Setup/pull/2263/commits/017f00f6e15f04b3272ff1abae8742dc4c47b608

Cc: Conn O'Griofa <connogriofa@gmail.com>
Signed-off-by: Martyn Welch <martyn@welchs.me.uk>
---
 drivers/hid/hid-sony.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 4c6ed6ef31f1..d1088a85cb59 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -56,6 +56,7 @@
 #define NSG_MR5U_REMOTE_BT        BIT(14)
 #define NSG_MR7U_REMOTE_BT        BIT(15)
 #define SHANWAN_GAMEPAD           BIT(16)
+#define GASIA_GAMEPAD             BIT(17)
 
 #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)
 #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB | MOTION_CONTROLLER_BT)
@@ -2067,6 +2068,7 @@ static void sixaxis_send_output_report(struct sony_sc *sc)
 	struct sixaxis_output_report *report =
 		(struct sixaxis_output_report *)sc->output_report_dmabuf;
 	int n;
+	int ret = -1;
 
 	/* Initialize the report with default values */
 	memcpy(report, &default_report, sizeof(struct sixaxis_output_report));
@@ -2101,14 +2103,23 @@ static void sixaxis_send_output_report(struct sony_sc *sc)
 		}
 	}
 
-	/* SHANWAN controllers require output reports via intr channel */
-	if (sc->quirks & SHANWAN_GAMEPAD)
-		hid_hw_output_report(sc->hdev, (u8 *)report,
-				sizeof(struct sixaxis_output_report));
-	else
-		hid_hw_raw_request(sc->hdev, report->report_id, (u8 *)report,
+	/*
+	 * SHANWAN & GASIA controllers require output reports via intr channel.
+	 * Some of the Gasia controllers are basically indistinguishable from
+	 * the official ones and thus try hid_hw_output_report() should
+	 * hid_hw_raw_request() fail.
+	 */
+	if (!(sc->quirks & (SHANWAN_GAMEPAD | GASIA_GAMEPAD)))
+		ret = hid_hw_raw_request(sc->hdev, report->report_id,
+				(u8 *)report,
 				sizeof(struct sixaxis_output_report),
 				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
+
+	if (ret >= 0)
+		return;
+
+	hid_hw_output_report(sc->hdev, (u8 *)report,
+			sizeof(struct sixaxis_output_report));
 }
 
 static void dualshock4_send_output_report(struct sony_sc *sc)
@@ -2833,6 +2844,14 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (!strcmp(hdev->name, "SHANWAN PS3 GamePad"))
 		quirks |= SHANWAN_GAMEPAD;
 
+	/*
+	 * Some Gasia controllers are named "PLAYSTATION(R)3 Controller"
+	 * where as the official Sony controllers are named
+	 * "Sony PLAYSTATION(R)3 Controller".
+	 */
+	if (!strcmp(hdev->name, "PLAYSTATION(R)3 Controller"))
+		quirks |= GASIA_GAMEPAD;
+
 	sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
 	if (sc == NULL) {
 		hid_err(hdev, "can't alloc sony descriptor\n");
-- 
2.20.1

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Roderick Colenbrander]

Hi Martyn,

Thanks for sharing your patch. Though I must say with my Sony hat on,
I don't really like supporting clone devices (they hijack our device
ids.. etcetera) and we support hid-sony in an official capacity now
across various devices. Though thischange  all relates to PS3
generation, which is not that important anymore so it shouldn't matter
that much.

Thanks,
Roderick

On Sun, Jan 26, 2020 at 12:28 PM Martyn Welch <martyn@welchs.me.uk> wrote:
>
> There seems to be a number of subtly different firmwares for the
> Playstation controllers made by "Gasia Co.,Ltd". Whilst such controllers
> are easily detectable when attached via USB that is not always the case
> via Bluetooth. Some controllers are named "PLAYSTATION(R)3 Controller"
> where as the official Sony controllers are named
> "Sony PLAYSTATION(R)3 Controller", however some versions of firmware use
> the exact name used by the official controllers. The only way I've been
> able to distinguish these versions of the controller (when connected via
> Bluetooth) is that the Bluetooth Class of Device incorrectly reports the
> controller as a keyboard rather than a gamepad. I've so far failed to work
> out how to access this information from a HID driver.
>
> The Gasia controllers need output reports to be configured in the same way
> as the Shanwan controllers. In order to ensure both types of Gasia firmware
> will work, this patch adds a quirk for those devices it can detect and
> reworks `sixaxis_send_output_report()` to attempt `hid_hw_output_report()`
> should `hid_hw_raw_request()` be known to be the wrong option (as is the
> case with the Shanwan controllers) or fails.
>
> This has got all the controllers I have working, with the slight
> anoyance that the Gasia controllers that don't currently get marked with
> a quirk require the call to `hid_hw_raw_request()` to fail before the
> controller finishes initialising (which adds a significant extra delay
> before the controller is ready).
>
> This patch is based on the following patch by Conn O'Griofa:
>
> https://github.com/RetroPie/RetroPie-Setup/pull/2263/commits/017f00f6e15f04b3272ff1abae8742dc4c47b608
>
> Cc: Conn O'Griofa <connogriofa@gmail.com>
> Signed-off-by: Martyn Welch <martyn@welchs.me.uk>
> ---
>  drivers/hid/hid-sony.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 4c6ed6ef31f1..d1088a85cb59 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -56,6 +56,7 @@
>  #define NSG_MR5U_REMOTE_BT        BIT(14)
>  #define NSG_MR7U_REMOTE_BT        BIT(15)
>  #define SHANWAN_GAMEPAD           BIT(16)
> +#define GASIA_GAMEPAD             BIT(17)
>
>  #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)
>  #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB | MOTION_CONTROLLER_BT)
> @@ -2067,6 +2068,7 @@ static void sixaxis_send_output_report(struct sony_sc *sc)
>         struct sixaxis_output_report *report =
>                 (struct sixaxis_output_report *)sc->output_report_dmabuf;
>         int n;
> +       int ret = -1;
>
>         /* Initialize the report with default values */
>         memcpy(report, &default_report, sizeof(struct sixaxis_output_report));
> @@ -2101,14 +2103,23 @@ static void sixaxis_send_output_report(struct sony_sc *sc)
>                 }
>         }
>
> -       /* SHANWAN controllers require output reports via intr channel */
> -       if (sc->quirks & SHANWAN_GAMEPAD)
> -               hid_hw_output_report(sc->hdev, (u8 *)report,
> -                               sizeof(struct sixaxis_output_report));
> -       else
> -               hid_hw_raw_request(sc->hdev, report->report_id, (u8 *)report,
> +       /*
> +        * SHANWAN & GASIA controllers require output reports via intr channel.
> +        * Some of the Gasia controllers are basically indistinguishable from
> +        * the official ones and thus try hid_hw_output_report() should
> +        * hid_hw_raw_request() fail.
> +        */
> +       if (!(sc->quirks & (SHANWAN_GAMEPAD | GASIA_GAMEPAD)))
> +               ret = hid_hw_raw_request(sc->hdev, report->report_id,
> +                               (u8 *)report,
>                                 sizeof(struct sixaxis_output_report),
>                                 HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> +
> +       if (ret >= 0)
> +               return;

I don't like this pattern so much. We only need this "ret" check for
SHANWAN and GASIA. I would just do:

if (!(sc->quirks & (SHANWAN_GAMEPAD | GASIA_GAMEPAD))) {
    int ret = hid_hw_raw_request(....);
    if (ret >= 0)
       return;
}

hid_hw_output_report(....)

> +
> +       hid_hw_output_report(sc->hdev, (u8 *)report,
> +                       sizeof(struct sixaxis_output_report));
>  }
>
>  static void dualshock4_send_output_report(struct sony_sc *sc)
> @@ -2833,6 +2844,14 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         if (!strcmp(hdev->name, "SHANWAN PS3 GamePad"))
>                 quirks |= SHANWAN_GAMEPAD;
>
> +       /*
> +        * Some Gasia controllers are named "PLAYSTATION(R)3 Controller"
> +        * where as the official Sony controllers are named
> +        * "Sony PLAYSTATION(R)3 Controller".
> +        */
> +       if (!strcmp(hdev->name, "PLAYSTATION(R)3 Controller"))
> +               quirks |= GASIA_GAMEPAD;
> +
>         sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
>         if (sc == NULL) {
>                 hid_err(hdev, "can't alloc sony descriptor\n");
> --
> 2.20.1
>

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Jiri Kosina]

On Mon, 27 Jan 2020, Roderick Colenbrander wrote:

> Thanks for sharing your patch. Though I must say with my Sony hat on, I 
> don't really like supporting clone devices (they hijack our device ids.. 
> etcetera) and we support hid-sony in an official capacity now across 
> various devices. 

Drifting away from this particular patch a little bit -- given this 
official support from Sony, would you be fine with putting this into 
writing, and adding yourself to MAINTAINERS file?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Roderick Colenbrander]

On Tue, Jan 28, 2020 at 2:11 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Mon, 27 Jan 2020, Roderick Colenbrander wrote:
>
> > Thanks for sharing your patch. Though I must say with my Sony hat on, I
> > don't really like supporting clone devices (they hijack our device ids..
> > etcetera) and we support hid-sony in an official capacity now across
> > various devices.
>
> Drifting away from this particular patch a little bit -- given this
> official support from Sony, would you be fine with putting this into
> writing, and adding yourself to MAINTAINERS file?

Of course that's no problem. Over time I need to figure out how will
maintain aspects of the driver though. I belong to the game console
division, some other devices currently supported by the driver (e.g.
TV remotes, Vaio laptops) are other divisions, so that's a challenge
as we don't know anything about those devices and don't have
datasheets or anything. Maybe we will slice the driver in some way or
something.

Thanks,
Roderick


> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Martyn Welch]

On Mon, 2020-01-27 at 14:07 -0800, Roderick Colenbrander wrote:
> Hi Martyn,
> 
> Thanks for sharing your patch. Though I must say with my Sony hat on,
> I don't really like supporting clone devices (they hijack our device
> ids.. etcetera) and we support hid-sony in an official capacity now
> across various devices. Though thischange  all relates to PS3
> generation, which is not that important anymore so it shouldn't
> matter
> that much.
> 

Hi Roderick,

I can understand that sentiment to a degree, however the hardware
exists and there are no doubt others like me that don't even own a PS3
and would just like to be able to use the gamepads they've purchased to
play games.

Martyn 

> Thanks,
> Roderick
> 
> On Sun, Jan 26, 2020 at 12:28 PM Martyn Welch <martyn@welchs.me.uk>
> wrote:
> > There seems to be a number of subtly different firmwares for the
> > Playstation controllers made by "Gasia Co.,Ltd". Whilst such
> > controllers
> > are easily detectable when attached via USB that is not always the
> > case
> > via Bluetooth. Some controllers are named "PLAYSTATION(R)3
> > Controller"
> > where as the official Sony controllers are named
> > "Sony PLAYSTATION(R)3 Controller", however some versions of
> > firmware use
> > the exact name used by the official controllers. The only way I've
> > been
> > able to distinguish these versions of the controller (when
> > connected via
> > Bluetooth) is that the Bluetooth Class of Device incorrectly
> > reports the
> > controller as a keyboard rather than a gamepad. I've so far failed
> > to work
> > out how to access this information from a HID driver.
> > 
> > The Gasia controllers need output reports to be configured in the
> > same way
> > as the Shanwan controllers. In order to ensure both types of Gasia
> > firmware
> > will work, this patch adds a quirk for those devices it can detect
> > and
> > reworks `sixaxis_send_output_report()` to attempt
> > `hid_hw_output_report()`
> > should `hid_hw_raw_request()` be known to be the wrong option (as
> > is the
> > case with the Shanwan controllers) or fails.
> > 
> > This has got all the controllers I have working, with the slight
> > anoyance that the Gasia controllers that don't currently get marked
> > with
> > a quirk require the call to `hid_hw_raw_request()` to fail before
> > the
> > controller finishes initialising (which adds a significant extra
> > delay
> > before the controller is ready).
> > 
> > This patch is based on the following patch by Conn O'Griofa:
> > 
> > https://github.com/RetroPie/RetroPie-Setup/pull/2263/commits/017f00f6e15f04b3272ff1abae8742dc4c47b608
> > 
> > Cc: Conn O'Griofa <connogriofa@gmail.com>
> > Signed-off-by: Martyn Welch <martyn@welchs.me.uk>
> > ---
> >  drivers/hid/hid-sony.c | 31 +++++++++++++++++++++++++------
> >  1 file changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> > index 4c6ed6ef31f1..d1088a85cb59 100644
> > --- a/drivers/hid/hid-sony.c
> > +++ b/drivers/hid/hid-sony.c
> > @@ -56,6 +56,7 @@
> >  #define NSG_MR5U_REMOTE_BT        BIT(14)
> >  #define NSG_MR7U_REMOTE_BT        BIT(15)
> >  #define SHANWAN_GAMEPAD           BIT(16)
> > +#define GASIA_GAMEPAD             BIT(17)
> > 
> >  #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB |
> > SIXAXIS_CONTROLLER_BT)
> >  #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB |
> > MOTION_CONTROLLER_BT)
> > @@ -2067,6 +2068,7 @@ static void sixaxis_send_output_report(struct
> > sony_sc *sc)
> >         struct sixaxis_output_report *report =
> >                 (struct sixaxis_output_report *)sc-
> > >output_report_dmabuf;
> >         int n;
> > +       int ret = -1;
> > 
> >         /* Initialize the report with default values */
> >         memcpy(report, &default_report, sizeof(struct
> > sixaxis_output_report));
> > @@ -2101,14 +2103,23 @@ static void
> > sixaxis_send_output_report(struct sony_sc *sc)
> >                 }
> >         }
> > 
> > -       /* SHANWAN controllers require output reports via intr
> > channel */
> > -       if (sc->quirks & SHANWAN_GAMEPAD)
> > -               hid_hw_output_report(sc->hdev, (u8 *)report,
> > -                               sizeof(struct
> > sixaxis_output_report));
> > -       else
> > -               hid_hw_raw_request(sc->hdev, report->report_id, (u8
> > *)report,
> > +       /*
> > +        * SHANWAN & GASIA controllers require output reports via
> > intr channel.
> > +        * Some of the Gasia controllers are basically
> > indistinguishable from
> > +        * the official ones and thus try hid_hw_output_report()
> > should
> > +        * hid_hw_raw_request() fail.
> > +        */
> > +       if (!(sc->quirks & (SHANWAN_GAMEPAD | GASIA_GAMEPAD)))
> > +               ret = hid_hw_raw_request(sc->hdev, report-
> > >report_id,
> > +                               (u8 *)report,
> >                                 sizeof(struct
> > sixaxis_output_report),
> >                                 HID_OUTPUT_REPORT,
> > HID_REQ_SET_REPORT);
> > +
> > +       if (ret >= 0)
> > +               return;
> 
> I don't like this pattern so much. We only need this "ret" check for
> SHANWAN and GASIA. I would just do:
> 
> if (!(sc->quirks & (SHANWAN_GAMEPAD | GASIA_GAMEPAD))) {
>     int ret = hid_hw_raw_request(....);
>     if (ret >= 0)
>        return;
> }
> 
> hid_hw_output_report(....)
> 
> > +
> > +       hid_hw_output_report(sc->hdev, (u8 *)report,
> > +                       sizeof(struct sixaxis_output_report));
> >  }
> > 
> >  static void dualshock4_send_output_report(struct sony_sc *sc)
> > @@ -2833,6 +2844,14 @@ static int sony_probe(struct hid_device
> > *hdev, const struct hid_device_id *id)
> >         if (!strcmp(hdev->name, "SHANWAN PS3 GamePad"))
> >                 quirks |= SHANWAN_GAMEPAD;
> > 
> > +       /*
> > +        * Some Gasia controllers are named "PLAYSTATION(R)3
> > Controller"
> > +        * where as the official Sony controllers are named
> > +        * "Sony PLAYSTATION(R)3 Controller".
> > +        */
> > +       if (!strcmp(hdev->name, "PLAYSTATION(R)3 Controller"))
> > +               quirks |= GASIA_GAMEPAD;
> > +
> >         sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
> >         if (sc == NULL) {
> >                 hid_err(hdev, "can't alloc sony descriptor\n");
> > --
> > 2.20.1
> > 

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Roderick Colenbrander]

On Tue, Jan 28, 2020 at 10:47 AM Martyn Welch <martyn@welchs.me.uk> wrote:
>
> On Mon, 2020-01-27 at 14:07 -0800, Roderick Colenbrander wrote:
> > Hi Martyn,
> >
> > Thanks for sharing your patch. Though I must say with my Sony hat on,
> > I don't really like supporting clone devices (they hijack our device
> > ids.. etcetera) and we support hid-sony in an official capacity now
> > across various devices. Though thischange  all relates to PS3
> > generation, which is not that important anymore so it shouldn't
> > matter
> > that much.
> >
>
> Hi Roderick,
>
> I can understand that sentiment to a degree, however the hardware
> exists and there are no doubt others like me that don't even own a PS3
> and would just like to be able to use the gamepads they've purchased to
> play games.
>
> Martyn

Let me explain the situation a little bit better from our angle. These
devices exist and from the Linux community perspective of course they
should see some level of support. And as I said since this is PS3
generation I don't have much of a concern.

Where it becomes tricky for any company in our situation is the
support side. We don't know these devices and don't have access to
datasheets or anything, but when such code is in our "official driver"
it means we have to involve them in our QA process and support them in
some manner (kind of legitimizing their existence as well). We now
support this driver in a large capacity (pretty much all mobile
devices will start shipping it) it puts challenges on our partners
(not a big issue since just PS3 right now).

As you can see this creates an awkward situation. I'm sure there other
such devices as well e.g. counterfeit Logitech keyboards, USB serial
adapters and other periperhals with similar challenges. In an ideal
world the support would live in another driver, but since in case of
this "fake" PS3 controller it "share" our product / vendor ids it is
tricky. At this point there is not a strong enough case yet to augment
the "hid-quirks" to do so, but perhaps if it became a serious issue
(e.g. for newer controllers) maybe we need to think of something.

Thanks,
Roderick

>
> > Thanks,
> > Roderick
> >
> > On Sun, Jan 26, 2020 at 12:28 PM Martyn Welch <martyn@welchs.me.uk>
> > wrote:
> > > There seems to be a number of subtly different firmwares for the
> > > Playstation controllers made by "Gasia Co.,Ltd". Whilst such
> > > controllers
> > > are easily detectable when attached via USB that is not always the
> > > case
> > > via Bluetooth. Some controllers are named "PLAYSTATION(R)3
> > > Controller"
> > > where as the official Sony controllers are named
> > > "Sony PLAYSTATION(R)3 Controller", however some versions of
> > > firmware use
> > > the exact name used by the official controllers. The only way I've
> > > been
> > > able to distinguish these versions of the controller (when
> > > connected via
> > > Bluetooth) is that the Bluetooth Class of Device incorrectly
> > > reports the
> > > controller as a keyboard rather than a gamepad. I've so far failed
> > > to work
> > > out how to access this information from a HID driver.
> > >
> > > The Gasia controllers need output reports to be configured in the
> > > same way
> > > as the Shanwan controllers. In order to ensure both types of Gasia
> > > firmware
> > > will work, this patch adds a quirk for those devices it can detect
> > > and
> > > reworks `sixaxis_send_output_report()` to attempt
> > > `hid_hw_output_report()`
> > > should `hid_hw_raw_request()` be known to be the wrong option (as
> > > is the
> > > case with the Shanwan controllers) or fails.
> > >
> > > This has got all the controllers I have working, with the slight
> > > anoyance that the Gasia controllers that don't currently get marked
> > > with
> > > a quirk require the call to `hid_hw_raw_request()` to fail before
> > > the
> > > controller finishes initialising (which adds a significant extra
> > > delay
> > > before the controller is ready).
> > >
> > > This patch is based on the following patch by Conn O'Griofa:
> > >
> > > https://github.com/RetroPie/RetroPie-Setup/pull/2263/commits/017f00f6e15f04b3272ff1abae8742dc4c47b608
> > >
> > > Cc: Conn O'Griofa <connogriofa@gmail.com>
> > > Signed-off-by: Martyn Welch <martyn@welchs.me.uk>
> > > ---
> > >  drivers/hid/hid-sony.c | 31 +++++++++++++++++++++++++------
> > >  1 file changed, 25 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> > > index 4c6ed6ef31f1..d1088a85cb59 100644
> > > --- a/drivers/hid/hid-sony.c
> > > +++ b/drivers/hid/hid-sony.c
> > > @@ -56,6 +56,7 @@
> > >  #define NSG_MR5U_REMOTE_BT        BIT(14)
> > >  #define NSG_MR7U_REMOTE_BT        BIT(15)
> > >  #define SHANWAN_GAMEPAD           BIT(16)
> > > +#define GASIA_GAMEPAD             BIT(17)
> > >
> > >  #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB |
> > > SIXAXIS_CONTROLLER_BT)
> > >  #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB |
> > > MOTION_CONTROLLER_BT)
> > > @@ -2067,6 +2068,7 @@ static void sixaxis_send_output_report(struct
> > > sony_sc *sc)
> > >         struct sixaxis_output_report *report =
> > >                 (struct sixaxis_output_report *)sc-
> > > >output_report_dmabuf;
> > >         int n;
> > > +       int ret = -1;
> > >
> > >         /* Initialize the report with default values */
> > >         memcpy(report, &default_report, sizeof(struct
> > > sixaxis_output_report));
> > > @@ -2101,14 +2103,23 @@ static void
> > > sixaxis_send_output_report(struct sony_sc *sc)
> > >                 }
> > >         }
> > >
> > > -       /* SHANWAN controllers require output reports via intr
> > > channel */
> > > -       if (sc->quirks & SHANWAN_GAMEPAD)
> > > -               hid_hw_output_report(sc->hdev, (u8 *)report,
> > > -                               sizeof(struct
> > > sixaxis_output_report));
> > > -       else
> > > -               hid_hw_raw_request(sc->hdev, report->report_id, (u8
> > > *)report,
> > > +       /*
> > > +        * SHANWAN & GASIA controllers require output reports via
> > > intr channel.
> > > +        * Some of the Gasia controllers are basically
> > > indistinguishable from
> > > +        * the official ones and thus try hid_hw_output_report()
> > > should
> > > +        * hid_hw_raw_request() fail.
> > > +        */
> > > +       if (!(sc->quirks & (SHANWAN_GAMEPAD | GASIA_GAMEPAD)))
> > > +               ret = hid_hw_raw_request(sc->hdev, report-
> > > >report_id,
> > > +                               (u8 *)report,
> > >                                 sizeof(struct
> > > sixaxis_output_report),
> > >                                 HID_OUTPUT_REPORT,
> > > HID_REQ_SET_REPORT);
> > > +
> > > +       if (ret >= 0)
> > > +               return;
> >
> > I don't like this pattern so much. We only need this "ret" check for
> > SHANWAN and GASIA. I would just do:
> >
> > if (!(sc->quirks & (SHANWAN_GAMEPAD | GASIA_GAMEPAD))) {
> >     int ret = hid_hw_raw_request(....);
> >     if (ret >= 0)
> >        return;
> > }
> >
> > hid_hw_output_report(....)
> >
> > > +
> > > +       hid_hw_output_report(sc->hdev, (u8 *)report,
> > > +                       sizeof(struct sixaxis_output_report));
> > >  }
> > >
> > >  static void dualshock4_send_output_report(struct sony_sc *sc)
> > > @@ -2833,6 +2844,14 @@ static int sony_probe(struct hid_device
> > > *hdev, const struct hid_device_id *id)
> > >         if (!strcmp(hdev->name, "SHANWAN PS3 GamePad"))
> > >                 quirks |= SHANWAN_GAMEPAD;
> > >
> > > +       /*
> > > +        * Some Gasia controllers are named "PLAYSTATION(R)3
> > > Controller"
> > > +        * where as the official Sony controllers are named
> > > +        * "Sony PLAYSTATION(R)3 Controller".
> > > +        */
> > > +       if (!strcmp(hdev->name, "PLAYSTATION(R)3 Controller"))
> > > +               quirks |= GASIA_GAMEPAD;
> > > +
> > >         sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
> > >         if (sc == NULL) {
> > >                 hid_err(hdev, "can't alloc sony descriptor\n");
> > > --
> > > 2.20.1
> > >
>

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Jiri Kosina]

On Tue, 28 Jan 2020, Roderick Colenbrander wrote:

> Let me explain the situation a little bit better from our angle. These 
> devices exist and from the Linux community perspective of course they 
> should see some level of support. And as I said since this is PS3 
> generation I don't have much of a concern.
> 
> Where it becomes tricky for any company in our situation is the support 
> side. We don't know these devices and don't have access to datasheets or 
> anything, but when such code is in our "official driver" it means we 
> have to involve them in our QA process and support them in some manner 
> (kind of legitimizing their existence as well). We now support this 
> driver in a large capacity (pretty much all mobile devices will start 
> shipping it) it puts challenges on our partners (not a big issue since 
> just PS3 right now).
> 
> As you can see this creates an awkward situation. I'm sure there other 
> such devices as well e.g. counterfeit Logitech keyboards, USB serial 
> adapters and other periperhals with similar challenges. In an ideal 
> world the support would live in another driver, but since in case of 
> this "fake" PS3 controller it "share" our product / vendor ids it is 
> tricky. At this point there is not a strong enough case yet to augment 
> the "hid-quirks" to do so, but perhaps if it became a serious issue 
> (e.g. for newer controllers) maybe we need to think of something.

If this is a big issue for you, one possible way around it would be 
creating a module parameter which would tell the driver whether it should 
those "fake" devices, and you can then turn it off in your products (or we 
can of course start the "what should the default setting me" bikeshedding 
:) ).

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Benjamin Tissoires]

On Mon, Feb 3, 2020 at 11:02 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Tue, 28 Jan 2020, Roderick Colenbrander wrote:
>
> > Let me explain the situation a little bit better from our angle. These
> > devices exist and from the Linux community perspective of course they
> > should see some level of support. And as I said since this is PS3
> > generation I don't have much of a concern.
> >
> > Where it becomes tricky for any company in our situation is the support
> > side. We don't know these devices and don't have access to datasheets or
> > anything, but when such code is in our "official driver" it means we
> > have to involve them in our QA process and support them in some manner
> > (kind of legitimizing their existence as well). We now support this
> > driver in a large capacity (pretty much all mobile devices will start
> > shipping it) it puts challenges on our partners (not a big issue since
> > just PS3 right now).
> >
> > As you can see this creates an awkward situation. I'm sure there other
> > such devices as well e.g. counterfeit Logitech keyboards, USB serial
> > adapters and other periperhals with similar challenges. In an ideal
> > world the support would live in another driver, but since in case of
> > this "fake" PS3 controller it "share" our product / vendor ids it is
> > tricky. At this point there is not a strong enough case yet to augment
> > the "hid-quirks" to do so, but perhaps if it became a serious issue
> > (e.g. for newer controllers) maybe we need to think of something.
>
> If this is a big issue for you, one possible way around it would be
> creating a module parameter which would tell the driver whether it should
> those "fake" devices, and you can then turn it off in your products (or we
> can of course start the "what should the default setting me" bikeshedding
> :) ).
>

I am definitely not in favour of that :(

The basic problem we have here is that some vendors are overriding
your VID/PIDs, and this is nasty. And I do not see any reasons why you
can't say: "well, we broke it, sorry, but we only support *our*
devices, not third party ones".

One thing that comes to my mind (probably not the best solution), is
to taint the kernel if you are facing a non genuine product. We do
that for nvidia, and basically, we can say: "well, supporting the
nvidia blob is done on a best effort case, and see with them directly
if you have an issue".
Tainting the kernel is a little bit rough, but maybe adding an info
message in the dmesg if you detect one of those can lead to a
situation were we can count on you for supporting the official
products, and you can get community support for the clones.

One last thing. Roderick, I am not sure if I mentioned that or not,
but I am heavily adding regression tests for HID in
https://gitlab.freedesktop.org/libevdev/hid-tools/
Given that hid-sony.ko seems to only use pure HID communication, it
should be easy enough to write regression tests for the devices, and
this way you can split up the QA in 2: automated and upstream tests
run by me for all devices handled by hid-sony, and your QA can focus
on the actual physical hardware, and ignore all of the clones.

Cheers,
Benjamin

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Jiri Kosina]

On Mon, 3 Feb 2020, Benjamin Tissoires wrote:

> I am definitely not in favour of that :(
> 
> The basic problem we have here is that some vendors are overriding your 
> VID/PIDs, and this is nasty. And I do not see any reasons why you can't 
> say: "well, we broke it, sorry, but we only support *our* devices, not 
> third party ones".

Well, it's not about "we broke it" in the first place, as far as I 
can tell.

Roderick's concern is that 3rd party devices with overriden VID/PID 
malfunction for completely unrelated reason to (correctly working) changes 
done in favor of stock Sony devices, but it'll be Sony receiving all the 
reports/blame.

> One thing that comes to my mind (probably not the best solution), is to 
> taint the kernel if you are facing a non genuine product. We do that for 
> nvidia, and basically, we can say: "well, supporting the nvidia blob is 
> done on a best effort case, and see with them directly if you have an 
> issue". Tainting the kernel is a little bit rough, but maybe adding an 
> info message in the dmesg if you detect one of those can lead to a 
> situation were we can count on you for supporting the official products, 
> and you can get community support for the clones.

Yeah; which I wouldn't like to do for upstream kernel, but Sony could 
definitely do this for the products they ship.

The same way distros are tainting their kernels when unsupported modules 
(but otherwise perfectly fine wrt. GPL and everything else) are loaded 
into distro-supported kernels.

> One last thing. Roderick, I am not sure if I mentioned that or not, but 
> I am heavily adding regression tests for HID in 
> https://gitlab.freedesktop.org/libevdev/hid-tools/

... and words can't express how thankful I am for that :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Benjamin Tissoires]

Hi,

On Mon, Feb 3, 2020 at 12:23 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Mon, 3 Feb 2020, Benjamin Tissoires wrote:
>
> > I am definitely not in favour of that :(
> >
> > The basic problem we have here is that some vendors are overriding your
> > VID/PIDs, and this is nasty. And I do not see any reasons why you can't
> > say: "well, we broke it, sorry, but we only support *our* devices, not
> > third party ones".
>
> Well, it's not about "we broke it" in the first place, as far as I
> can tell.
>
> Roderick's concern is that 3rd party devices with overriden VID/PID
> malfunction for completely unrelated reason to (correctly working) changes
> done in favor of stock Sony devices, but it'll be Sony receiving all the
> reports/blame.

After re-reading the code, I am not sure we can easily detect the
clones. So at some point, I think we will break them, but there is not
much we can do. I don't really have a solution for that :(

>
> > One thing that comes to my mind (probably not the best solution), is to
> > taint the kernel if you are facing a non genuine product. We do that for
> > nvidia, and basically, we can say: "well, supporting the nvidia blob is
> > done on a best effort case, and see with them directly if you have an
> > issue". Tainting the kernel is a little bit rough, but maybe adding an
> > info message in the dmesg if you detect one of those can lead to a
> > situation were we can count on you for supporting the official products,
> > and you can get community support for the clones.
>
> Yeah; which I wouldn't like to do for upstream kernel, but Sony could
> definitely do this for the products they ship.
>
> The same way distros are tainting their kernels when unsupported modules
> (but otherwise perfectly fine wrt. GPL and everything else) are loaded
> into distro-supported kernels.
>
> > One last thing. Roderick, I am not sure if I mentioned that or not, but
> > I am heavily adding regression tests for HID in
> > https://gitlab.freedesktop.org/libevdev/hid-tools/
>
> ... and words can't express how thankful I am for that :)
>

OK, I played with that idea earlier this week:
https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/74
I only have a Sixaxis controller, and I only implemented the USB part
of it (AFAICT).
Currently this ensures the button mapping is correct, and that the
LEDs are working properly.
We are still missing a few bits and pieces, but the initialization
(requests made by the kernel to start the device and press on the PS
button) is handled properly.

If this is something Roderick would be interested in, we can then try
to extend this initial work on Bluetooth controllers and the DualShock
ones.
Adding the clones ones based on the current kernel code is something
doable, but I do not expect Sony to be involved in that process.

That being said, before we merge this particular patch about Gasia
controllers, now we need to implement a regression test first :)

Cheers,
Benjamin

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Bastien Nocera]

On Thu, 2020-02-06 at 09:09 +0100, Benjamin Tissoires wrote:
> 
<snip>
> If this is something Roderick would be interested in, we can then try
> to extend this initial work on Bluetooth controllers and the
> DualShock
> ones.
> Adding the clones ones based on the current kernel code is something
> doable, but I do not expect Sony to be involved in that process.

Sony didn't provide any of the code that allows us to support those
devices over Bluetooth, and support isn't complete either.

I'd certainly appreciate getting information about how to pair those
devices (if there's anything on top of the code already implemented),
how to pair the PS3 headset and keyboard accessories (which are still
unsupported), and how to access the headset pairing for the PS4
controllers.

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Roderick Colenbrander]

On Thu, Feb 6, 2020 at 1:34 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Thu, 2020-02-06 at 09:09 +0100, Benjamin Tissoires wrote:
> >
> <snip>
> > If this is something Roderick would be interested in, we can then try
> > to extend this initial work on Bluetooth controllers and the
> > DualShock
> > ones.
> > Adding the clones ones based on the current kernel code is something
> > doable, but I do not expect Sony to be involved in that process.
>
> Sony didn't provide any of the code that allows us to support those
> devices over Bluetooth, and support isn't complete either.
>
> I'd certainly appreciate getting information about how to pair those
> devices (if there's anything on top of the code already implemented),
> how to pair the PS3 headset and keyboard accessories (which are still
> unsupported), and how to access the headset pairing for the PS4
> controllers.
>

At this point our main priority is supported related to DS4 (this is
what our permission is for). The other areas are very hard for me to
get info on, so I can't promise that right now. Audio related stuff
for DS4 is a maybe at some point. It is very complicated and all
tunnelled through HID... (in case of Bluetooth).

Thanks,
Roderick

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Roderick Colenbrander]

On Thu, Feb 6, 2020 at 12:10 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi,
>
> On Mon, Feb 3, 2020 at 12:23 PM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Mon, 3 Feb 2020, Benjamin Tissoires wrote:
> >
> > > I am definitely not in favour of that :(
> > >
> > > The basic problem we have here is that some vendors are overriding your
> > > VID/PIDs, and this is nasty. And I do not see any reasons why you can't
> > > say: "well, we broke it, sorry, but we only support *our* devices, not
> > > third party ones".
> >
> > Well, it's not about "we broke it" in the first place, as far as I
> > can tell.
> >
> > Roderick's concern is that 3rd party devices with overriden VID/PID
> > malfunction for completely unrelated reason to (correctly working) changes
> > done in favor of stock Sony devices, but it'll be Sony receiving all the
> > reports/blame.
>
> After re-reading the code, I am not sure we can easily detect the
> clones. So at some point, I think we will break them, but there is not
> much we can do. I don't really have a solution for that :(
>
> >
> > > One thing that comes to my mind (probably not the best solution), is to
> > > taint the kernel if you are facing a non genuine product. We do that for
> > > nvidia, and basically, we can say: "well, supporting the nvidia blob is
> > > done on a best effort case, and see with them directly if you have an
> > > issue". Tainting the kernel is a little bit rough, but maybe adding an
> > > info message in the dmesg if you detect one of those can lead to a
> > > situation were we can count on you for supporting the official products,
> > > and you can get community support for the clones.
> >
> > Yeah; which I wouldn't like to do for upstream kernel, but Sony could
> > definitely do this for the products they ship.
> >
> > The same way distros are tainting their kernels when unsupported modules
> > (but otherwise perfectly fine wrt. GPL and everything else) are loaded
> > into distro-supported kernels.
> >
> > > One last thing. Roderick, I am not sure if I mentioned that or not, but
> > > I am heavily adding regression tests for HID in
> > > https://gitlab.freedesktop.org/libevdev/hid-tools/
> >
> > ... and words can't express how thankful I am for that :)
> >
>
> OK, I played with that idea earlier this week:
> https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/74
> I only have a Sixaxis controller, and I only implemented the USB part
> of it (AFAICT).
> Currently this ensures the button mapping is correct, and that the
> LEDs are working properly.
> We are still missing a few bits and pieces, but the initialization
> (requests made by the kernel to start the device and press on the PS
> button) is handled properly.
>
> If this is something Roderick would be interested in, we can then try
> to extend this initial work on Bluetooth controllers and the DualShock
> ones.

We can probably help out there (need to ask official permission). We
have similar tests in Android (still adding more). Just in case you
are not familiar this is their framework:
https://android.googlesource.com/platform/cts/+/master/tests/tests/hardware/src/android/hardware/input/cts/tests/

It is a small Java class and then there is a json blob with the actual
test (forgot where the json is). It defines the report descriptors
etcetera.

Thanks,
Roderick

> Adding the clones ones based on the current kernel code is something
> doable, but I do not expect Sony to be involved in that process.
>
> That being said, before we merge this particular patch about Gasia
> controllers, now we need to implement a regression test first :)
>
> Cheers,
> Benjamin
>

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Benjamin Tissoires]

Hey,

On Thu, Feb 6, 2020 at 4:31 PM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> On Thu, Feb 6, 2020 at 12:10 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Hi,
> >
> > On Mon, Feb 3, 2020 at 12:23 PM Jiri Kosina <jikos@kernel.org> wrote:
> > >
> > > On Mon, 3 Feb 2020, Benjamin Tissoires wrote:
> > >
> > > > I am definitely not in favour of that :(
> > > >
> > > > The basic problem we have here is that some vendors are overriding your
> > > > VID/PIDs, and this is nasty. And I do not see any reasons why you can't
> > > > say: "well, we broke it, sorry, but we only support *our* devices, not
> > > > third party ones".
> > >
> > > Well, it's not about "we broke it" in the first place, as far as I
> > > can tell.
> > >
> > > Roderick's concern is that 3rd party devices with overriden VID/PID
> > > malfunction for completely unrelated reason to (correctly working) changes
> > > done in favor of stock Sony devices, but it'll be Sony receiving all the
> > > reports/blame.
> >
> > After re-reading the code, I am not sure we can easily detect the
> > clones. So at some point, I think we will break them, but there is not
> > much we can do. I don't really have a solution for that :(
> >
> > >
> > > > One thing that comes to my mind (probably not the best solution), is to
> > > > taint the kernel if you are facing a non genuine product. We do that for
> > > > nvidia, and basically, we can say: "well, supporting the nvidia blob is
> > > > done on a best effort case, and see with them directly if you have an
> > > > issue". Tainting the kernel is a little bit rough, but maybe adding an
> > > > info message in the dmesg if you detect one of those can lead to a
> > > > situation were we can count on you for supporting the official products,
> > > > and you can get community support for the clones.
> > >
> > > Yeah; which I wouldn't like to do for upstream kernel, but Sony could
> > > definitely do this for the products they ship.
> > >
> > > The same way distros are tainting their kernels when unsupported modules
> > > (but otherwise perfectly fine wrt. GPL and everything else) are loaded
> > > into distro-supported kernels.
> > >
> > > > One last thing. Roderick, I am not sure if I mentioned that or not, but
> > > > I am heavily adding regression tests for HID in
> > > > https://gitlab.freedesktop.org/libevdev/hid-tools/
> > >
> > > ... and words can't express how thankful I am for that :)
> > >
> >
> > OK, I played with that idea earlier this week:
> > https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/74
> > I only have a Sixaxis controller, and I only implemented the USB part
> > of it (AFAICT).
> > Currently this ensures the button mapping is correct, and that the
> > LEDs are working properly.
> > We are still missing a few bits and pieces, but the initialization
> > (requests made by the kernel to start the device and press on the PS
> > button) is handled properly.
> >
> > If this is something Roderick would be interested in, we can then try
> > to extend this initial work on Bluetooth controllers and the DualShock
> > ones.
>
> We can probably help out there (need to ask official permission). We
> have similar tests in Android (still adding more). Just in case you
> are not familiar this is their framework:
> https://android.googlesource.com/platform/cts/+/master/tests/tests/hardware/src/android/hardware/input/cts/tests/

thanks. That's a good pointer I wasn't aware of.

>
> It is a small Java class and then there is a json blob with the actual
> test (forgot where the json is). It defines the report descriptors
> etcetera.

Found them at https://android.googlesource.com/platform/cts/+/master/tests/tests/hardware/res/raw

Of course, I had to find advantages to my own test suite (in case you
need to explain to management):
- I am running it upstream on any patch that comes in, so less chances
to catch a failure after the fact
- I am emulating the firmware more precisely IMO (it's a python class
and you can overwrite the set_report, get_report and set_output
report)
- I am emulating both USB and Bluetooth (or whatever bus you want)
- I am testing the LED classes
- we can easily extend to test the rumbles and the battery reporting
- I am not relying on preformatted reports, meaning that it's harder
to cheat in the driver and we can extend the test cases more easily
(what if we have a left d-pad + button 7 that runs into a problem in
the driver?)

Anyway, I just merged the PS3 controller I have. I'll try to see if I
can get the DS4 working based on those json files.

Cheers,
Benjamin

>
> Thanks,
> Roderick
>
> > Adding the clones ones based on the current kernel code is something
> > doable, but I do not expect Sony to be involved in that process.
> >
> > That being said, before we merge this particular patch about Gasia
> > controllers, now we need to implement a regression test first :)
> >
> > Cheers,
> > Benjamin
> >
>

Re: [PATCH] HID: Sony: Add support for Gasia controllers

[Bastien Nocera]

On Tue, 2020-01-28 at 22:22 -0800, Roderick Colenbrander wrote:
> As you can see this creates an awkward situation. I'm sure there
> other
> such devices as well e.g. counterfeit Logitech keyboards, USB serial
> adapters and other periperhals with similar challenges. In an ideal
> world the support would live in another driver, but since in case of
> this "fake" PS3 controller it "share" our product / vendor ids it is
> tricky. At this point there is not a strong enough case yet to
> augment
> the "hid-quirks" to do so, but perhaps if it became a serious issue
> (e.g. for newer controllers) maybe we need to think of something.

I'm pretty certain that the only reason why those use "fake" VID/PID
combinations is because that's the only thing the consoles will
accept. 

That's par for the course when you try to close off your ecosystem that
those that try to enter it will use the means at their disposal to do
that.