Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Peter Chubb]

On my HP Elitebook 8740w qith a Mobility Radeon HD 5870
commit 846ae41ae99d314bf2a02784152208a6ddf7eddc
breaks shutdown.  The machine hangs when trying to shutdown, kexec or
hibernate, before seeing the usual `machine halted' (or whatever) message.

If I comment out thus:

index 9f5ff28..40bff3c 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -514,7 +514,7 @@ radeon_pci_shutdown(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
-	radeon_driver_unload_kms(dev);
+	/*radeon_driver_unload_kms(dev);*/
 }
 
then everything works again.  Thsi is obviously not the proper fix.

--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA

Re: Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Markus Trippelsdorf]

On 2013.12.11 at 11:37 +1100, Peter Chubb wrote:
> On my HP Elitebook 8740w qith a Mobility Radeon HD 5870
> commit 846ae41ae99d314bf2a02784152208a6ddf7eddc
> breaks shutdown.  The machine hangs when trying to shutdown, kexec or
> hibernate, before seeing the usual `machine halted' (or whatever) message.
> 
> If I comment out thus:
> 
> index 9f5ff28..40bff3c 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -514,7 +514,7 @@ radeon_pci_shutdown(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  
> -	radeon_driver_unload_kms(dev);
> +	/*radeon_driver_unload_kms(dev);*/
>  }
>  
> then everything works again.  Thsi is obviously not the proper fix.

It would be interesting to know where exactly it hangs.
Could you comment out the *_fini(rdev) calls in radeon_driver_unload_kms
(drivers/gpu/drm/radeon/radeon_kms.c) one after the other to find out
which one is responsible? 

-- 
Markus

Re: Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Peter Chubb]

>>>>> "Markus" == Markus Trippelsdorf <markus@trippelsdorf.de> writes:

Markus> On 2013.12.11 at 11:37 +1100, Peter Chubb wrote:

Markus> It would be interesting to know where exactly it hangs.  Could
Markus> you comment out the *_fini(rdev) calls in
Markus> radeon_driver_unload_kms (drivers/gpu/drm/radeon/radeon_kms.c)
Markus> one after the other to find out which one is responsible?

It's radeon_device_fini() that is the problem.
--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA

RE: Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Deucher, Alexander]

> -----Original Message-----
> From: Peter Chubb [mailto:peter.chubb@nicta.com.au]
> Sent: Wednesday, December 11, 2013 5:11 PM
> To: Markus Trippelsdorf
> Cc: Peter Chubb; Deucher, Alexander; airlied@linux.ie; dri-
> devel@lists.freedesktop.org
> Subject: Re: Can no longer shutdown after drm/radeon: Implement
> radeon_pci_shutdown
> 
> >>>>> "Markus" == Markus Trippelsdorf <markus@trippelsdorf.de> writes:
> 
> Markus> On 2013.12.11 at 11:37 +1100, Peter Chubb wrote:
> 
> Markus> It would be interesting to know where exactly it hangs.  Could
> Markus> you comment out the *_fini(rdev) calls in
> Markus> radeon_driver_unload_kms
> (drivers/gpu/drm/radeon/radeon_kms.c)
> Markus> one after the other to find out which one is responsible?
> 
> It's radeon_device_fini() that is the problem.

I think the problem is that the drm subsystem tears down the device via drm_driver.unload in drm_dev_unregister(), but now that we have a pci_driver.shutdown callback (which is needed for kexec) that gets called too so the driver gets torn down twice.  What exactly happens when you say it's broken?

Alex

Re: Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Peter Chubb]

>>>>> "Deucher," == Deucher, Alexander <Alexander.Deucher@amd.com> writes:

>> 
>> >>>>> "Markus" == Markus Trippelsdorf <markus@trippelsdorf.de>
>> writes:
>> 
Markus> On 2013.12.11 at 11:37 +1100, Peter Chubb wrote:
>> 
Markus> It would be interesting to know where exactly it hangs.  Could
Markus> you comment out the *_fini(rdev) calls in
Markus> radeon_driver_unload_kms
>> (drivers/gpu/drm/radeon/radeon_kms.c)
Markus> one after the other to find out which one is responsible?
>> 
>> It's radeon_device_fini() that is the problem.

Deucher,> I think the problem is that the drm subsystem tears down the
Deucher,> device via drm_driver.unload in drm_dev_unregister(), but
Deucher,> now that we have a pci_driver.shutdown callback (which is
Deucher,> needed for kexec) that gets called too so the driver gets
Deucher,> torn down twice.  What exactly happens when you say it's
Deucher,> broken?

The machine hangs, with blank screen, getting hotter and hotter with
the fan screaming.  Magic sysrq does nothing.  Power button sometimes
works and sometimes does nothing.  The only sure way out is to take the
battery out of the laptop.

--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA

Re: Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Markus Trippelsdorf]

