On Wed, Sep 21, 2022 at 12:53 PM Johnothan King <johnothanking@xxxxxxxxxxxxxx> wrote: > > For my v4 patch I'll just change the check back to using >=. The > signedness of the min, max and center values will probably need some > sort of fix, but that's out of the scope of this patch. Yeah, no worries. The signedness issue was more a FYI, in case you ever need signed integers from that function. I hope we won't have the case here and that we are actually having unsigned integers in the data. Cheers, Benjamin > > - Johnothan King > > ------- Original Message ------- > On Wednesday, September 21st, 2022 at 1:34 AM, Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote: > > > > Hi Johnothan, > > > > On Sep 21 2022, Johnothan King 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 > > > > > > 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. > > > > > > I am not so sure this is a great idea. I agree this is correct, but it > > definitely requires some processing from my brain and double > > verifications in the code that this is correct. > > > > The problem is that all of the values are declared as s32. > > hid_field_extract() returns a u32, yes, but I haven't checked the report > > descriptor if that value can be a negative one. What needs to be done, > > if the logical min value is negative is that we should call hid_snto32() > > to convert into a proper s32 (I doubt you have to do it but I am putting > > it here for completeness). > > > > So basically, you are blindly converting a u32 into a s32 and do not > > take rollover into account. > > > > Given that this function is only called at probe time where timing is > > not the biggest of our concerns, I would simply leave the more human > > friendy with obvious failures cases with >= and <=. > > > > > > > > Second note: please move all "Changes in v*" below the first '---' and > > before the file stats. This way they will be stripped out when applying > > the patch. People who want to see the changes can always follow the lore > > link that should be applied to the commit when this patch gets applied. > > > > > - 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. > > > > > > 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 | 55 +++++++++++++++++++++----------------- > > > 1 file changed, 30 insertions(+), 25 deletions(-) > > > > > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > > > index 6028af3c3aae..f25b7b19e9a4 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 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); > > > > > > nitpick: why not putting the format string on the line above and leave > > "stick" and "ret in the second line? It should be OK for checkpatch and > > will be less weird to have "ret" on its line all by itself. > > > > > + > > > + 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 > > > > > > Cheers, > > Benjamin >
