On Tue, 05 Dec, 2023 18:15:51 -0300 "Guilherme G. Piccoli" <gpiccoli@xxxxxxxxxx> wrote: > It was reported [0] that adding a generic joycon to the system caused > a kernel crash on Steam Deck, with the below panic spew: > > divide error: 0000 [#1] PREEMPT SMP NOPTI > [...] > Hardware name: Valve Jupiter/Jupiter, BIOS F7A0119 10/24/2023 > RIP: 0010:nintendo_hid_event+0x340/0xcc1 [hid_nintendo] > [...] > Call Trace: > [...] > ? exc_divide_error+0x38/0x50 > ? nintendo_hid_event+0x340/0xcc1 [hid_nintendo] > ? asm_exc_divide_error+0x1a/0x20 > ? nintendo_hid_event+0x307/0xcc1 [hid_nintendo] > hid_input_report+0x143/0x160 > hidp_session_run+0x1ce/0x700 [hidp] > > Since it's a divide-by-0 error, by tracking the code for potential > denominator issues, we've spotted 2 places in which this could happen; > so let's guard against the possibility and log in the kernel if the > condition happens. This is specially useful since some data that > fills some denominators are read from the joycon HW in some cases, > increasing the potential for flaws. > > [0] https://github.com/ValveSoftware/SteamOS/issues/1070 > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@xxxxxxxxxx> > --- > drivers/hid/hid-nintendo.c | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > index 138f154fecef..23f3f96c8c85 100644 > --- a/drivers/hid/hid-nintendo.c > +++ b/drivers/hid/hid-nintendo.c [...] > @@ -1163,16 +1176,16 @@ static void joycon_parse_imu_report(struct joycon_ctlr *ctlr, > JC_IMU_SAMPLES_PER_DELTA_AVG) { > ctlr->imu_avg_delta_ms = ctlr->imu_delta_samples_sum / > ctlr->imu_delta_samples_count; > - /* don't ever want divide by zero shenanigans */ > - if (ctlr->imu_avg_delta_ms == 0) { > - ctlr->imu_avg_delta_ms = 1; > - hid_warn(ctlr->hdev, > - "calculated avg imu delta of 0\n"); > - } > ctlr->imu_delta_samples_count = 0; > ctlr->imu_delta_samples_sum = 0; > } > > + /* don't ever want divide by zero shenanigans */ > + if (ctlr->imu_avg_delta_ms == 0) { > + ctlr->imu_avg_delta_ms = 1; > + hid_warn(ctlr->hdev, "calculated avg imu delta of 0\n"); > + } > + Hi Guilherme, I agree with the previous hunks you added and can see how those could trigger the divide-by-zero issue you were seeing. However, I am not sure if this hunk that I have left makes sense. Reason being, is that the hid-nintendo driver has a special conditional to prevent divide-by-zero in this case without this change. 1. If the first packet has not been received by the IMU, set imu_avg_delta_ms to JC_IMU_DFLT_AVG_DELTA_MS. 2. Only change imu_avg_delta_ms when imu_delta_samples_count >= JC_IMU_SAMPLES_PER_DELTA_AVG. 3. If that change leads to imu_avg_delta_ms being 0, set it to 1. With this logic as-is, I do not see how a divide by zero could have occurred in this specific path without your change. I might be missing something, but I wanted to make sure to avoid integrating a hunk that does not help. Would it be possible to retest without this hunk? > /* useful for debugging IMU sample rate */ > hid_dbg(ctlr->hdev, > "imu_report: ms=%u last_ms=%u delta=%u avg_delta=%u\n", -- Thanks, Rahul Rameshbabu
