On Thu, Jul 06, 2017 at 09:24:50PM +0200, Hans de Goede wrote: > intel_uncore_forcewake_reset() does forcewake puts and gets as such > we need to make sure that no-one tries to access the PUNIT->PMIC bus > (on systems where this bus is shared) while it runs, otherwise bad > things happen. > > Normally this is taken care of by the i915_pmic_bus_access_notifier() > which does an intel_uncore_forcewake_get(FORCEWAKE_ALL) when some other > driver tries to access the PMIC bus, so that later forcewake gets are > no-ops (for the duration of the bus access). > > But intel_uncore_forcewake_reset gets called in 3 cases: > 1) Before registering the pmic_bus_access_notifier > 2) After unregistering the pmic_bus_access_notifier > 3) To reset forcewake state on a GPU reset > > In all 3 cases the i915_pmic_bus_access_notifier() protection is > insufficient. This commit fixes the pmic bus access race this causes > by making intel_uncore_forcewake_reset() call iosf_mbi_punit_acquire() > (and iosf_mbi_punit_release() when done). > > Note that iosf_mbi_punit_acquire() locks a mutex and thus > intel_uncore_forcewake_reset() may sleep after this commit. I've checked > all callers and they all already take other mutexes, so this is not a > problem. > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > --- > Changes in v2: > -Rebase on current (July 6th 2017) drm-next > --- > drivers/gpu/drm/i915/intel_uncore.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 4a547cdfafa9..f9441c9ae226 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -237,6 +237,9 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, > int retry_count = 100; > enum forcewake_domains fw, active_domains; > > + /* Acquire the PUNIT->PMIC bus before modifying forcewake settings */ > + iosf_mbi_punit_acquire(); > + > /* Hold uncore.lock across reset to prevent any register access > * with forcewake not set correctly. Wait until all pending > * timers are run before holding. > @@ -294,6 +297,7 @@ static void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv, > assert_forcewakes_inactive(dev_priv); > > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > + iosf_mbi_punit_release(); > } Looks ok in general, but during intel_uncore_suspend() and intel_uncore_fini() this still allows the following race: 1. (task 1) i915 MMIO access requiring forcewake, arms intel_uncore_fw_release_timer(). 2. (task 1) intel_uncore_suspend()->iosf_mbi_unregister_pmic_bus_access_notifier() 3. (task 2) DW I2C access start 4. (hrtimer irq) intel_uncore_fw_release_timer() does forcewake disable 5. (task 2) DW I2C access end One way to avoid this would be holding iosf_mbi_punit_mutex() around the whole iosf_mbi_unregister_pmic_bus_access_notifier() intel_uncore_forcewake_reset() sequence. --Imre > > static u64 gen9_edram_size(struct drm_i915_private *dev_priv) > -- > 2.13.0 > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
