On Friday, 22 May 2026 19:38:24 Central European Summer Time Daniel Stone wrote: > Hi Nicolas, > > On Wed, 20 May 2026 at 14:36, Nicolas Frattaroli > <[email protected]> wrote: > > - for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) { > > + for (i = 0; i <= ERR_DET_OFF(SCDC_ERR_DET_2_H); i += 2) { > > if (buf[i + 1] & SCDC_CHANNEL_VALID) > > counter[i / 2] = buf[i] | (buf[i + 1] & > > ~SCDC_CHANNEL_VALID) << 8; > > else > > @@ -355,9 +433,15 @@ int drm_scdc_read_error_counters(struct drm_connector > > *connector, u16 counter[3] > > buf[i] = 0; > > buf[i + 1] = 0; > > } > > - buf[ARRAY_SIZE(buf) - 1] = 0; > > + buf[ERR_DET_OFF(SCDC_ERR_DET_CHECKSUM)] = 0; > > + > > + if (num_lanes == 4) > > + counter[3] = buf[ERR_DET_OFF(SCDC_ERR_DET_3_L)] | > > + (buf[ERR_DET_OFF(SCDC_ERR_DET_3_H)] & > > ~SCDC_CHANNEL_VALID) << 8; > > + else > > + counter[3] = 0; > > I get that this is separate from the loop above because it's > discontiguous, but is it also missing the validity check? I mean, > lane_count == 3 means 'fsvo 3 which may be 1 or 2' so have to check > SCDC_CHANNEL_VALID there, but if we have 4 lanes explicitly specified > then we are we missing the check for the valid bit, or is it just > always valid / we shouldn't check? > > tbh having it separately is a bit messy. I half-wonder if unrolling > the loop wouldn't be cleaner, e.g.: > #define GET_SCDC_ERR_CNT(c) { \ > bool valid = (buf[(c) * 2 + 1] & SCDC_CHANNEL_VALID; \ > if (valid) { \ > counter[c] = buf[ERR_DET_OFF(SCDC_ERR_DET_ ##x _L)]; \ > counter[c] |= (buf[ERR_DET_OFF(SCDC_ERR_DET_ ##x _H)] > & ~SCDC_CHANNEL_VALID) << 8; \ > } else { > counter[c] = 0; > } \ > } > > GET_SCDC_ERR_CNT(0); > GET_SCDC_ERR_CNT(1); > GET_SCDC_ERR_CNT(2); > if (num_lanes == 4) > GET_SCDC_ERR_CNT(3); > else > counter[3] = 0; > > Up to you though. It's a messy format, with no objectively great solution.
Yeah, I think you found an actual bug here. Maybe iterating once over i = 0 to buf_sz is the better choice here anyway. I'm also realising now that for lane 4 I'm not writing 0 to the fields after being done with them, so that's not right either. Thanks for the review, will send v3 in short order. Kind regards, Nicolas Frattaroli > > With or without that suggestion, series is: > Reviewed-by: Daniel Stone <[email protected]> > > Thanks for pulling all this together! > > Cheers, > Daniel >
