| Message ID | yawjxnu62q6obpz6yy2ksrrwfprafdq5b23lktvcu2oiydbmgt@oag7ts74jxu4 (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "drm/i915/gt: Log reason for setting TAINT_WARN at reset" | expand |
Hi Sebastian, On 2025-01-23 at 11:33:00 GMT, Sebastian Brzezinka wrote: > This reverts commit 835443da6f50d9516b58bba5a4fdf9e563d961c7. > > Kernel taint information is present in dmesg already, and in > the case of an unrecoverable error, the CI restarts the device > accordingly. Raising an error causes intentional error injection > to report an undesired error notification. > > Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com> I'd rephrase the last sentence of the commit log a bit. It's a little unclear why the patch should be reverted, e.g.: - turns out that logging with gt_err() causes CI to pick up an error even in intentional error injects, - the unintentional (real) errors are already reported correctly by CI, - a gt wedge is already being logged without this patch, so we should revert the new message instead of, for example, relaxing the loglevel. I obviously have the context on this so I know exactly what's happening, but the maintainers might not be as familiar with the issue, so going into more detail in the patch log would be good. Thanks Krzysztof
On Thu Jan 23, 2025 at 1:54 PM UTC, Krzysztof Niemiec wrote: > Hi Sebastian, > > On 2025-01-23 at 11:33:00 GMT, Sebastian Brzezinka wrote: >> This reverts commit 835443da6f50d9516b58bba5a4fdf9e563d961c7. >> >> Kernel taint information is present in dmesg already, and in >> the case of an unrecoverable error, the CI restarts the device >> accordingly. Raising an error causes intentional error injection >> to report an undesired error notification. >> >> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com> > > I'd rephrase the last sentence of the commit log a bit. It's a little > unclear why the patch should be reverted, e.g.: > > - turns out that logging with gt_err() causes CI to pick up an error > even in intentional error injects, > - the unintentional (real) errors are already reported correctly by CI, > - a gt wedge is already being logged without this patch, so we should > revert the new message instead of, for example, relaxing the loglevel. > > I obviously have the context on this so I know exactly what's happening, > but the maintainers might not be as familiar with the issue, so going > into more detail in the patch log would be good. > > Thanks > Krzysztof Thank you for the review, I sent a second version with a corrected commit message.
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c index aae5a081cb53..c2fe3fc78e76 100644 --- a/drivers/gpu/drm/i915/gt/intel_reset.c +++ b/drivers/gpu/drm/i915/gt/intel_reset.c @@ -1113,7 +1113,6 @@ static bool __intel_gt_unset_wedged(struct intel_gt *gt) * Warn CI about the unrecoverable wedged condition. * Time for a reboot. */ - gt_err(gt, "Unrecoverable wedged condition\n"); add_taint_for_CI(gt->i915, TAINT_WARN); return false; } @@ -1265,10 +1264,8 @@ void intel_gt_reset(struct intel_gt *gt, } ret = resume(gt); - if (ret) { - gt_err(gt, "Failed to resume (%d)\n", ret); + if (ret) goto taint; - } finish: reset_finish(gt, awake); @@ -1611,7 +1608,6 @@ void intel_gt_set_wedged_on_init(struct intel_gt *gt) set_bit(I915_WEDGED_ON_INIT, >->reset.flags); /* Wedged on init is non-recoverable */ - gt_err(gt, "Non-recoverable wedged on init\n"); add_taint_for_CI(gt->i915, TAINT_WARN); }
This reverts commit 835443da6f50d9516b58bba5a4fdf9e563d961c7. Kernel taint information is present in dmesg already, and in the case of an unrecoverable error, the CI restarts the device accordingly. Raising an error causes intentional error injection to report an undesired error notification. Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com> --- drivers/gpu/drm/i915/gt/intel_reset.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)