Messages in this thread | | | Date | Mon, 19 Sep 2022 08:28:04 -0700 | Subject | Re: [PATCH] dmaengine: idxd: Set workqueue state to disabled before trying to re-enable | From | Dave Jiang <> |
| |
On 9/17/2022 10:05 AM, Jerry Snitselaar wrote: > On Wed, Aug 24, 2022 at 03:19:51PM -0700, Dave Jiang wrote: >> On 8/24/2022 3:07 PM, Jerry Snitselaar wrote: >>> On Wed, 2022-08-24 at 14:59 -0700, Dave Jiang wrote: >>>> On 8/24/2022 2:16 PM, Jerry Snitselaar wrote: >>>>> On Wed, Aug 24, 2022 at 01:29:03PM -0700, Dave Jiang wrote: >>>>>> On 8/24/2022 12:29 PM, Jerry Snitselaar wrote: >>>>>>> For a software reset idxd_device_reinit() is called, which will >>>>>>> walk >>>>>>> the device workqueues to see which ones were enabled, and try >>>>>>> to >>>>>>> re-enable them. It keys off wq->state being iDXD_WQ_ENABLED, >>>>>>> but the >>>>>>> first thing idxd_enable_wq() will do is see that the state of >>>>>>> the >>>>>>> workqueue is enabled, and return 0 instead of attempting to >>>>>>> issue >>>>>>> a command to enable the workqueue. >>>>>>> >>>>>>> So once a workqueue is found that needs to be re-enabled, >>>>>>> set the state to disabled prior to calling idxd_enable_wq(). >>>>>>> This would accurately reflect the state if the enable fails >>>>>>> as well. >>>>>>> >>>>>>> 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: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel >>>>>>> data accelerators") >>>>>>> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com> >>>>>>> --- >>>>>>> drivers/dma/idxd/irq.c | 1 + >>>>>>> 1 file changed, 1 insertion(+) >>>>>>> >>>>>>> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c >>>>>>> index 743ead5ebc57..723eeb5328d6 100644 >>>>>>> --- a/drivers/dma/idxd/irq.c >>>>>>> +++ b/drivers/dma/idxd/irq.c >>>>>>> @@ -52,6 +52,7 @@ static void idxd_device_reinit(struct >>>>>>> work_struct *work) >>>>>>> struct idxd_wq *wq = idxd->wqs[i]; >>>>>>> if (wq->state == IDXD_WQ_ENABLED) { >>>>>>> + wq->state = IDXD_WQ_DISABLED; >>>>>> Might be better off to insert this line in >>>>>> idxd_wq_disable_cleanup(). I >>>>>> think that should put it in sane state. >>>>> I don't think that is called in the code path that I was lookng at. >>>>> I've been >>>>> looking at this bit of process_misc_interrupts(): >>>>> >>>>> halt: >>>>> gensts.bits = ioread32(idxd->reg_base + >>>>> IDXD_GENSTATS_OFFSET); >>>>> if (gensts.state == IDXD_DEVICE_STATE_HALT) { >>>>> idxd->state = IDXD_DEV_HALTED; >>>>> if (gensts.reset_type == >>>>> IDXD_DEVICE_RESET_SOFTWARE) { >>>>> /* >>>>> * If we need a software reset, we will >>>>> throw the work >>>>> * on a system workqueue in order to allow >>>>> interrupts >>>>> * for the device command completions. >>>>> */ >>>>> INIT_WORK(&idxd->work, idxd_device_reinit); >>>>> queue_work(idxd->wq, &idxd->work); >>>>> } else { >>>>> 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; >>>>> } >>>>> } >>>>> >>>>> return 0; >>>>> } >>>>> >>>>> So it sees that the device is halted, and sticks >>>>> idxd_device_reinint() on that >>>>> workqueue. The idxd_device_reinit() has this loop to re-enable the >>>>> idxd wqs: >>>> idxd_device_reinit() should called idxd_device_reset() first. And >>>> that >>>> should at some point call idxd_wq_disable_cleanup() and clean up the >>>> states. >>>> >>>> https://elixir.bootlin.com/linux/v6.0-rc2/source/drivers/dma/idxd/irq.c#L42 >>>> >>>> https://elixir.bootlin.com/linux/v6.0-rc2/source/drivers/dma/idxd/device.c#L725 >>>> >>>> https://elixir.bootlin.com/linux/v6.0-rc2/source/drivers/dma/idxd/device.c#L711 >>>> >>>> https://elixir.bootlin.com/linux/v6.0-rc2/source/drivers/dma/idxd/device.c#L376 >>>> >>>> So if we stick the wq state reset in there, it should show up as >>>> "disabled" by the time we try to enable the WQs again. Does that look >>>> reasonable? >>>> >>> Ah, yeah I see that now. So, if it does set the state to disabled in >>> idxd_wq_disable_cleanup(), does it have another means to track which >>> wqs need to be re-enabled for that loop that happens after the >>> idxd_device_reset() call? >> Oh I see what you mean... So we can either do what you did or create a mask >> and mark the WQ that are "enabled" before reset. Maybe that's cleaner rather >> than relying on the side effect of the WQ state isn't cleared? Thoughts? >> > Circling back to this. Since max_wqs could theoretically go up to 2^8, I guess > this would need to be done with the bitmap_* functions?
Hi Jerry,
I wouldn't say never but I doubt any time soon for 2^8. DSA 1.0 has 8 WQs, and 2.0 (spec just went public) has 16. But yes we can use bitmap to be future proof. Are you currently working on a fix for this? Just don't want to duplicate effort if you already have something going. Thank you!
> > Regards, > Jerry > >>>>> for (i = 0; i < idxd->max_wqs; i++) { >>>>> struct idxd_wq *wq = idxd->wqs[i]; >>>>> >>>>> if (wq->state == IDXD_WQ_ENABLED) { >>>>> wq->state = IDXD_WQ_DISABLED; >>>>> rc = idxd_wq_enable(wq); >>>>> if (rc < 0) { >>>>> dev_warn(dev, "Unable to re-enable >>>>> wq %s\n", >>>>> dev_name(wq_confdev(wq))); >>>>> } >>>>> } >>>>> } >>>>> >>>>> Once you go into idxd_wq_enable() though you get this check at the >>>>> beginning: >>>>> >>>>> if (wq->state == IDXD_WQ_ENABLED) { >>>>> dev_dbg(dev, "WQ %d already enabled\n", wq->id); >>>>> return 0; >>>>> } >>>>> >>>>> So IIUC it sees the device is halted, goes to reset it, figures out >>>>> a wq >>>>> should be re-enabled, calls idxd_wq_enable() which hits the check, >>>>> returns >>>>> 0 and the wq is never really re-enabled, though it will still have >>>>> wq state >>>>> set to IDXD_WQ_ENABLED. >>>>> >>>>> Or am I missing something? >>>>> >>>>> Regards, >>>>> Jerry >>>>> >>>>>>> rc = idxd_wq_enable(wq); >>>>>>> if (rc < 0) { >>>>>>> dev_warn(dev, "Unable to re- >>>>>>> enable wq %s\n",
| |