On 2013.12.11 at 23:46 +0000, Deucher, Alexander wrote:
> > -----Original Message-----
> > From: Peter Chubb [mailto:peter.chubb@nicta.com.au]
> > Sent: Wednesday, December 11, 2013 5:11 PM
> > To: Markus Trippelsdorf
> > Cc: Peter Chubb; Deucher, Alexander; airlied@linux.ie; dri-
> > devel@lists.freedesktop.org
> > Subject: Re: Can no longer shutdown after drm/radeon: Implement
> > radeon_pci_shutdown
> > 
> > >>>>> "Markus" == Markus Trippelsdorf <markus@trippelsdorf.de> writes:
> > 
> > Markus> On 2013.12.11 at 11:37 +1100, Peter Chubb wrote:
> > 
> > Markus> It would be interesting to know where exactly it hangs.  Could
> > Markus> you comment out the *_fini(rdev) calls in
> > Markus> radeon_driver_unload_kms
> > (drivers/gpu/drm/radeon/radeon_kms.c)
> > Markus> one after the other to find out which one is responsible?
> > 
> > It's radeon_device_fini() that is the problem.
> 
> I think the problem is that the drm subsystem tears down the device
> via drm_driver.unload in drm_dev_unregister(), but now that we have a
> pci_driver.shutdown callback (which is needed for kexec) that gets
> called too so the driver gets torn down twice.

If that is the case the following patch should fix the issue.
Can you give it a try, Peter?

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index 55d0b474bd37..539e5f1ff5e3 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -59,7 +59,8 @@ int radeon_driver_unload_kms(struct drm_device *dev)
 	radeon_acpi_fini(rdev);
 	
 	radeon_modeset_fini(rdev);
-	radeon_device_fini(rdev);
+	if (!rdev->shutdown)
+		radeon_device_fini(rdev);
 
 done_free:
 	kfree(rdev);
-- 
Markus

Re: Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Peter Chubb]

>>>>> "Markus" == Markus Trippelsdorf <markus@trippelsdorf.de> writes:
>> > >>>>> "Markus" == Markus Trippelsdorf <markus@trippelsdorf.de>


Markus> If that is the case the following patch should fix the issue.
Markus> Can you give it a try, Peter?

Thanks that works.  I tested shutdown, kexec, and s2disk --- all work
correctly.


Peter C
--
Dr Peter Chubb				        peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA

RE: Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Deucher, Alexander]

> -----Original Message-----
> From: Markus Trippelsdorf [mailto:markus@trippelsdorf.de]
> Sent: Wednesday, December 11, 2013 9:58 PM
> To: Deucher, Alexander
> Cc: Peter Chubb; airlied@linux.ie; dri-devel@lists.freedesktop.org
> Subject: Re: Can no longer shutdown after drm/radeon: Implement
> radeon_pci_shutdown
> 
> On 2013.12.11 at 23:46 +0000, Deucher, Alexander wrote:
> > > -----Original Message-----
> > > From: Peter Chubb [mailto:peter.chubb@nicta.com.au]
> > > Sent: Wednesday, December 11, 2013 5:11 PM
> > > To: Markus Trippelsdorf
> > > Cc: Peter Chubb; Deucher, Alexander; airlied@linux.ie; dri-
> > > devel@lists.freedesktop.org
> > > Subject: Re: Can no longer shutdown after drm/radeon: Implement
> > > radeon_pci_shutdown
> > >
> > > >>>>> "Markus" == Markus Trippelsdorf <markus@trippelsdorf.de>
> writes:
> > >
> > > Markus> On 2013.12.11 at 11:37 +1100, Peter Chubb wrote:
> > >
> > > Markus> It would be interesting to know where exactly it hangs.  Could
> > > Markus> you comment out the *_fini(rdev) calls in
> > > Markus> radeon_driver_unload_kms
> > > (drivers/gpu/drm/radeon/radeon_kms.c)
> > > Markus> one after the other to find out which one is responsible?
> > >
> > > It's radeon_device_fini() that is the problem.
> >
> > I think the problem is that the drm subsystem tears down the device
> > via drm_driver.unload in drm_dev_unregister(), but now that we have a
> > pci_driver.shutdown callback (which is needed for kexec) that gets
> > called too so the driver gets torn down twice.
> 
> If that is the case the following patch should fix the issue.
> Can you give it a try, Peter?

That may work, but I think it's just papering over a race which may still bite someone else depending on the timing.

Alex

> 
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c
> b/drivers/gpu/drm/radeon/radeon_kms.c
> index 55d0b474bd37..539e5f1ff5e3 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -59,7 +59,8 @@ int radeon_driver_unload_kms(struct drm_device
> *dev)
>  	radeon_acpi_fini(rdev);
> 
>  	radeon_modeset_fini(rdev);
> -	radeon_device_fini(rdev);
> +	if (!rdev->shutdown)
> +		radeon_device_fini(rdev);
> 
>  done_free:
>  	kfree(rdev);
> --
> Markus

