Re: [PATCH v1 1/2] dmaengine: idxd: Fix race condition between WQ enable and reset paths

[Shuai Xue]

在 2025/6/3 22:32, Dave Jiang 写道:
> On 5/27/25 7:21 PM, Vinicius Costa Gomes wrote:
> > Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx> writes:
> >
> > > 在 2025/5/23 22:54, Dave Jiang 写道:
> > > > On 5/22/25 10:20 PM, Shuai Xue wrote:
> > > > > 在 2025/5/22 22:55, Dave Jiang 写道:
> > > > > > On 5/21/25 11:33 PM, Shuai Xue wrote:
> > > > > > > A device reset command disables all WQs in hardware. If issued while a WQ
> > > > > > > is being enabled, it can cause a mismatch between the software and hardware
> > > > > > > states.
> > > > > > >
> > > > > > > When a hardware error occurs, the IDXD driver calls idxd_device_reset() to
> > > > > > > send a reset command and clear the state (wq->state) of all WQs. It then
> > > > > > > uses wq_enable_map (a bitmask tracking enabled WQs) to re-enable them and
> > > > > > > ensure consistency between the software and hardware states.
> > > > > > >
> > > > > > > However, a race condition exists between the WQ enable path and the
> > > > > > > reset/recovery path. For example:
> > > > > > >
> > > > > > > A: WQ enable path                   B: Reset and recovery path
> > > > > > > ------------------                 ------------------------
> > > > > > > a1. issue IDXD_CMD_ENABLE_WQ
> > > > > > >                                       b1. issue IDXD_CMD_RESET_DEVICE
> > > > > > >                                       b2. clear wq->state
> > > > > > >                                       b3. check wq_enable_map bit, not set
> > > > > > > a2. set wq->state = IDXD_WQ_ENABLED
> > > > > > > a3. set wq_enable_map
> > > > > > >
> > > > > > > In this case, b1 issues a reset command that disables all WQs in hardware.
> > > > > > > Since b3 checks wq_enable_map before a2, it doesn't re-enable the WQ,
> > > > > > > leading to an inconsistency between wq->state (software) and the actual
> > > > > > > hardware state (IDXD_WQ_DISABLED).
> > > > > >
> > > > > >
> > > > > > Would it lessen the complication to just have wq enable path grab the device lock before proceeding?
> > > > > >
> > > > > > DJ
> > > > >
> > > > > Yep, how about add a spin lock to enable wq and reset device path.
> > > > >
> > > > > diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> > > > > index 38633ec5b60e..c0dc904b2a94 100644
> > > > > --- a/drivers/dma/idxd/device.c
> > > > > +++ b/drivers/dma/idxd/device.c
> > > > > @@ -203,6 +203,29 @@ int idxd_wq_enable(struct idxd_wq *wq)
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(idxd_wq_enable);
> > > > >    
> > > > > +/*
> > > > > + * This function enables a WQ in hareware and updates the driver maintained
> > > > > + * wq->state to IDXD_WQ_ENABLED. It should be called with the dev_lock held
> > > > > + * to prevent race conditions with IDXD_CMD_RESET_DEVICE, which could
> > > > > + * otherwise disable the WQ without the driver's state being properly
> > > > > + * updated.
> > > > > + *
> > > > > + * For IDXD_CMD_DISABLE_DEVICE, this function is safe because it is only
> > > > > + * called after the WQ has been explicitly disabled, so no concurrency
> > > > > + * issues arise.
> > > > > + */
> > > > > +int idxd_wq_enable_locked(struct idxd_wq *wq)
> > > > > +{
> > > > > +       struct idxd_device *idxd = wq->idxd;
> > > > > +       int ret;
> > > > > +
> > > > > +       spin_lock(&idxd->dev_lock);
> > > >
> > > > Let's start using the new cleanup macro going forward:
> > > > guard(spinlock)(&idxd->dev_lock);
> > > >
> > > > On a side note, there's been a cleanup on my mind WRT this driver's locking. I think we can replace idxd->dev_lock with idxd_confdev(idxd) device lock. You can end up just do:
> > > > guard(device)(idxd_confdev(idxd));
> > >
> > > Then we need to replace the lock from spinlock to mutex lock?
> >
> > We still need a (spin) lock that we could hold in interrupt contexts.
> >
> > > > And also drop the wq->wq_lock and replace with wq_confdev(wq) device lock:
> > > > guard(device)(wq_confdev(wq));
> > > >
> > > > If you are up for it that is.
> > >
> > > We creates a hierarchy: pdev -> idxd device -> wq device.
> > > idxd_confdev(idxd) is the parent of wq_confdev(wq) because:
> > >
> > >       (wq_confdev(wq))->parent = idxd_confdev(idxd);
> > >
> > > Is it safe to grap lock of idxd_confdev(idxd) under hold
> > > lock of wq_confdev(wq)?
> > >
> > > We have mounts of code use spinlock of idxd->dev_lock under
> > > hold of wq->wq_lock.
> >
> > I agree with Dave that the locking could be simplified, but I don't
> > think that we should hold this series because of that. That
> > simplification can be done later.
>
> I agree. Just passing musing on the current code.

Got it, do I need to send a separate patch for Patch 2?

Thanks.
Shuai