diff mbox

[15/17] drm/tegra: Fix potential bug on driver unload

Message ID 1415006868-318-15-git-send-email-thierry.reding@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thierry Reding Nov. 3, 2014, 9:27 a.m. UTC
From: Thierry Reding <treding@nvidia.com>

The HDMI hotplug signal may toggle after the DRM driver has been
unloaded. Make sure not to call into DRM if that's the case.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/output.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Andrzej Hajda Nov. 4, 2014, 10:59 a.m. UTC | #1
Hi Thierry,

Just passing by.

On 11/03/2014 10:27 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The HDMI hotplug signal may toggle after the DRM driver has been
> unloaded. Make sure not to call into DRM if that's the case.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/output.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index 6b393cfbb5e7..def74914dd72 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -181,7 +181,8 @@ static irqreturn_t hpd_irq(int irq, void *data)
>  {
>  	struct tegra_output *output = data;
>  
> -	drm_helper_hpd_irq_event(output->connector.dev);
> +	if (output->connector.dev)
> +		drm_helper_hpd_irq_event(output->connector.dev);
>  
>  	return IRQ_HANDLED;
>  }
> 

Since output->connector.dev is not synchronized between irq and other
code this patch do not solves the issue, it just decreases chances of
the disaster.

Regards
Andrzej
Thierry Reding Nov. 4, 2014, 12:30 p.m. UTC | #2
On Tue, Nov 04, 2014 at 11:59:46AM +0100, Andrzej Hajda wrote:
> Hi Thierry,
> 
> Just passing by.
> 
> On 11/03/2014 10:27 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The HDMI hotplug signal may toggle after the DRM driver has been
> > unloaded. Make sure not to call into DRM if that's the case.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/gpu/drm/tegra/output.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> > index 6b393cfbb5e7..def74914dd72 100644
> > --- a/drivers/gpu/drm/tegra/output.c
> > +++ b/drivers/gpu/drm/tegra/output.c
> > @@ -181,7 +181,8 @@ static irqreturn_t hpd_irq(int irq, void *data)
> >  {
> >  	struct tegra_output *output = data;
> >  
> > -	drm_helper_hpd_irq_event(output->connector.dev);
> > +	if (output->connector.dev)
> > +		drm_helper_hpd_irq_event(output->connector.dev);
> >  
> >  	return IRQ_HANDLED;
> >  }
> > 
> 
> Since output->connector.dev is not synchronized between irq and other
> code this patch do not solves the issue, it just decreases chances of
> the disaster.

You're right. I guess in addition to this we could call enable_irq()
when the connector is bound to the DRM device and disable_irq() when it
is unbound. Actually, that should even allow drm_helper_hpd_irq_event()
to be called unconditionally because .dev could not be NULL in that
case.

Upon closer inspection it seems like the majority of drivers would fall
prey to this particular race.

Thierry
diff mbox

Patch

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 6b393cfbb5e7..def74914dd72 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -181,7 +181,8 @@  static irqreturn_t hpd_irq(int irq, void *data)
 {
 	struct tegra_output *output = data;
 
-	drm_helper_hpd_irq_event(output->connector.dev);
+	if (output->connector.dev)
+		drm_helper_hpd_irq_event(output->connector.dev);
 
 	return IRQ_HANDLED;
 }