Re: Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Markus Trippelsdorf]

On 2013.12.12 at 03:27 +0000, Deucher, Alexander wrote:
> > On 2013.12.11 at 23:46 +0000, Deucher, Alexander wrote:
> > > > -----Original Message-----
> > > > From: Peter Chubb [mailto:peter.chubb@nicta.com.au]
> > > > Sent: Wednesday, December 11, 2013 5:11 PM
> > > > To: Markus Trippelsdorf
> > > > Cc: Peter Chubb; Deucher, Alexander; airlied@linux.ie; dri-
> > > > devel@lists.freedesktop.org
> > > > Subject: Re: Can no longer shutdown after drm/radeon: Implement
> > > > radeon_pci_shutdown
> > > >
> > > > >>>>> "Markus" == Markus Trippelsdorf <markus@trippelsdorf.de>
> > writes:
> > > >
> > > > Markus> On 2013.12.11 at 11:37 +1100, Peter Chubb wrote:
> > > >
> > > > Markus> It would be interesting to know where exactly it hangs.  Could
> > > > Markus> you comment out the *_fini(rdev) calls in
> > > > Markus> radeon_driver_unload_kms
> > > > (drivers/gpu/drm/radeon/radeon_kms.c)
> > > > Markus> one after the other to find out which one is responsible?
> > > >
> > > > It's radeon_device_fini() that is the problem.
> > >
> > > I think the problem is that the drm subsystem tears down the device
> > > via drm_driver.unload in drm_dev_unregister(), but now that we have a
> > > pci_driver.shutdown callback (which is needed for kexec) that gets
> > > called too so the driver gets torn down twice.
> > 
> > If that is the case the following patch should fix the issue.
> > Can you give it a try, Peter?
(Peter:)
> Thanks that works.  I tested shutdown, kexec, and s2disk --- all work
> correctly.

> That may work, but I think it's just papering over a race which may
> still bite someone else depending on the timing.

This leaves three possibilities:

1) Revert 846ae41ae99d now and come up with a solution with proper
locking for 3.14
2) Add my simple fix now and implement additional locking if the need
arises for 3.14.
3) Implement a fix with proper locking now.

It's your choice Alex.

-- 
Markus

Re: Can no longer shutdown after drm/radeon: Implement radeon_pci_shutdown

[Alex Deucher]

On Thu, Dec 12, 2013 at 8:56 AM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2013.12.12 at 03:27 +0000, Deucher, Alexander wrote:
>> > On 2013.12.11 at 23:46 +0000, Deucher, Alexander wrote:
>> > > > -----Original Message-----
>> > > > From: Peter Chubb [mailto:peter.chubb@nicta.com.au]
>> > > > Sent: Wednesday, December 11, 2013 5:11 PM
>> > > > To: Markus Trippelsdorf
>> > > > Cc: Peter Chubb; Deucher, Alexander; airlied@linux.ie; dri-
>> > > > devel@lists.freedesktop.org
>> > > > Subject: Re: Can no longer shutdown after drm/radeon: Implement
>> > > > radeon_pci_shutdown
>> > > >
>> > > > >>>>> "Markus" == Markus Trippelsdorf <markus@trippelsdorf.de>
>> > writes:
>> > > >
>> > > > Markus> On 2013.12.11 at 11:37 +1100, Peter Chubb wrote:
>> > > >
>> > > > Markus> It would be interesting to know where exactly it hangs.  Could
>> > > > Markus> you comment out the *_fini(rdev) calls in
>> > > > Markus> radeon_driver_unload_kms
>> > > > (drivers/gpu/drm/radeon/radeon_kms.c)
>> > > > Markus> one after the other to find out which one is responsible?
>> > > >
>> > > > It's radeon_device_fini() that is the problem.
>> > >
>> > > I think the problem is that the drm subsystem tears down the device
>> > > via drm_driver.unload in drm_dev_unregister(), but now that we have a
>> > > pci_driver.shutdown callback (which is needed for kexec) that gets
>> > > called too so the driver gets torn down twice.
>> >
>> > If that is the case the following patch should fix the issue.
>> > Can you give it a try, Peter?
> (Peter:)
>> Thanks that works.  I tested shutdown, kexec, and s2disk --- all work
>> correctly.
>
>> That may work, but I think it's just papering over a race which may
>> still bite someone else depending on the timing.
>
> This leaves three possibilities:
>
> 1) Revert 846ae41ae99d now and come up with a solution with proper
> locking for 3.14
> 2) Add my simple fix now and implement additional locking if the need
> arises for 3.14.
> 3) Implement a fix with proper locking now.
>
> It's your choice Alex.

I guess I'll revert it for now.  I need to think about it a bit more.
Thanks for your help.

Alex

>
> --
> Markus
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel