On Thu, Aug 25, 2022 at 01:24:04PM +0200, Andrzej Hajda wrote: > On 24.08.2022 13:22, Imre Deak wrote: > > On Tue, Aug 23, 2022 at 11:48:01PM +0200, Andrzej Hajda wrote: > > > > > > > > > On 22.08.2022 19:27, Imre Deak wrote: > > > > On Fri, Jul 22, 2022 at 02:51:42PM +0200, Andrzej Hajda wrote: > > > > > HPD events during driver removal can be generated by hardware and > > > > > software frameworks - drm_dp_mst, the former we can avoid by disabling > > > > > interrupts, the latter can be triggered by any drm_dp_mst transaction, > > > > > and this is too late. Introducing suspended flag allows to solve this > > > > > chicken-egg problem. > > > > intel_hpd_cancel_work() is always called after suspending MST and > > > > disabling IRQs (with the order I suggested in patch 1). If both of these > > > > have disabled the corresponding functionality (MST, IRQs) properly with > > > > all their MST/IRQ scheduled works guaranteed to not get rescheduled, > > > > then it's not clear how could either intel_hpd_trigger_irq() or an IRQ > > > > work run. So the problematic sequence would need a better explanation. > > > > > > I am not familiar with MST but as I understand from earlier discussion MST > > > framework can be called during driver removal code even after > > > intel_dp_mst_suspend. > > > > Not sure how that happens, but it looks wrong. One way I can imagine is > > that connector detection re-enables MST, which should be prevented then. > > I am not MST expert and atm I have no access to the machine on which I could > look for real prove. > As I understand intel_dp_mst_suspend prevents only downstream devices to > initiate transactions, it does not prevent transactions initiated from i915 > driver. > My guesses for such transactions are: > - any ioctl/sysfs/drm_property access initiated by user which can involve > MST tranaction (set brightness, read EDID, reading capabilities, statuses, > ....), unless they are already blocked, how? In theory all these should be blocked already, after i915_driver_unregister() has returned; for the above cases in particular via drm_connector_unregister_all(). > - maybe some mode_config disabling code (for example intel_mst_disable_dp) - This should be called already via i915_driver_unregister() -> intel_display_driver_unregister() -> drm_atomic_helper_shutdown(). > intel_mode_config_cleanup is called after intel_dp_mst_suspend. This should only free the allocated objects etc, but not do any transactions. > And since MST transfer can timeout it can trigger > drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) --> > intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq. > > If such situation happens after intel_dp_mst_suspend > i915->hotplug.dig_port_work will be queued, and we have risk of > use-after-free. > > If this analysis looks incorrect I can send patches 1, 2, 4 with your > comments addressed. CI probably will verify this anyway. Ok. I suppose blocking detection based on a new flag would mean avoiding scheduling and flushing hotplug.hotplug_work already as the first step in intel_display_driver_unregister() and somewhere early during system suspend. > Regards > Andrzej > > > > > > > And since MST transfer can timeout it can trigger > > > drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) --> > > > intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq. > > > > > > So actually this patch supersedes the 1st patch. > > > > > > > > > > > There's also already > > > > dev_priv->runtime_pm.irqs_enabled > > > > showing if hotplug interrupts are disabled (along with all other IRQs). > > > > > > So it is slightly different, this patch introduces flag indicating if HPD is > > > enabled, we can have IRQs not related to HPD, and HPD events not related to > > > IRQs. > > > > In its current form I can't see a difference. What would make sense is > > to add a flag that prevents connector detection (which would > > incorrectly enable MST for instace), but leave the handling of other > > interrupts enabled. That flag would be set already before suspending > > MST. > > > > > Regards > > > Andrzej > > > > > > > > > > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950 > > > > > Signed-off-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx> > > > > > Reviewed-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > > > > > drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++- > > > > > drivers/gpu/drm/i915/display/intel_hotplug.h | 2 +- > > > > > drivers/gpu/drm/i915/i915_driver.c | 4 ++-- > > > > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > > > > 5 files changed, 16 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > index f1c765ac7ab8aa..cd6139bb36151b 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > @@ -9022,7 +9022,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915) > > > > > intel_dp_mst_suspend(i915); > > > > > /* MST is the last user of HPD work */ > > > > > - intel_hpd_cancel_work(i915); > > > > > + intel_hpd_suspend(i915); > > > > > /* poll work can call into fbdev, hence clean that up afterwards */ > > > > > intel_fbdev_fini(i915); > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c > > > > > index 5f8b4f481cff9a..e1d384cb99df6b 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c > > > > > @@ -303,6 +303,8 @@ static void i915_digport_work_func(struct work_struct *work) > > > > > u32 old_bits = 0; > > > > > spin_lock_irq(&dev_priv->irq_lock); > > > > > + if (dev_priv->hotplug.suspended) > > > > > + return spin_unlock_irq(&dev_priv->irq_lock); > > > > > long_port_mask = dev_priv->hotplug.long_port_mask; > > > > > dev_priv->hotplug.long_port_mask = 0; > > > > > short_port_mask = dev_priv->hotplug.short_port_mask; > > > > > @@ -353,6 +355,8 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port) > > > > > struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); > > > > > spin_lock_irq(&i915->irq_lock); > > > > > + if (i915->hotplug.suspended) > > > > > + return spin_unlock_irq(&i915->irq_lock); > > > > > i915->hotplug.short_port_mask |= BIT(dig_port->base.port); > > > > > spin_unlock_irq(&i915->irq_lock); > > > > > @@ -475,6 +479,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv, > > > > > spin_lock(&dev_priv->irq_lock); > > > > > + if (dev_priv->hotplug.suspended) > > > > > + return spin_unlock(&dev_priv->irq_lock); > > > > > + > > > > > /* > > > > > * Determine whether ->hpd_pulse() exists for each pin, and > > > > > * whether we have a short or a long pulse. This is needed > > > > > @@ -603,6 +610,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv) > > > > > * just to make the assert_spin_locked checks happy. > > > > > */ > > > > > spin_lock_irq(&dev_priv->irq_lock); > > > > > + dev_priv->hotplug.suspended = false; > > > > > intel_hpd_irq_setup(dev_priv); > > > > > spin_unlock_irq(&dev_priv->irq_lock); > > > > > } > > > > > @@ -721,13 +729,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv) > > > > > intel_hpd_irq_storm_reenable_work); > > > > > } > > > > > -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) > > > > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv) > > > > > { > > > > > if (!HAS_DISPLAY(dev_priv)) > > > > > return; > > > > > spin_lock_irq(&dev_priv->irq_lock); > > > > > + dev_priv->hotplug.suspended = true; > > > > > dev_priv->hotplug.long_port_mask = 0; > > > > > dev_priv->hotplug.short_port_mask = 0; > > > > > dev_priv->hotplug.event_bits = 0; > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h > > > > > index b87e95d606e668..54bddc4dd63421 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.h > > > > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h > > > > > @@ -23,7 +23,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv, > > > > > void intel_hpd_trigger_irq(struct intel_digital_port *dig_port); > > > > > void intel_hpd_init(struct drm_i915_private *dev_priv); > > > > > void intel_hpd_init_work(struct drm_i915_private *dev_priv); > > > > > -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv); > > > > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv); > > > > > enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv, > > > > > enum port port); > > > > > bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin); > > > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c > > > > > index deb8a8b76965a1..57a063a306e3a4 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_driver.c > > > > > +++ b/drivers/gpu/drm/i915/i915_driver.c > > > > > @@ -1092,7 +1092,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915) > > > > > intel_dp_mst_suspend(i915); > > > > > intel_runtime_pm_disable_interrupts(i915); > > > > > - intel_hpd_cancel_work(i915); > > > > > + intel_hpd_suspend(i915); > > > > > intel_suspend_encoders(i915); > > > > > intel_shutdown_encoders(i915); > > > > > @@ -1161,7 +1161,7 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > > intel_dp_mst_suspend(dev_priv); > > > > > intel_runtime_pm_disable_interrupts(dev_priv); > > > > > - intel_hpd_cancel_work(dev_priv); > > > > > + intel_hpd_suspend(dev_priv); > > > > > intel_suspend_encoders(dev_priv); > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > > > index d25647be25d18b..dc1562b95d7ade 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > > @@ -106,6 +106,8 @@ struct vlv_s0ix_state; > > > > > #define HPD_STORM_DEFAULT_THRESHOLD 50 > > > > > struct i915_hotplug { > > > > > + bool suspended; > > > > > + > > > > > struct delayed_work hotplug_work; > > > > > const u32 *hpd, *pch_hpd; > > > > > -- > > > > > 2.25.1 > > > > > > > > >
