Re: [Nouveau] [PATCH] drm/nouveau/bl: Fix oops on driver unbind
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 signature.asc Description: PGP signature ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] drm/nouveau/bl: Fix oops on driver unbind
On Mon, Feb 19, 2018 at 10:38:04AM +0100, Pierre Moreau wrote: > 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. Sorry Pierre, I missed this question and am seeing it only now on saving the message away: On shutdown the driver isn't unloaded, rather the driver core walks the device hierarchy bottom-up in device_shutdown() and, for PCI- enumerated GPUs, invokes the bus shutdown hook pci_device_shutdown(). This in turn would normally invoke ->shutdown in nouveau_drm_pci_driver, but we haven't defined it. So the only thing that happens is that pci_device_shutdown() runtime resumes the device and optionally turns off busmastering if a kexec kernel is going to be loaded. So that's mostly fine, the only silly thing is that we shouldn't runtime resume the GPU if it's runtime suspended, only to turn off power immediately afterwards. I submitted a patch to the PCI subsystem a while ago but it was received with some skepticism, I'll have to respin it one of these days or come up with a better solution: https://patchwork.kernel.org/patch/9337553/ Thanks, Lukas ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] drm/nouveau/bl: Fix oops on driver unbind
On 14 March 2018 at 21:54, Lukas Wunnerwrote: > 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 = >client.device; >> > struct drm_connector *connector; >> > >> > + INIT_LIST_HEAD(>bl_connectors); >> > + >> > if (apple_gmux_present()) { >> > NV_INFO(drm, "Apple GMUX detected: not registering Nouveau >> > backlight interface\n"); >> > return 0; >> > } >> > >> > - INIT_LIST_HEAD(>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 > > 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 ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] drm/nouveau/bl: Fix oops on driver unbind
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 = >client.device; > > struct drm_connector *connector; > > > > + INIT_LIST_HEAD(>bl_connectors); > > + > > if (apple_gmux_present()) { > > NV_INFO(drm, "Apple GMUX detected: not registering Nouveau > > backlight interface\n"); > > return 0; > > } > > > > - INIT_LIST_HEAD(>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 MoreauHi 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 ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH] drm/nouveau/bl: Fix oops on driver unbind
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 0010 > 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: sta...@vger.kernel.org # v4.10+ > Cc: Pierre Moreau> Signed-off-by: Lukas Wunner > --- > 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 = >client.device; > struct drm_connector *connector; > > + INIT_LIST_HEAD(>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 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(>bl_connectors); > - > list_for_each_entry(connector, >mode_config.connector_list, head) { > if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS && > connector->connector_type != DRM_MODE_CONNECTOR_eDP) > -- > 2.15.1 > signature.asc Description: PGP signature ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] [PATCH] drm/nouveau/bl: Fix oops on driver unbind
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 0010 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: sta...@vger.kernel.org # v4.10+ Cc: Pierre MoreauSigned-off-by: Lukas Wunner --- 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 = >client.device; struct drm_connector *connector; + INIT_LIST_HEAD(>bl_connectors); + if (apple_gmux_present()) { NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface\n"); return 0; } - INIT_LIST_HEAD(>bl_connectors); - list_for_each_entry(connector, >mode_config.connector_list, head) { if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS && connector->connector_type != DRM_MODE_CONNECTOR_eDP) -- 2.15.1 ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau