Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] guc_ct_send_recv() still checks xe_device_wedged() instead of xe_device_io_blocked() when returning cancellation errors, causing state inconsistency. Pre-existing issues: - [High] Directly calling probe during AER slot reset bypasses driver core error handling, leading to double-remove and crashes if it fails. -- commit 1cbbd9c3fc1b9bad858f6195cfff20e523edc2ca Author: Arvind Yadav <[email protected]> drm/xe: Separate AER reset state from device wedging This commit modifies the PCI error recovery path (AER) to use a temporary in_reset state instead of the permanent wedged flag to block driver I/O. It introduces the xe_device_io_blocked() helper to check both conditions, replacing xe_device_wedged() in multiple places where temporary blocks should also apply. > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c > index 5c4733da385c5..3c3fe4928fa20 100644 > --- a/drivers/gpu/drm/xe/xe_guc_ct.c > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c > @@ -1062,7 +1062,7 @@ static int __guc_ct_send_locked(struct xe_guc_ct *ct, > const u32 *action, > xe_gt_assert(gt, g2h_len || !num_g2h); > lockdep_assert_held(&ct->lock); > > - if (xe_device_wedged(ct_to_xe(ct))) { > + if (xe_device_io_blocked(ct_to_xe(ct))) { > ret = -ENOTRECOVERABLE; > goto out; > } [ ... ] [Severity: Medium] In guc_ct_send_recv(), the code still checks the old xe_device_wedged() state when returning cancellation errors: if (g2h_fence.fail) { if (g2h_fence.cancel) { xe_gt_dbg(gt, "H2G request %#x canceled!\n", action[0]); ret = xe_device_wedged(ct_to_xe(ct)) ? -ENOTRECOVERABLE : -ECANCELED; goto unlock; } Does this leave a state inconsistency during a PCI AER reset? If an in-flight H2G request is cancelled because of a temporary IO block, xe_device_wedged() will evaluate to false. This means the caller receives -ECANCELED instead of -ENOTRECOVERABLE. Should this check be updated to use xe_device_io_blocked() as well? [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_pci_error.c > b/drivers/gpu/drm/xe/xe_pci_error.c > index 79ce0c671549a..d82256d8721f3 100644 > --- a/drivers/gpu/drm/xe/xe_pci_error.c > +++ b/drivers/gpu/drm/xe/xe_pci_error.c [ ... ] > @@ -116,7 +116,6 @@ static pci_ers_result_t xe_pci_error_slot_reset(struct > pci_dev *pdev) > * TODO: optimize by re-initializing only the hardware state and > re-creating > * kernel BOs. > */ > - xe_device_clear_in_reset(xe); > pdev->driver->remove(pdev); > devres_release_group(&pdev->dev, xe->devres_group); > [Severity: High] This is a pre-existing issue, but does manually calling probe here bypass driver core error handling? If pdev->driver->probe(pdev, ent) fails during xe_pci_error_slot_reset(), it returns PCI_ERS_RESULT_DISCONNECT. The PCI AER core will then unbind the driver and call pdev->driver->remove(pdev). Since the failing probe likely already called xe_device_remove(xe) on its error path, wouldn't this lead to a double remove and potential crashes? > @@ -125,8 +124,8 @@ static pci_ers_result_t xe_pci_error_slot_reset(struct > pci_dev *pdev) > > xe = pdev_to_xe_device(pdev); > > - /* Wedge the device to prevent I/O operations till the resume callback > */ > - atomic_set(&xe->wedged.flag, 1); > + /* Block the new instance until the resume callback. */ > + xe_device_set_in_reset(xe); > > return PCI_ERS_RESULT_RECOVERED; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
