[PATCHv2 31/31] drm/omap: fix crash on module unload

[Tomi Valkeinen]

On 25/03/17 23:25, Daniel Vetter wrote:
> On Fri, Mar 24, 2017 at 11:40:52AM +0200, Tomi Valkeinen wrote:
>> When unloading omapdrm we get a NULL pointer deref in
>> omap_drm_irq_uninstall(). This is caused by:
>>
>> 967dd48417874dd25491a4e933648f394a64f70f ("drm: remove
>> drm_vblank_no_hw_counter assignment from driver code")
>>
>> As OMAP DSS does not have a HW vblank counter, vblank[i].last is anyway
>> always 0, so we can just remove the call to get_vblank_counter().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_irq.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
>> index 26a3c06aa14d..130fdc3225ed 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_irq.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
>> @@ -299,8 +299,6 @@ void omap_drm_irq_uninstall(struct drm_device *dev)
>>  		for (i = 0; i < dev->num_crtcs; i++) {
>>  			wake_up(&dev->vblank[i].queue);
>>  			dev->vblank[i].enabled = false;
>> -			dev->vblank[i].last =
>> -				dev->driver->get_vblank_counter(dev, i);
>>  		}
>>  		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> 
> Why do you even need to open-code this? Assuming you shut down all crtc
> before unloading (there's a nice new atomic helper for that, soon in
> drm-misc-next), and you properly call drm_crtc_vblank_on/off() in the crtc
> enable/disable functions, this should all behandled for your already ...

Yes, I thought this all felt a bit pointless. But I haven't written that
code and I didn't quite figure out all the details about vblank handling
while going through the code, so I decided to just fix the crash bug for
now.

So there should not be anything in any of the vblank queues if
drm_crtc_vblank_off()s have been called? Yes... Looks like it. Ok, I'll
ensure that we disable the crtcs (we should) when unloading, and update
the patch to drop all that code in the omap_drm_irq_uninstall().

Thanks for pointing this out, I love dropping unneeded code lines =).

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170327/f35cb0bf/attachment.sig>