Jeffery Miller
2018-Jul-05 19:09 UTC
[Nouveau] [PATCH 0/2] drm/nouveau: Fix panic on nouveau unload.
If have a couple patches I found while looking at a panic I was seeing while unloading the nouveau module. Unloading the nouveau module on my optimus notebook machine causes the system to panic. This started occuring when moving from 4.4 to 4.14. These patches make it such that the system does not panic when unloading the module. 4.14 also requires commit 34112bf4935d ("drm/nouveau/fbcon: fix NULL pointer access in nouveau_fbcon_destroy") which was already included in the 4.18 tree. These patches make it so I can unload the module without a panic but there is a warning when unloading the module: sysfs group 'power' not found for kobject 'nv_backlight' WARNING: CPU: 2 PID: 1434 at fs/sysfs/group.c:235 sysfs_remove_group+0x76/0x80 RIP: 0010:sysfs_remove_group+0x76/0x80 Call Trace: device_del+0x56/0x350 ? down_write+0xe/0x40 device_unregister+0x16/0x60 nouveau_backlight_exit+0x4a/0x60 [nouveau] nouveau_display_destroy+0x29/0x80 [nouveau] nouveau_drm_unload+0x61/0xd0 [nouveau] drm_dev_unregister+0x3f/0xe0 [drm] drm_put_dev+0x27/0x50 [drm] nouveau_drm_device_remove+0x47/0x70 [nouveau] pci_device_remove+0x3b/0xb0 device_release_driver_internal+0x180/0x250 driver_detach+0x32/0x5f bus_remove_driver+0x74/0xc6 pci_unregister_driver+0x22/0xa0 nouveau_drm_exit+0x15/0x16b [nouveau] I don't beleive them to be related. Perhaps there is another issue here? Jeffery Miller (2): drm/nouveau/fbcon: Fix NULL pointer access in nouveau_fbcon_destroy. drm/nouveau/bl: Allocate backlight connector nodes. drivers/gpu/drm/nouveau/nouveau_backlight.c | 70 ++++++++++++++------- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 +- 2 files changed, 51 insertions(+), 24 deletions(-) -- 2.17.1
Jeffery Miller
2018-Jul-05 19:09 UTC
[Nouveau] [PATCH 1/2] drm/nouveau/fbcon: Fix NULL pointer access in nouveau_fbcon_destroy.
It is possible for this to get called with a null helper.fb. This can cause a bad pointer access when unloading the nouveau module on an Optimus system. Signed-off-by: Jeffery Miller <jmiller at neverware.com> --- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 85c1f10bc2b6..99b2e5bb2bce 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -420,7 +420,10 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, static int nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon) { - struct nouveau_framebuffer *nouveau_fb = nouveau_framebuffer(fbcon->helper.fb); + struct nouveau_framebuffer *nouveau_fb = NULL; + + if (fbcon->helper.fb) + nouveau_fb = nouveau_framebuffer(fbcon->helper.fb); drm_fb_helper_unregister_fbi(&fbcon->helper); drm_fb_helper_fini(&fbcon->helper); -- 2.17.1
Jeffery Miller
2018-Jul-05 19:10 UTC
[Nouveau] [PATCH 2/2] drm/nouveau/bl: Allocate backlight connector nodes.
Avoid adding nodes to the bl_connectors list from the stack. Nodes on the stack were being added to the bl_connectors list. nouveau_backlight_exit would panic while traversing the bl_connectors list. This panic was observed on an optimus system when unloading the nouveau module. Signed-off-by: Jeffery Miller <jmiller at neverware.com> --- drivers/gpu/drm/nouveau/nouveau_backlight.c | 70 ++++++++++++++------- 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c index debbbf0fd4bd..fce2441cce0d 100644 --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c @@ -46,19 +46,40 @@ struct backlight_connector { int id; }; -static bool +static void nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE], struct backlight_connector *connector) { - const int nb = ida_simple_get(&bl_ida, 0, 0, GFP_KERNEL); - if (nb < 0 || nb >= 100) - return false; - if (nb > 0) - snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb); + if (connector->id > 0) + snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", + connector->id); else snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight"); - connector->id = nb; - return true; +} + +static struct backlight_connector * +nouveau_alloc_bl_connector(void) +{ + const int nb = ida_simple_get(&bl_ida, 0, 100, GFP_KERNEL); + struct backlight_connector *bl_connector; + + if (nb < 0) + return ERR_PTR(nb); + bl_connector = kzalloc(sizeof(*bl_connector), GFP_KERNEL); + if (!bl_connector) { + ida_simple_remove(&bl_ida, nb); + return ERR_PTR(-ENOMEM); + } + bl_connector->id = nb; + return bl_connector; +} + +static void +nouveau_free_bl_connector(struct backlight_connector *bl_connector) +{ + if (bl_connector->id >= 0) + ida_simple_remove(&bl_ida, bl_connector->id); + kfree(bl_connector); } static int @@ -99,7 +120,7 @@ nv40_backlight_init(struct drm_connector *connector) struct nvif_object *device = &drm->client.device.object; struct backlight_properties props; struct backlight_device *bd; - struct backlight_connector bl_connector; + struct backlight_connector *bl_connector; char backlight_name[BL_NAME_SIZE]; if (!(nvif_rd32(device, NV40_PMC_BACKLIGHT) & NV40_PMC_BACKLIGHT_MASK)) @@ -108,19 +129,21 @@ nv40_backlight_init(struct drm_connector *connector) memset(&props, 0, sizeof(struct backlight_properties)); props.type = BACKLIGHT_RAW; props.max_brightness = 31; - if (!nouveau_get_backlight_name(backlight_name, &bl_connector)) { + bl_connector = nouveau_alloc_bl_connector(); + if (IS_ERR(bl_connector)) { NV_ERROR(drm, "Failed to retrieve a unique name for the backlight interface\n"); - return 0; + return PTR_ERR(bl_connector); } + + nouveau_get_backlight_name(backlight_name, bl_connector); bd = backlight_device_register(backlight_name , connector->kdev, drm, &nv40_bl_ops, &props); if (IS_ERR(bd)) { - if (bl_connector.id > 0) - ida_simple_remove(&bl_ida, bl_connector.id); + nouveau_free_bl_connector(bl_connector); return PTR_ERR(bd); } - list_add(&bl_connector.head, &drm->bl_connectors); + list_add(&bl_connector->head, &drm->bl_connectors); drm->backlight = bd; bd->props.brightness = nv40_get_intensity(bd); backlight_update_status(bd); @@ -218,7 +241,7 @@ nv50_backlight_init(struct drm_connector *connector) struct backlight_properties props; struct backlight_device *bd; const struct backlight_ops *ops; - struct backlight_connector bl_connector; + struct backlight_connector *bl_connector; char backlight_name[BL_NAME_SIZE]; nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS); @@ -241,20 +264,21 @@ nv50_backlight_init(struct drm_connector *connector) memset(&props, 0, sizeof(struct backlight_properties)); props.type = BACKLIGHT_RAW; props.max_brightness = 100; - if (!nouveau_get_backlight_name(backlight_name, &bl_connector)) { + bl_connector = nouveau_alloc_bl_connector(); + if (IS_ERR(bl_connector)) { NV_ERROR(drm, "Failed to retrieve a unique name for the backlight interface\n"); - return 0; + return PTR_ERR(bl_connector); } + nouveau_get_backlight_name(backlight_name, bl_connector); bd = backlight_device_register(backlight_name , connector->kdev, nv_encoder, ops, &props); if (IS_ERR(bd)) { - if (bl_connector.id > 0) - ida_simple_remove(&bl_ida, bl_connector.id); + nouveau_free_bl_connector(bl_connector); return PTR_ERR(bd); } - list_add(&bl_connector.head, &drm->bl_connectors); + list_add(&bl_connector->head, &drm->bl_connectors); drm->backlight = bd; bd->props.brightness = bd->ops->get_brightness(bd); backlight_update_status(bd); @@ -302,10 +326,10 @@ nouveau_backlight_exit(struct drm_device *dev) { struct nouveau_drm *drm = nouveau_drm(dev); struct backlight_connector *connector; + struct backlight_connector *safe; - list_for_each_entry(connector, &drm->bl_connectors, head) { - if (connector->id >= 0) - ida_simple_remove(&bl_ida, connector->id); + list_for_each_entry_safe(connector, safe, &drm->bl_connectors, head) { + nouveau_free_bl_connector(connector); } if (drm->backlight) { -- 2.17.1
Karol Herbst
2018-Jul-17 10:46 UTC
[Nouveau] [PATCH 0/2] drm/nouveau: Fix panic on nouveau unload.
does this also happen with the newest kernel? I was kind of under the impression we already fixed such issues. On Thu, Jul 5, 2018 at 9:09 PM, Jeffery Miller <jmiller at neverware.com> wrote:> If have a couple patches I found while looking at a panic > I was seeing while unloading the nouveau module. > > Unloading the nouveau module on my optimus notebook machine causes > the system to panic. This started occuring when moving from 4.4 > to 4.14. > > These patches make it such that the system does not panic > when unloading the module. > > 4.14 also requires commit 34112bf4935d ("drm/nouveau/fbcon: fix NULL > pointer access in nouveau_fbcon_destroy") which was already included in > the 4.18 tree. > > These patches make it so I can unload the module without a panic but > there is a warning when unloading the module: > sysfs group 'power' not found for kobject 'nv_backlight' > WARNING: CPU: 2 PID: 1434 at fs/sysfs/group.c:235 sysfs_remove_group+0x76/0x80 > RIP: 0010:sysfs_remove_group+0x76/0x80 > Call Trace: > device_del+0x56/0x350 > ? down_write+0xe/0x40 > device_unregister+0x16/0x60 > nouveau_backlight_exit+0x4a/0x60 [nouveau] > nouveau_display_destroy+0x29/0x80 [nouveau] > nouveau_drm_unload+0x61/0xd0 [nouveau] > drm_dev_unregister+0x3f/0xe0 [drm] > drm_put_dev+0x27/0x50 [drm] > nouveau_drm_device_remove+0x47/0x70 [nouveau] > pci_device_remove+0x3b/0xb0 > device_release_driver_internal+0x180/0x250 > driver_detach+0x32/0x5f > bus_remove_driver+0x74/0xc6 > pci_unregister_driver+0x22/0xa0 > nouveau_drm_exit+0x15/0x16b [nouveau] > > I don't beleive them to be related. Perhaps there is another issue here? > > Jeffery Miller (2): > drm/nouveau/fbcon: Fix NULL pointer access in nouveau_fbcon_destroy. > drm/nouveau/bl: Allocate backlight connector nodes. > > drivers/gpu/drm/nouveau/nouveau_backlight.c | 70 ++++++++++++++------- > drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 +- > 2 files changed, 51 insertions(+), 24 deletions(-) > > -- > 2.17.1 > > _______________________________________________ > Nouveau mailing list > Nouveau at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau
Karol Herbst
2018-Jul-17 13:05 UTC
[Nouveau] [PATCH 0/2] drm/nouveau: Fix panic on nouveau unload.
nevermind, I just hit it today. Will test your patches On Tue, Jul 17, 2018 at 12:46 PM, Karol Herbst <kherbst at redhat.com> wrote:> does this also happen with the newest kernel? I was kind of under the > impression we already fixed such issues. > > On Thu, Jul 5, 2018 at 9:09 PM, Jeffery Miller <jmiller at neverware.com> wrote: >> If have a couple patches I found while looking at a panic >> I was seeing while unloading the nouveau module. >> >> Unloading the nouveau module on my optimus notebook machine causes >> the system to panic. This started occuring when moving from 4.4 >> to 4.14. >> >> These patches make it such that the system does not panic >> when unloading the module. >> >> 4.14 also requires commit 34112bf4935d ("drm/nouveau/fbcon: fix NULL >> pointer access in nouveau_fbcon_destroy") which was already included in >> the 4.18 tree. >> >> These patches make it so I can unload the module without a panic but >> there is a warning when unloading the module: >> sysfs group 'power' not found for kobject 'nv_backlight' >> WARNING: CPU: 2 PID: 1434 at fs/sysfs/group.c:235 sysfs_remove_group+0x76/0x80 >> RIP: 0010:sysfs_remove_group+0x76/0x80 >> Call Trace: >> device_del+0x56/0x350 >> ? down_write+0xe/0x40 >> device_unregister+0x16/0x60 >> nouveau_backlight_exit+0x4a/0x60 [nouveau] >> nouveau_display_destroy+0x29/0x80 [nouveau] >> nouveau_drm_unload+0x61/0xd0 [nouveau] >> drm_dev_unregister+0x3f/0xe0 [drm] >> drm_put_dev+0x27/0x50 [drm] >> nouveau_drm_device_remove+0x47/0x70 [nouveau] >> pci_device_remove+0x3b/0xb0 >> device_release_driver_internal+0x180/0x250 >> driver_detach+0x32/0x5f >> bus_remove_driver+0x74/0xc6 >> pci_unregister_driver+0x22/0xa0 >> nouveau_drm_exit+0x15/0x16b [nouveau] >> >> I don't beleive them to be related. Perhaps there is another issue here? >> >> Jeffery Miller (2): >> drm/nouveau/fbcon: Fix NULL pointer access in nouveau_fbcon_destroy. >> drm/nouveau/bl: Allocate backlight connector nodes. >> >> drivers/gpu/drm/nouveau/nouveau_backlight.c | 70 ++++++++++++++------- >> drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 +- >> 2 files changed, 51 insertions(+), 24 deletions(-) >> >> -- >> 2.17.1 >> >> _______________________________________________ >> Nouveau mailing list >> Nouveau at lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/nouveau
Apparently Analagous Threads
- [PATCH 0/2] drm/nouveau: Fix panic on nouveau unload.
- [PATCH 0/2] drm/nouveau: Fix panic on nouveau unload.
- [PATCH 0/2] drm/nouveau: Fix panic on nouveau unload.
- [PATCH] drm/nouveau/bl: Fix oops on driver unbind
- [PATCH] drm/nouveau/bl: Fix oops on driver unbind