On Wed, Sep 14, 2022 at 9:51 AM Johnothan King <johnothanking@xxxxxxxxxxxxxx> 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@xxxxxxx> > Signed-off-by: Johnothan King <johnothanking@xxxxxxxxxxxxxx> > --- > 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 > >
