| Message ID | 20200126194513.6359-1-martyn@welchs.me.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: Sony: Add support for Gasia controllers | expand |
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 >
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,
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 > >
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 >
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 > > > >
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,
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
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,
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
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.
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.
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 >
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 > > >
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
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");
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(-)