| Message ID | ByMO0BENaLBLEnGGrPwe37i3VDtN-VuRlSHqkdgk7Q1JHQ16bI_S1QuEtqtSdeV0XcwGMZwrAkFEGaEdXN_Z1qaN2r1cFeZnu5TyHMxszIU=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | HID: nintendo: check analog user calibration for plausibility | expand |
On Wed, Sep 14, 2022 at 9:51 AM Johnothan King <johnothanking@protonmail.com> wrote: > > Arne Wendt writes: > Cheap clone controllers may (falsely) report as having a user > calibration for the analog sticks in place, but return > wrong/impossible values for the actual calibration data. > In the present case at mine, the controller reports having a > user calibration in place and successfully executes the read > commands. The reported user calibration however is > min = center = max = 0. > > This pull request addresses problems of this kind by checking the > provided user calibration-data for plausibility (min < center < max) > and falling back to the default values if implausible. > > I'll note that I was experiencing a crash because of this bug when using > the GuliKit KingKong 2 controller. The crash manifests as a divide by > zero error in the kernel logs: > kernel: divide error: 0000 [#1] PREEMPT SMP NOPTI > > Link: https://github.com/nicman23/dkms-hid-nintendo/pull/25 > Link: https://github.com/DanielOgorchock/linux/issues/36 > Co-authored-by: Arne Wendt <arne.wendt@tuhh.de> > Signed-off-by: Johnothan King <johnothanking@protonmail.com> > --- > drivers/hid/hid-nintendo.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > index 6028af3c3aae..7f287f6a75f5 100644 > --- a/drivers/hid/hid-nintendo.c > +++ b/drivers/hid/hid-nintendo.c > @@ -793,7 +793,17 @@ static int joycon_request_calibration(struct joycon_ctlr *ctlr) > &ctlr->left_stick_cal_x, > &ctlr->left_stick_cal_y, > true); > - if (ret) { > + > + /* > + * Check whether read succeeded and perform plausibility check > + * for retrieved values. > + */ > + if (ret || > + ctlr->left_stick_cal_x.min >= ctlr->left_stick_cal_x.center || > + ctlr->left_stick_cal_x.center >= ctlr->left_stick_cal_x.max || > + ctlr->left_stick_cal_y.min >= ctlr->left_stick_cal_y.center || > + ctlr->left_stick_cal_y.center >= ctlr->left_stick_cal_y.max > + ) { > hid_warn(ctlr->hdev, > "Failed to read left stick cal, using dflts; e=%d\n", > ret); > @@ -812,7 +822,17 @@ static int joycon_request_calibration(struct joycon_ctlr *ctlr) > &ctlr->right_stick_cal_x, > &ctlr->right_stick_cal_y, > false); > - if (ret) { > + > + /* > + * Check whether read succeeded and perform plausibility check > + * for retrieved values. > + */ > + if (ret || > + ctlr->right_stick_cal_x.min >= ctlr->right_stick_cal_x.center || > + ctlr->right_stick_cal_x.center >= ctlr->right_stick_cal_x.max || > + ctlr->right_stick_cal_y.min >= ctlr->right_stick_cal_y.center || > + ctlr->right_stick_cal_y.center >= ctlr->right_stick_cal_y.max Wouldn't it make sense to have a function that takes a single struct joycon_stick_cal pointer and does the check? This would make things more readable IMO. Cheers, Benjamin > + ) { > hid_warn(ctlr->hdev, > "Failed to read right stick cal, using dflts; e=%d\n", > ret); > -- > 2.37.3 > >
> Wouldn't it make sense to have a function that takes a single struct > joycon_stick_cal pointer and does the check? I acknowledge the current code can be simplified further. I will submit a second version of this patch that moves the plausibility check to joycon_read_stick_calibration() and adds a joycon_use_default_calibration() function. This will avoid the code duplication present in the current patch. In the v2 patch the check will still use both the cal_x and cal_y pointers. However, since the check will be reduced to just one if statement, that should be enough to simplify the code. The new function in the v2 patch (joycon_use_default_calibration()) was implemented so that I could simplify the existing default stick calibration code for reuse with both the left and right sticks. The new function in effect reduces four lines of code to one: -ctlr->left_stick_cal_x.center = DFLT_STICK_CAL_CEN; -ctlr->left_stick_cal_y.center = DFLT_STICK_CAL_CEN; -ctlr->right_stick_cal_x.center = DFLT_STICK_CAL_CEN; -ctlr->right_stick_cal_y.center = DFLT_STICK_CAL_CEN; +cal_x->center = cal_y->center = DFLT_STICK_CAL_CEN; - Johnothan King ------- Original Message ------- On Tuesday, September 20th, 2022 at 3:21 AM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > On Wed, Sep 14, 2022 at 9:51 AM Johnothan King > johnothanking@protonmail.com wrote: > > > Arne Wendt writes: > > Cheap clone controllers may (falsely) report as having a user > > calibration for the analog sticks in place, but return > > wrong/impossible values for the actual calibration data. > > In the present case at mine, the controller reports having a > > user calibration in place and successfully executes the read > > commands. The reported user calibration however is > > min = center = max = 0. > > > > This pull request addresses problems of this kind by checking the > > provided user calibration-data for plausibility (min < center < max) > > and falling back to the default values if implausible. > > > > I'll note that I was experiencing a crash because of this bug when using > > the GuliKit KingKong 2 controller. The crash manifests as a divide by > > zero error in the kernel logs: > > kernel: divide error: 0000 [#1] PREEMPT SMP NOPTI > > > > Link: https://github.com/nicman23/dkms-hid-nintendo/pull/25 > > Link: https://github.com/DanielOgorchock/linux/issues/36 > > Co-authored-by: Arne Wendt arne.wendt@tuhh.de > > Signed-off-by: Johnothan King johnothanking@protonmail.com > > --- > > drivers/hid/hid-nintendo.c | 24 ++++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > > index 6028af3c3aae..7f287f6a75f5 100644 > > --- a/drivers/hid/hid-nintendo.c > > +++ b/drivers/hid/hid-nintendo.c > > @@ -793,7 +793,17 @@ static int joycon_request_calibration(struct joycon_ctlr ctlr) > > &ctlr->left_stick_cal_x, > > &ctlr->left_stick_cal_y, > > true); > > - if (ret) { > > + > > + / > > + * Check whether read succeeded and perform plausibility check > > + * for retrieved values. > > + */ > > + if (ret || > > + ctlr->left_stick_cal_x.min >= ctlr->left_stick_cal_x.center || > > + ctlr->left_stick_cal_x.center >= ctlr->left_stick_cal_x.max || > > + ctlr->left_stick_cal_y.min >= ctlr->left_stick_cal_y.center || > > + ctlr->left_stick_cal_y.center >= ctlr->left_stick_cal_y.max > > + ) { > > hid_warn(ctlr->hdev, > > "Failed to read left stick cal, using dflts; e=%d\n", > > ret); > > @@ -812,7 +822,17 @@ static int joycon_request_calibration(struct joycon_ctlr ctlr) > > &ctlr->right_stick_cal_x, > > &ctlr->right_stick_cal_y, > > false); > > - if (ret) { > > + > > + / > > + * Check whether read succeeded and perform plausibility check > > + * for retrieved values. > > + */ > > + if (ret || > > + ctlr->right_stick_cal_x.min >= ctlr->right_stick_cal_x.center || > > + ctlr->right_stick_cal_x.center >= ctlr->right_stick_cal_x.max || > > + ctlr->right_stick_cal_y.min >= ctlr->right_stick_cal_y.center || > > + ctlr->right_stick_cal_y.center >= ctlr->right_stick_cal_y.max > > > Wouldn't it make sense to have a function that takes a single struct > joycon_stick_cal pointer and does the check? > This would make things more readable IMO. > > Cheers, > Benjamin > > > + ) { > > hid_warn(ctlr->hdev, > > "Failed to read right stick cal, using dflts; e=%d\n", > > ret); > > -- > > 2.37.3
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c index 6028af3c3aae..7f287f6a75f5 100644 --- a/drivers/hid/hid-nintendo.c +++ b/drivers/hid/hid-nintendo.c @@ -793,7 +793,17 @@ static int joycon_request_calibration(struct joycon_ctlr *ctlr) &ctlr->left_stick_cal_x, &ctlr->left_stick_cal_y, true); - if (ret) { + + /* + * Check whether read succeeded and perform plausibility check + * for retrieved values. + */ + if (ret || + ctlr->left_stick_cal_x.min >= ctlr->left_stick_cal_x.center || + ctlr->left_stick_cal_x.center >= ctlr->left_stick_cal_x.max || + ctlr->left_stick_cal_y.min >= ctlr->left_stick_cal_y.center || + ctlr->left_stick_cal_y.center >= ctlr->left_stick_cal_y.max + ) { hid_warn(ctlr->hdev, "Failed to read left stick cal, using dflts; e=%d\n", ret); @@ -812,7 +822,17 @@ static int joycon_request_calibration(struct joycon_ctlr *ctlr) &ctlr->right_stick_cal_x, &ctlr->right_stick_cal_y, false); - if (ret) { + + /* + * Check whether read succeeded and perform plausibility check + * for retrieved values. + */ + if (ret || + ctlr->right_stick_cal_x.min >= ctlr->right_stick_cal_x.center || + ctlr->right_stick_cal_x.center >= ctlr->right_stick_cal_x.max || + ctlr->right_stick_cal_y.min >= ctlr->right_stick_cal_y.center || + ctlr->right_stick_cal_y.center >= ctlr->right_stick_cal_y.max + ) { hid_warn(ctlr->hdev, "Failed to read right stick cal, using dflts; e=%d\n", ret);
Arne Wendt writes: Cheap clone controllers may (falsely) report as having a user calibration for the analog sticks in place, but return wrong/impossible values for the actual calibration data. In the present case at mine, the controller reports having a user calibration in place and successfully executes the read commands. The reported user calibration however is min = center = max = 0. This pull request addresses problems of this kind by checking the provided user calibration-data for plausibility (min < center < max) and falling back to the default values if implausible. I'll note that I was experiencing a crash because of this bug when using the GuliKit KingKong 2 controller. The crash manifests as a divide by zero error in the kernel logs: kernel: divide error: 0000 [#1] PREEMPT SMP NOPTI Link: https://github.com/nicman23/dkms-hid-nintendo/pull/25 Link: https://github.com/DanielOgorchock/linux/issues/36 Co-authored-by: Arne Wendt <arne.wendt@tuhh.de> Signed-off-by: Johnothan King <johnothanking@protonmail.com> --- drivers/hid/hid-nintendo.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)