drm/nouveau/bl: Fix oops on driver unbind

Submitted by Lukas Wunner on Feb. 17, 2018, 12:40 p.m.

Details

Message ID 5c682f35ac338efac66416b7670dc01f1ee02676.1518870434.git.lukas@wunner.de
State New
Headers show
Series "drm/nouveau/bl: Fix oops on driver unbind" ( rev: 1 ) in Nouveau

Not browsing as part of any series.

Commit Message

Lukas Wunner Feb. 17, 2018, 12:40 p.m.
Unbinding nouveau on a dual GPU MacBook Pro oopses because we iterate
over the bl_connectors list in nouveau_backlight_exit() but skipped
initializing it in nouveau_backlight_init().  Stacktrace for posterity:

    BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
    IP: nouveau_backlight_exit+0x2b/0x70 [nouveau]
    nouveau_display_destroy+0x29/0x80 [nouveau]
    nouveau_drm_unload+0x65/0xe0 [nouveau]
    drm_dev_unregister+0x3c/0xe0 [drm]
    drm_put_dev+0x2e/0x60 [drm]
    nouveau_drm_device_remove+0x47/0x70 [nouveau]
    pci_device_remove+0x36/0xb0
    device_release_driver_internal+0x157/0x220
    driver_detach+0x39/0x70
    bus_remove_driver+0x51/0xd0
    pci_unregister_driver+0x2a/0xa0
    nouveau_drm_exit+0x15/0xfb0 [nouveau]
    SyS_delete_module+0x18c/0x290
    system_call_fast_compare_end+0xc/0x6f

Fixes: b53ac1ee12a3 ("drm/nouveau/bl: Do not register interface if Apple GMUX detected")
Cc: stable@vger.kernel.org # v4.10+
Cc: Pierre Moreau <pierre.morrow@free.fr>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
I reviewed the patch causing the oops but unfortunately missed this, sorry!

 drivers/gpu/drm/nouveau/nouveau_backlight.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 380f340204e8..f56f60f695e1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -268,13 +268,13 @@  nouveau_backlight_init(struct drm_device *dev)
 	struct nvif_device *device = &drm->client.device;
 	struct drm_connector *connector;
 
+	INIT_LIST_HEAD(&drm->bl_connectors);
+
 	if (apple_gmux_present()) {
 		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface\n");
 		return 0;
 	}
 
-	INIT_LIST_HEAD(&drm->bl_connectors);
-
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
 		    connector->connector_type != DRM_MODE_CONNECTOR_eDP)

Comments

My bad; I did test suspend/resume, but not unbinding. Shouldn’t that
happen on shutdown as well, as the driver gets unloaded? I don’t remember
seeing that issue there though.

On 2018-02-17 — 13:40, Lukas Wunner wrote:
> Unbinding nouveau on a dual GPU MacBook Pro oopses because we iterate
> over the bl_connectors list in nouveau_backlight_exit() but skipped
> initializing it in nouveau_backlight_init().  Stacktrace for posterity:
> 
>     BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
>     IP: nouveau_backlight_exit+0x2b/0x70 [nouveau]
>     nouveau_display_destroy+0x29/0x80 [nouveau]
>     nouveau_drm_unload+0x65/0xe0 [nouveau]
>     drm_dev_unregister+0x3c/0xe0 [drm]
>     drm_put_dev+0x2e/0x60 [drm]
>     nouveau_drm_device_remove+0x47/0x70 [nouveau]
>     pci_device_remove+0x36/0xb0
>     device_release_driver_internal+0x157/0x220
>     driver_detach+0x39/0x70
>     bus_remove_driver+0x51/0xd0
>     pci_unregister_driver+0x2a/0xa0
>     nouveau_drm_exit+0x15/0xfb0 [nouveau]
>     SyS_delete_module+0x18c/0x290
>     system_call_fast_compare_end+0xc/0x6f
> 
> Fixes: b53ac1ee12a3 ("drm/nouveau/bl: Do not register interface if Apple GMUX detected")
> Cc: stable@vger.kernel.org # v4.10+
> Cc: Pierre Moreau <pierre.morrow@free.fr>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> I reviewed the patch causing the oops but unfortunately missed this, sorry!
> 
>  drivers/gpu/drm/nouveau/nouveau_backlight.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> index 380f340204e8..f56f60f695e1 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> @@ -268,13 +268,13 @@ nouveau_backlight_init(struct drm_device *dev)
>  	struct nvif_device *device = &drm->client.device;
>  	struct drm_connector *connector;
>  
> +	INIT_LIST_HEAD(&drm->bl_connectors);
> +

