Messages in this thread Patch in this message | | | Date | Mon, 2 Mar 2015 13:36:01 +0000 | From | One Thousand Gnomes <> | Subject | Re: drm/gma500: Possible deadlock in gma_power_begin() |
| |
On Sat, 28 Feb 2015 18:02:33 +0300 Alexey Khoroshilov <khoroshilov@ispras.ru> wrote:
> gma_power_begin() starts with locking power_ctrl_lock spinlock and then, > if gma_resume_pci(dev->pdev) succeed, it calls > psb_irq_preinstall(dev); > psb_irq_postinstall(dev); > > psb_irq_postinstall() does some pipestat enabling/disabling dance: > if (dev->vblank[0].enabled) > psb_enable_pipestat(dev_priv, 0, PIPE_VBLANK_INTERRUPT_ENABLE); > else > psb_disable_pipestat(dev_priv, 0, PIPE_VBLANK_INTERRUPT_ENABLE); > > where > void psb_enable_pipestat(struct drm_psb_private *dev_priv, int pipe, u32 > mask) > { > if ((dev_priv->pipestat[pipe] & mask) != mask) { > u32 reg = psb_pipestat(pipe); > dev_priv->pipestat[pipe] |= mask; > /* Enable the interrupt, clear any pending status */ > if (gma_power_begin(dev_priv->dev, false)) { > u32 writeVal = PSB_RVDC32(reg); > writeVal |= (mask | (mask >> 16)); > PSB_WVDC32(writeVal, reg); > (void) PSB_RVDC32(reg); > gma_power_end(dev_priv->dev); > } > } > } > > So, if a flag in dev_priv->pipestat[pipe] is not in agreement with > dev->vblank[0].enabled, > we will have a call to gma_power_begin() again and got an unavoidable > deadlock.
I don't think it can ever happen in the current driver but the fix also looks trivial. I think its sufficient to do this
commit 76d209fc77516f638c6db0342fddd0930d8e5957 Author: Alan <gnomes@lxorguk.ukuu.org.uk> Date: Mon Mar 2 13:32:46 2015 +0000
gma500: drop the power lock before doing the irq pre/postinstall The irq install is not locked or managed by the power lock. It will take the lock again internally to do the vblank update so we would deadlock if a change was ever needed.
diff --git a/drivers/gpu/drm/gma500/power.c b/drivers/gpu/drm/gma500/power.c index b6b135f..8d101f4 100644 --- a/drivers/gpu/drm/gma500/power.c +++ b/drivers/gpu/drm/gma500/power.c @@ -266,11 +266,11 @@ bool gma_power_begin(struct drm_device *dev, bool force_on) /* Ok power up needed */ ret = gma_resume_pci(dev->pdev); if (ret == 0) { - psb_irq_preinstall(dev); - psb_irq_postinstall(dev); pm_runtime_get(&dev->pdev->dev); dev_priv->display_count++; spin_unlock_irqrestore(&power_ctrl_lock, flags); + psb_irq_preinstall(dev); + psb_irq_postinstall(dev); return true; } out_false:
| |