| Message ID | 20220823163709.2102468-1-jsnitsel@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 407171717a4f4d2d80825584643374a2dfdb0540 |
| Headers | show |
| Series | [v2] dmaengine: idxd: avoid deadlock in process_misc_interrupts() | expand |
On 8/23/2022 9:37 AM, Jerry Snitselaar wrote: > idxd_device_clear_state() now grabs the idxd->dev_lock > itself, so don't grab the lock prior to calling it. > > This was seen in testing after dmar fault occurred on system, > resulting in lockup stack traces. > > Cc: Fenghua Yu <fenghua.yu@intel.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Vinod Koul <vkoul@kernel.org> > Cc: dmaengine@vger.kernel.org > Fixes: cf4ac3fef338 ("dmaengine: idxd: fix lockdep warning on device driver removal") > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com> Thanks Jerry! Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > v2: add Fixes tag, and add subsystem to summary > > drivers/dma/idxd/irq.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c > index 743ead5ebc57..5b9921475be6 100644 > --- a/drivers/dma/idxd/irq.c > +++ b/drivers/dma/idxd/irq.c > @@ -324,13 +324,11 @@ static int process_misc_interrupts(struct idxd_device *idxd, u32 cause) > idxd->state = IDXD_DEV_HALTED; > idxd_wqs_quiesce(idxd); > idxd_wqs_unmap_portal(idxd); > - spin_lock(&idxd->dev_lock); > idxd_device_clear_state(idxd); > dev_err(&idxd->pdev->dev, > "idxd halted, need %s.\n", > gensts.reset_type == IDXD_DEVICE_RESET_FLR ? > "FLR" : "system reset"); > - spin_unlock(&idxd->dev_lock); > return -ENXIO; > } > }
On Tue, Aug 23, 2022 at 09:46:19AM -0700, Dave Jiang wrote: > > On 8/23/2022 9:37 AM, Jerry Snitselaar wrote: > > idxd_device_clear_state() now grabs the idxd->dev_lock > > itself, so don't grab the lock prior to calling it. > > > > This was seen in testing after dmar fault occurred on system, > > resulting in lockup stack traces. > > > > Cc: Fenghua Yu <fenghua.yu@intel.com> > > Cc: Dave Jiang <dave.jiang@intel.com> > > Cc: Vinod Koul <vkoul@kernel.org> > > Cc: dmaengine@vger.kernel.org > > Fixes: cf4ac3fef338 ("dmaengine: idxd: fix lockdep warning on device driver removal") > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com> > > Thanks Jerry! > > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > I noticed another problem while looking at this. When the device ends up in the halted state, and needs an flr or system reset, it calls idxd_wqs_unmap_portal(). Then if you do a modprobe -r idxd, you hit the WARN_ON in devm_iounmap(), because the remove code path calls idxd_wq_portal_unmap(), and wq->portal is null. I'm not sure if it just needs a simple sanity check in drv_disable_wq() to avoid the call in the case that it has already been unmapped, or if more cleanup needs to be done, and possibly a state to differentiate between halted + soft reset possible, versus halted + flr or system reset needed. You get multiple "Device is HALTED" messages during the removal as well. Regards, Jerry
On 8/23/2022 5:54 PM, Jerry Snitselaar wrote: > On Tue, Aug 23, 2022 at 09:46:19AM -0700, Dave Jiang wrote: >> On 8/23/2022 9:37 AM, Jerry Snitselaar wrote: >>> idxd_device_clear_state() now grabs the idxd->dev_lock >>> itself, so don't grab the lock prior to calling it. >>> >>> This was seen in testing after dmar fault occurred on system, >>> resulting in lockup stack traces. >>> >>> Cc: Fenghua Yu <fenghua.yu@intel.com> >>> Cc: Dave Jiang <dave.jiang@intel.com> >>> Cc: Vinod Koul <vkoul@kernel.org> >>> Cc: dmaengine@vger.kernel.org >>> Fixes: cf4ac3fef338 ("dmaengine: idxd: fix lockdep warning on device driver removal") >>> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com> >> Thanks Jerry! >> >> Reviewed-by: Dave Jiang <dave.jiang@intel.com> >> > I noticed another problem while looking at this. When the device ends > up in the halted state, and needs an flr or system reset, it calls > idxd_wqs_unmap_portal(). Then if you do a modprobe -r idxd, you hit > the WARN_ON in devm_iounmap(), because the remove code path calls > idxd_wq_portal_unmap(), and wq->portal is null. I'm not sure if it > just needs a simple sanity check in drv_disable_wq() to avoid the call > in the case that it has already been unmapped, or if more cleanup > needs to be done, and possibly a state to differentiate between > halted + soft reset possible, versus halted + flr or system reset > needed. You get multiple "Device is HALTED" messages during the > removal as well. Thanks! Fenghua, can you please take a look at this when you have a chance? Thank you! > > Regards, > Jerry >
On Wed, 2022-08-24 at 10:45 -0700, Dave Jiang wrote: > > On 8/23/2022 5:54 PM, Jerry Snitselaar wrote: > > On Tue, Aug 23, 2022 at 09:46:19AM -0700, Dave Jiang wrote: > > > On 8/23/2022 9:37 AM, Jerry Snitselaar wrote: > > > > idxd_device_clear_state() now grabs the idxd->dev_lock > > > > itself, so don't grab the lock prior to calling it. > > > > > > > > This was seen in testing after dmar fault occurred on system, > > > > resulting in lockup stack traces. > > > > > > > > Cc: Fenghua Yu <fenghua.yu@intel.com> > > > > Cc: Dave Jiang <dave.jiang@intel.com> > > > > Cc: Vinod Koul <vkoul@kernel.org> > > > > Cc: dmaengine@vger.kernel.org > > > > Fixes: cf4ac3fef338 ("dmaengine: idxd: fix lockdep warning on > > > > device driver removal") > > > > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > Thanks Jerry! > > > > > > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > > > > > I noticed another problem while looking at this. When the device > > ends > > up in the halted state, and needs an flr or system reset, it calls > > idxd_wqs_unmap_portal(). Then if you do a modprobe -r idxd, you hit > > the WARN_ON in devm_iounmap(), because the remove code path calls > > idxd_wq_portal_unmap(), and wq->portal is null. I'm not sure if it > > just needs a simple sanity check in drv_disable_wq() to avoid the > > call > > in the case that it has already been unmapped, or if more cleanup > > needs to be done, and possibly a state to differentiate between > > halted + soft reset possible, versus halted + flr or system reset > > needed. You get multiple "Device is HALTED" messages during the > > removal as well. > > Thanks! > > Fenghua, can you please take a look at this when you have a chance? > Thank you! > > Fenghua, I see another potential issue. If a software reset is attempted idxd_device_reinit() will be called which walks the wqs, and if a wq has the state IDXD_WQ_ENABLED it calls idxd_wq_enable(), but the first thing idxd_wq_enable() does is see that the state is IDXD_WQ_ENABLED and returns 0. Without the wq enable command being sent, it will not be re-enabled, yes? Regards, Jerry > > > > Regards, > > Jerry > > >
Hi, Jerry, > I see another potential issue. If a software reset is attempted idxd_device_reinit() > will be called which walks the wqs, and if a wq has the state > IDXD_WQ_ENABLED it calls idxd_wq_enable(), but the first thing > idxd_wq_enable() does is see that the state is IDXD_WQ_ENABLED and returns 0. > Without the wq enable command being sent, it will not be re-enabled, yes? Could you please describe how to reproduce the issues and test case? Thanks. -Fenghua
On Wed, 2022-08-24 at 21:11 +0000, Yu, Fenghua wrote: > Hi, Jerry, > > > I see another potential issue. If a software reset is attempted > > idxd_device_reinit() > > will be called which walks the wqs, and if a wq has the state > > IDXD_WQ_ENABLED it calls idxd_wq_enable(), but the first thing > > idxd_wq_enable() does is see that the state is IDXD_WQ_ENABLED and > > returns 0. > > Without the wq enable command being sent, it will not be re- > > enabled, yes? > > Could you please describe how to reproduce the issues and test case? Nothing special, just running dsa_user_test_runner.sh from idxd-config (3.4.6.3) with intel_iommu=on,sm_on (default is lazy dma domain) on an Intel supplied SPR system. During the run there is a dmar fault, halts the device with it needing an flr reset. That is where the deadlock was noticed. Then after the dsa_user_runner.sh fails, if you do modprobe -r idxd, you hit the warning in devm_iounmap(). The idxd_device_reinit() case I haven't reproduced, I'm just looking at the code. I should note that I've only run into an issue with that model of system. The other models of SPR systems don't hit an issue. This is with a 6.0-rc1 kernel, and our kernel. I'm guessing there is an issue with that model of system/fw. I've seen both fault reasons 0x50 and 0x58 when it faults. Regards, Jerry > > Thanks. > > -Fenghua
On 23-08-22, 09:37, Jerry Snitselaar wrote: > idxd_device_clear_state() now grabs the idxd->dev_lock > itself, so don't grab the lock prior to calling it. > > This was seen in testing after dmar fault occurred on system, > resulting in lockup stack traces. Applied, thanks
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c index 743ead5ebc57..5b9921475be6 100644 --- a/drivers/dma/idxd/irq.c +++ b/drivers/dma/idxd/irq.c @@ -324,13 +324,11 @@ static int process_misc_interrupts(struct idxd_device *idxd, u32 cause) idxd->state = IDXD_DEV_HALTED; idxd_wqs_quiesce(idxd); idxd_wqs_unmap_portal(idxd); - spin_lock(&idxd->dev_lock); idxd_device_clear_state(idxd); dev_err(&idxd->pdev->dev, "idxd halted, need %s.\n", gensts.reset_type == IDXD_DEVICE_RESET_FLR ? "FLR" : "system reset"); - spin_unlock(&idxd->dev_lock); return -ENXIO; } }
idxd_device_clear_state() now grabs the idxd->dev_lock itself, so don't grab the lock prior to calling it. This was seen in testing after dmar fault occurred on system, resulting in lockup stack traces. Cc: Fenghua Yu <fenghua.yu@intel.com> Cc: Dave Jiang <dave.jiang@intel.com> Cc: Vinod Koul <vkoul@kernel.org> Cc: dmaengine@vger.kernel.org Fixes: cf4ac3fef338 ("dmaengine: idxd: fix lockdep warning on device driver removal") Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com> --- v2: add Fixes tag, and add subsystem to summary drivers/dma/idxd/irq.c | 2 -- 1 file changed, 2 deletions(-)