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?
- maybe some mode_config disabling code (for example
intel_mst_disable_dp) - intel_mode_config_cleanup is called after
intel_dp_mst_suspend.
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.
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