We could instead have an early return in the exit function if
`apple_gmux_present()` or `drm->bl_connectors` is null, but your current fix
seems better.

Reviewed-by: Pierre Moreau <pierre.morrow@free.fr>

Thank you for the fix!
Pierre

>  	if (apple_gmux_present()) {
>  		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface\n");
>  		return 0;
>  	}
>  
> -	INIT_LIST_HEAD(&drm->bl_connectors);
> -
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>  		if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
>  		    connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> -- 
> 2.15.1
>
On Mon, Feb 19, 2018 at 10:38:04AM +0100, Pierre Moreau wrote:
> On 2018-02-17 13:40, Lukas Wunner wrote:
> > Unbinding nouveau on a dual GPU MacBook Pro oopses because we iterate
> > over the bl_connectors list in nouveau_backlight_exit() but skipped
> > initializing it in nouveau_backlight_init().
> > --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> > @@ -268,13 +268,13 @@ nouveau_backlight_init(struct drm_device *dev)
> >  	struct nvif_device *device = &drm->client.device;
> >  	struct drm_connector *connector;
> >  
> > +	INIT_LIST_HEAD(&drm->bl_connectors);
> > +
> >  	if (apple_gmux_present()) {
> >  		NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface\n");
> >  		return 0;
> >  	}
> >  
> > -	INIT_LIST_HEAD(&drm->bl_connectors);
> > -
> 
> We could instead have an early return in the exit function if
> `apple_gmux_present()` or `drm->bl_connectors` is null, but your
> current fix seems better.
> 
> Reviewed-by: Pierre Moreau <pierre.morrow@free.fr>

Hi Ben, just a gentle ping:  I'm not seeing this fix on one of Dave's
branches and neither in your GitHub repo.  I could merge it through
drm-misc-fixes but I'd need an ack from you for that.  Also, please
let me know if you'd prefer the alternative solution Pierre outlined
above.  Thanks!

Lukas
On 14 March 2018 at 21:54, Lukas Wunner <lukas@wunner.de> wrote:
> On Mon, Feb 19, 2018 at 10:38:04AM +0100, Pierre Moreau wrote:
>> On 2018-02-17 13:40, Lukas Wunner wrote:
>> > Unbinding nouveau on a dual GPU MacBook Pro oopses because we iterate
>> > over the bl_connectors list in nouveau_backlight_exit() but skipped
>> > initializing it in nouveau_backlight_init().
>> > --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
>> > +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
>> > @@ -268,13 +268,13 @@ nouveau_backlight_init(struct drm_device *dev)
>> >     struct nvif_device *device = &drm->client.device;
>> >     struct drm_connector *connector;
>> >
>> > +   INIT_LIST_HEAD(&drm->bl_connectors);
>> > +
>> >     if (apple_gmux_present()) {
>> >             NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface\n");
>> >             return 0;
>> >     }
>> >
>> > -   INIT_LIST_HEAD(&drm->bl_connectors);
>> > -
>>
>> We could instead have an early return in the exit function if
>> `apple_gmux_present()` or `drm->bl_connectors` is null, but your
>> current fix seems better.
>>
>> Reviewed-by: Pierre Moreau <pierre.morrow@free.fr>
>
> Hi Ben, just a gentle ping:  I'm not seeing this fix on one of Dave's
> branches and neither in your GitHub repo.  I could merge it through
> drm-misc-fixes but I'd need an ack from you for that.  Also, please
> let me know if you'd prefer the alternative solution Pierre outlined
> above.  Thanks!
Thanks for the ping!  I've picked up the patch in my tree, and
forwarded a -fixes onto Dave.

Ben.

>
> Lukas
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
Hey Lukas,

> Sorry Pierre, I missed this question and am seeing it only now on
> saving the message away:

No worries, and thank you for the great explanation! I’ll definitely make a
mental note to try unloading Nouveau, whenever writing/testing similar patches.

Thanks,
Pierre