| Message ID | 1475636338-3779-5-git-send-email-roderick@gaikai.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[adding Frank, Simon and Antonio as they are the main developpers of hid-sony] On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote: > From: Roderick Colenbrander <roderick.colenbrander@sony.com> > > Add a CRC value to each output report. This removes the need for the > 'no output reports on interrupt end-point' quirk. > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com> > --- > drivers/hid/hid-sony.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index 34988ce..24f7d19 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -1895,7 +1895,7 @@ static void dualshock4_send_output_report(struct sony_sc *sc) > } else { > memset(buf, 0, DS4_OUTPUT_REPORT_0x11_SIZE); > buf[0] = 0x11; > - buf[1] = 0x80; > + buf[1] = 0xC0; /* HID + CRC */ > buf[3] = 0x0F; > offset = 6; > } > @@ -1922,9 +1922,16 @@ static void dualshock4_send_output_report(struct sony_sc *sc) > > if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) > hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x05_SIZE); > - else > - hid_hw_raw_request(hdev, 0x11, buf, DS4_OUTPUT_REPORT_0x11_SIZE, > - HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); > + else { > + /* CRC generation */ > + u8 bthdr = 0xA2; > + u32 crc; > + > + crc = crc32_le(0xFFFFFFFF, &bthdr, 1); > + crc = ~crc32_le(crc, buf, DS4_OUTPUT_REPORT_0x11_SIZE-4); > + put_unaligned_le32(crc, &buf[74]); > + hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE); > + } nitpicking: hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE); could be factorized out if you add the crc only for BT devices. > } > > static void motion_send_output_report(struct sony_sc *sc) > @@ -2378,11 +2385,6 @@ static int sony_input_configured(struct hid_device *hdev, > sony_init_output_report(sc, sixaxis_send_output_report); > } else if (sc->quirks & DUALSHOCK4_CONTROLLER) { > if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) { > - /* > - * The DualShock 4 wants output reports sent on the ctrl > - * endpoint when connected via Bluetooth. > - */ > - hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP; The purpose of this quirk is only to have hidraw behaving like legacy when the HID low-level transport has been worked on. So basically, this might potentially break userspace as the CRC doesn't get added automatically by the driver when talking to the device through hidraw. Frank, Simon, Antonio, are there any userspace application that need to communicate over hidraw to the controllers or is it entirely done in the kernel now? Cheers, Benjamin > ret = dualshock4_set_operational_bt(hdev); > if (ret < 0) { > hid_err(hdev, "failed to set the Dualshock 4 operational mode\n"); > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Oct 5, 2016, at 04:31, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > [adding Frank, Simon and Antonio as they are the main developpers of > hid-sony] > > On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote: >> From: Roderick Colenbrander <roderick.colenbrander@sony.com> >> >> Add a CRC value to each output report. This removes the need for the >> 'no output reports on interrupt end-point' quirk. >> >> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com> >> --- >> drivers/hid/hid-sony.c | 20 +++++++++++--------- >> 1 file changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c >> index 34988ce..24f7d19 100644 >> --- a/drivers/hid/hid-sony.c >> +++ b/drivers/hid/hid-sony.c >> @@ -1895,7 +1895,7 @@ static void dualshock4_send_output_report(struct sony_sc *sc) >> } else { >> memset(buf, 0, DS4_OUTPUT_REPORT_0x11_SIZE); >> buf[0] = 0x11; >> - buf[1] = 0x80; >> + buf[1] = 0xC0; /* HID + CRC */ >> buf[3] = 0x0F; >> offset = 6; >> } >> @@ -1922,9 +1922,16 @@ static void dualshock4_send_output_report(struct sony_sc *sc) >> >> if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) >> hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x05_SIZE); >> - else >> - hid_hw_raw_request(hdev, 0x11, buf, DS4_OUTPUT_REPORT_0x11_SIZE, >> - HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); >> + else { >> + /* CRC generation */ >> + u8 bthdr = 0xA2; >> + u32 crc; >> + >> + crc = crc32_le(0xFFFFFFFF, &bthdr, 1); >> + crc = ~crc32_le(crc, buf, DS4_OUTPUT_REPORT_0x11_SIZE-4); >> + put_unaligned_le32(crc, &buf[74]); >> + hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE); >> + } > > nitpicking: > hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE); could be > factorized out if you add the crc only for BT devices. > >> } >> >> static void motion_send_output_report(struct sony_sc *sc) >> @@ -2378,11 +2385,6 @@ static int sony_input_configured(struct hid_device *hdev, >> sony_init_output_report(sc, sixaxis_send_output_report); >> } else if (sc->quirks & DUALSHOCK4_CONTROLLER) { >> if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) { >> - /* >> - * The DualShock 4 wants output reports sent on the ctrl >> - * endpoint when connected via Bluetooth. >> - */ >> - hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP; > > The purpose of this quirk is only to have hidraw behaving like legacy > when the HID low-level transport has been worked on. > So basically, this might potentially break userspace as the CRC doesn't > get added automatically by the driver when talking to the device through > hidraw. > > Frank, Simon, Antonio, are there any userspace application that need to > communicate over hidraw to the controllers or is it entirely done in the > kernel now? > > Cheers, > Benjamin The best answer I can give is "not to my knowledge”. Rumble and LEDs have standard kernel interfaces but obviously we can’t guarantee 100% that nobody is using hidraw directly. Commercial games and engines aren't running as root and thus can’t use hidraw so I think we can safely say that no major commercial software will be broken by this. There are people who have had to alter the reporting rate of the controller for use with slow embedded processors. With this change can the rate-control value be altered from the full-speed value of C0? I guess that in these corner cases people can revert it to the old behavior themselves if necessary since they have to customize the module anyways. Regards, Frank > >> ret = dualshock4_set_operational_bt(hdev); >> if (ret < 0) { >> hid_err(hdev, "failed to set the Dualshock 4 operational mode\n"); >> -- >> 2.7.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, October 5, 2016 10:54 am, Frank Praznik wrote: > The best answer I can give is "not to my knowledge”. Rumble and LEDs > have standard kernel interfaces but obviously we can’t guarantee 100% that > nobody is using hidraw directly. Commercial games and engines aren't > running as root and thus can’t use hidraw so I think we can safely say > that no major commercial software will be broken by this. There are a couple of projects I know of, but I am unable to say whether they are still active or whether proposed changes will affect them... https://thp.io/2010/psmove/ http://moveonpc.blogspot.ca/ https://github.com/chrippa/ds4drv Audio output is one area of the DS4 which is not supported in kernel, maybe some renewed interest can get this working. I agree with Frank that we should move forward, if something gets broken we can work a better solution. Simon. acked-by: Simon Wood <simon@mungewell.org> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 5, 2016 at 9:54 AM, Frank Praznik <frank.praznik@gmail.com> wrote: > >> On Oct 5, 2016, at 04:31, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: >> >> [adding Frank, Simon and Antonio as they are the main developpers of >> hid-sony] >> >> On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote: >>> From: Roderick Colenbrander <roderick.colenbrander@sony.com> >>> >>> Add a CRC value to each output report. This removes the need for the >>> 'no output reports on interrupt end-point' quirk. >>> >>> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com> >>> --- >>> drivers/hid/hid-sony.c | 20 +++++++++++--------- >>> 1 file changed, 11 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c >>> index 34988ce..24f7d19 100644 >>> --- a/drivers/hid/hid-sony.c >>> +++ b/drivers/hid/hid-sony.c >>> @@ -1895,7 +1895,7 @@ static void dualshock4_send_output_report(struct sony_sc *sc) >>> } else { >>> memset(buf, 0, DS4_OUTPUT_REPORT_0x11_SIZE); >>> buf[0] = 0x11; >>> - buf[1] = 0x80; >>> + buf[1] = 0xC0; /* HID + CRC */ >>> buf[3] = 0x0F; >>> offset = 6; >>> } >>> @@ -1922,9 +1922,16 @@ static void dualshock4_send_output_report(struct sony_sc *sc) >>> >>> if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) >>> hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x05_SIZE); >>> - else >>> - hid_hw_raw_request(hdev, 0x11, buf, DS4_OUTPUT_REPORT_0x11_SIZE, >>> - HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); >>> + else { >>> + /* CRC generation */ >>> + u8 bthdr = 0xA2; >>> + u32 crc; >>> + >>> + crc = crc32_le(0xFFFFFFFF, &bthdr, 1); >>> + crc = ~crc32_le(crc, buf, DS4_OUTPUT_REPORT_0x11_SIZE-4); >>> + put_unaligned_le32(crc, &buf[74]); >>> + hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE); >>> + } >> >> nitpicking: >> hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE); could be >> factorized out if you add the crc only for BT devices. >> >>> } >>> >>> static void motion_send_output_report(struct sony_sc *sc) >>> @@ -2378,11 +2385,6 @@ static int sony_input_configured(struct hid_device *hdev, >>> sony_init_output_report(sc, sixaxis_send_output_report); >>> } else if (sc->quirks & DUALSHOCK4_CONTROLLER) { >>> if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) { >>> - /* >>> - * The DualShock 4 wants output reports sent on the ctrl >>> - * endpoint when connected via Bluetooth. >>> - */ >>> - hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP; >> >> The purpose of this quirk is only to have hidraw behaving like legacy >> when the HID low-level transport has been worked on. >> So basically, this might potentially break userspace as the CRC doesn't >> get added automatically by the driver when talking to the device through >> hidraw. >> >> Frank, Simon, Antonio, are there any userspace application that need to >> communicate over hidraw to the controllers or is it entirely done in the >> kernel now? >> >> Cheers, >> Benjamin > > The best answer I can give is "not to my knowledge”. Rumble and LEDs have standard > kernel interfaces but obviously we can’t guarantee 100% that nobody is using hidraw > directly. Commercial games and engines aren't running as root and thus can’t use > hidraw so I think we can safely say that no major commercial software will be broken > by this. > > There are people who have had to alter the reporting rate of the controller for use with > slow embedded processors. With this change can the rate-control value be altered > from the full-speed value of C0? > > I guess that in these corner cases people can revert it to the old behavior themselves if > necessary since they have to customize the module anyways. > > Regards, > Frank > To be honest I'm not sure how the rate control works. I think it is handled outside the driver more by the bluetooth layer, but I'm not sure. At least those few upper bits we changed don't seem to be related to the speed. Thanks, Roderick -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Oct 5, 2016, at 13:35, Simon Wood <simon@mungewell.org> wrote: > > On Wed, October 5, 2016 10:54 am, Frank Praznik wrote: >> The best answer I can give is "not to my knowledge”. Rumble and LEDs >> have standard kernel interfaces but obviously we can’t guarantee 100% that >> nobody is using hidraw directly. Commercial games and engines aren't >> running as root and thus can’t use hidraw so I think we can safely say >> that no major commercial software will be broken by this. > > There are a couple of projects I know of, but I am unable to say whether > they are still active or whether proposed changes will affect them... > https://thp.io/2010/psmove/ > http://moveonpc.blogspot.ca/ > https://github.com/chrippa/ds4drv > > Audio output is one area of the DS4 which is not supported in kernel, > maybe some renewed interest can get this working. > > I agree with Frank that we should move forward, if something gets broken > we can work a better solution. > Simon. > > acked-by: Simon Wood <simon@mungewell.org> > > > This change only affects sending rumble/LED output reports to the DS4, so hidraw input reports and sending/receiving feature reports will be unchanged as will anything not in the DS4 codepath (ie. Move controllers). It looks like ds4drv has both a hidraw backend and one which uses raw Bluetooth sockets. The raw socket backend should be unaffected, but the hidraw backend will probably have issues. It should be trivial to patch it to append the CRC to the raw data packet like the kernel module does though. It also looks like hidraw isn’t the default Bluetooth backend which reduces the odds that it will be an issue even further. I’m definitely in favor of moving toward more correct behavior as long as the ds4drv project is given a warning to start sending CRC-ed output reports. acked-by: Frank Praznik <frank.praznik@gmail.com>-- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 05 2016 or thereabouts, Frank Praznik wrote: > > > On Oct 5, 2016, at 13:35, Simon Wood <simon@mungewell.org> wrote: > > > > On Wed, October 5, 2016 10:54 am, Frank Praznik wrote: > >> The best answer I can give is "not to my knowledge”. Rumble and LEDs > >> have standard kernel interfaces but obviously we can’t guarantee 100% that > >> nobody is using hidraw directly. Commercial games and engines aren't > >> running as root and thus can’t use hidraw so I think we can safely say > >> that no major commercial software will be broken by this. > > > > There are a couple of projects I know of, but I am unable to say whether > > they are still active or whether proposed changes will affect them... > > https://thp.io/2010/psmove/ > > http://moveonpc.blogspot.ca/ > > https://github.com/chrippa/ds4drv > > > > Audio output is one area of the DS4 which is not supported in kernel, > > maybe some renewed interest can get this working. > > > > I agree with Frank that we should move forward, if something gets broken > > we can work a better solution. > > Simon. > > > > acked-by: Simon Wood <simon@mungewell.org> > > > > > > > > This change only affects sending rumble/LED output reports to the DS4, so hidraw input reports and sending/receiving feature reports will be unchanged as will anything not in the DS4 codepath (ie. Move controllers). It looks like ds4drv has both a hidraw backend and one which uses raw Bluetooth sockets. The raw socket backend should be unaffected, but the hidraw backend will probably have issues. It should be trivial to patch it to append the CRC to the raw data packet like the kernel module does though. It also looks like hidraw isn’t the default Bluetooth backend which reduces the odds that it will be an issue even further. > > I’m definitely in favor of moving toward more correct behavior as long as the ds4drv project is given a warning to start sending CRC-ed output reports. > > acked-by: Frank Praznik <frank.praznik@gmail.com> Then: Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> For the user-space change: Acked-by me too. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index 34988ce..24f7d19 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c @@ -1895,7 +1895,7 @@ static void dualshock4_send_output_report(struct sony_sc *sc) } else { memset(buf, 0, DS4_OUTPUT_REPORT_0x11_SIZE); buf[0] = 0x11; - buf[1] = 0x80; + buf[1] = 0xC0; /* HID + CRC */ buf[3] = 0x0F; offset = 6; } @@ -1922,9 +1922,16 @@ static void dualshock4_send_output_report(struct sony_sc *sc) if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x05_SIZE); - else - hid_hw_raw_request(hdev, 0x11, buf, DS4_OUTPUT_REPORT_0x11_SIZE, - HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); + else { + /* CRC generation */ + u8 bthdr = 0xA2; + u32 crc; + + crc = crc32_le(0xFFFFFFFF, &bthdr, 1); + crc = ~crc32_le(crc, buf, DS4_OUTPUT_REPORT_0x11_SIZE-4); + put_unaligned_le32(crc, &buf[74]); + hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE); + } } static void motion_send_output_report(struct sony_sc *sc) @@ -2378,11 +2385,6 @@ static int sony_input_configured(struct hid_device *hdev, sony_init_output_report(sc, sixaxis_send_output_report); } else if (sc->quirks & DUALSHOCK4_CONTROLLER) { if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) { - /* - * The DualShock 4 wants output reports sent on the ctrl - * endpoint when connected via Bluetooth. - */ - hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP; ret = dualshock4_set_operational_bt(hdev); if (ret < 0) { hid_err(hdev, "failed to set the Dualshock 4 operational mode\n");