On Wed, Sep 21, 2022 at 12:56 PM 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> > --- Thanks for the quick v4. Daniel, I applied it without your rev-by. I think we are close enough to what you reviewed previously. The only thing though is that I just realized I lost your rev-by from v2. If you really want it, I can probably force push the patch. Anyway, applied to for-6.1/nintendo in hid.git. Cheers, Benjamin > Changes in v2: > - Move the plausibility check to joycon_read_stick_calibration() and > have that function return -EINVAL if the check fails. > - In the plausibility check, change >= to ==. hid_field_extract() never > returns a negative value, so a scenario involving min > center or > center > max is impossible. > - To reduce code duplication, move the code for setting default > calibration values into a single function called > joycon_use_default_calibration(). > > Changes in v3: > - Unbreak warning string to conform to coding style. > - Change joycon_use_default_calibration() to accept a struct hid_device > pointer instead of a struct joycon_ctlr pointer. > > Changes in v4: > - Reformat joycon_use_default_calibration() another time to move stick > and ret onto the same line. > - Revert == to >= change to account for rollover. > - Additional minor change to comment for value check. > > drivers/hid/hid-nintendo.c | 55 +++++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 25 deletions(-) > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > index 5cb5a1d53b71..71e2a9a0882a 100644 > --- a/drivers/hid/hid-nintendo.c > +++ b/drivers/hid/hid-nintendo.c > @@ -760,12 +760,31 @@ static int joycon_read_stick_calibration(struct joycon_ctlr *ctlr, u16 cal_addr, > cal_y->max = cal_y->center + y_max_above; > cal_y->min = cal_y->center - y_min_below; > > - return 0; > + /* check if calibration values are plausible */ > + if (cal_x->min >= cal_x->center || cal_x->center >= cal_x->max || > + cal_y->min >= cal_y->center || cal_y->center >= cal_y->max) > + ret = -EINVAL; > + > + return ret; > } > > static const u16 DFLT_STICK_CAL_CEN = 2000; > static const u16 DFLT_STICK_CAL_MAX = 3500; > static const u16 DFLT_STICK_CAL_MIN = 500; > +static void joycon_use_default_calibration(struct hid_device *hdev, > + struct joycon_stick_cal *cal_x, > + struct joycon_stick_cal *cal_y, > + const char *stick, int ret) > +{ > + hid_warn(hdev, > + "Failed to read %s stick cal, using defaults; e=%d\n", > + stick, ret); > + > + cal_x->center = cal_y->center = DFLT_STICK_CAL_CEN; > + cal_x->max = cal_y->max = DFLT_STICK_CAL_MAX; > + cal_x->min = cal_y->min = DFLT_STICK_CAL_MIN; > +} > + > static int joycon_request_calibration(struct joycon_ctlr *ctlr) > { > u16 left_stick_addr = JC_CAL_FCT_DATA_LEFT_ADDR; > @@ -793,38 +812,24 @@ static int joycon_request_calibration(struct joycon_ctlr *ctlr) > &ctlr->left_stick_cal_x, > &ctlr->left_stick_cal_y, > true); > - if (ret) { > - hid_warn(ctlr->hdev, > - "Failed to read left stick cal, using dflts; e=%d\n", > - ret); > - > - ctlr->left_stick_cal_x.center = DFLT_STICK_CAL_CEN; > - ctlr->left_stick_cal_x.max = DFLT_STICK_CAL_MAX; > - ctlr->left_stick_cal_x.min = DFLT_STICK_CAL_MIN; > > - ctlr->left_stick_cal_y.center = DFLT_STICK_CAL_CEN; > - ctlr->left_stick_cal_y.max = DFLT_STICK_CAL_MAX; > - ctlr->left_stick_cal_y.min = DFLT_STICK_CAL_MIN; > - } > + if (ret) > + joycon_use_default_calibration(ctlr->hdev, > + &ctlr->left_stick_cal_x, > + &ctlr->left_stick_cal_y, > + "left", ret); > > /* read the right stick calibration data */ > ret = joycon_read_stick_calibration(ctlr, right_stick_addr, > &ctlr->right_stick_cal_x, > &ctlr->right_stick_cal_y, > false); > - if (ret) { > - hid_warn(ctlr->hdev, > - "Failed to read right stick cal, using dflts; e=%d\n", > - ret); > - > - ctlr->right_stick_cal_x.center = DFLT_STICK_CAL_CEN; > - ctlr->right_stick_cal_x.max = DFLT_STICK_CAL_MAX; > - ctlr->right_stick_cal_x.min = DFLT_STICK_CAL_MIN; > > - ctlr->right_stick_cal_y.center = DFLT_STICK_CAL_CEN; > - ctlr->right_stick_cal_y.max = DFLT_STICK_CAL_MAX; > - ctlr->right_stick_cal_y.min = DFLT_STICK_CAL_MIN; > - } > + if (ret) > + joycon_use_default_calibration(ctlr->hdev, > + &ctlr->right_stick_cal_x, > + &ctlr->right_stick_cal_y, > + "right", ret); > > hid_dbg(ctlr->hdev, "calibration:\n" > "l_x_c=%d l_x_max=%d l_x_min=%d\n" > -- > 2.37.3 > >
