Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Incorrect maximum delay in usleep_range() introduces massive timer slack, defeating exponential backoff and causing extreme latency and spurious timeouts. - [Medium] The commit message claims to simplify a `sleep_min` assignment, but no such change exists in the code. - [Low] 64-bit division on 32-bit architectures causes link errors. --
commit 76cc600b7d97fbc1edcd9a133df8168f9ad41977 Author: Alan Previn <[email protected]> drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums This patch adds overflow checks for delays in __xe_mmio_wait32() and introduces caps to the maximum in-loop wait time. It also aims to pick the correct sleep or delay helper based on the requested wait duration. > drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums > > Check for overflow in udelay/usleep_range use in __xe_mmio_wait32 > and pick the correct helper accordingly. Fix sign-overflow and avoid > growing delays becoming intollerably large by capping the in-loop > wait time. Simplify sleep_min assignment by removing redundant cast. [Severity: Medium] The commit message claims to simplify the sleep_min assignment by removing a redundant cast, but the variable sleep_min does not exist in the modified code. Could the commit message be updated to reflect the actual changes? > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c > index 7fa18dfcb5a28..cf2e05aaa4b2f 100644 > --- a/drivers/gpu/drm/xe/xe_mmio.c > +++ b/drivers/gpu/drm/xe/xe_mmio.c [ ... ] > @@ -349,11 +349,23 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, > struct xe_reg reg, u32 mask, u > if (ktime_after(ktime_add_us(cur, wait), end)) > wait = ktime_us_delta(end, cur); > > - if (atomic) > - udelay(wait); > - else > - usleep_range(wait, wait << 1); > - wait <<= 1; > +#define __XE_MMIO_WAIT_MAX_INLOOP_TIME (100 * USEC_PER_MSEC) > + if (atomic) { > + if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC) > + udelay(wait); > + else > + mdelay(DIV_ROUND_UP(wait, USEC_PER_MSEC)); [Severity: Low] Wait is defined as an s64 type. Does using DIV_ROUND_UP(wait, USEC_PER_MSEC) here cause a 64-bit division? On 32-bit architectures, this could lead to an undefined reference to __divdi3 and break the build. > + } else { > + usleep_range(wait, __XE_MMIO_WAIT_MAX_INLOOP_TIME); [Severity: High] Does hardcoding __XE_MMIO_WAIT_MAX_INLOOP_TIME (100ms) as the maximum bound for usleep_range() introduce massive timer slack? For example, on the first iteration where wait is 10us, this allows the scheduler to delay the wakeup for up to 100ms, completely defeating the intended exponential backoff. This could cause massive latency spikes for fast MMIO operations in non-atomic calls to __xe_mmio_wait32(). > + } > + /* > + * As we keep doubling the wait time for every check that > fails, cap the > + * in-loop delay-or-sleep to less than 2x 100 miliseconds to > prevent from > + * expanding 'wait' into exponentially longer wait times per > loop that > + * end up delaying the next completion check way later than > tolerable. > + */ > + wait = wait < __XE_MMIO_WAIT_MAX_INLOOP_TIME >> 1 ? > + wait << 1 : __XE_MMIO_WAIT_MAX_INLOOP_TIME; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
