| Message ID | 20180414091233.32224-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Apr 14, 2018 at 09:18:52AM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Call i915_perf_fini() on init_hw error unwind > URL : https://patchwork.freedesktop.org/series/41711/ > State : failure > > == Summary == > > CHK include/config/kernel.release > CHK include/generated/uapi/linux/version.h > CHK include/generated/utsrelease.h > CHK include/generated/bounds.h > CHK include/generated/timeconst.h > CHK include/generated/asm-offsets.h > CALL scripts/checksyscalls.sh > DESCEND objtool > CHK scripts/mod/devicetable-offsets.h > CHK include/generated/compile.h > CHK kernel/config_data.h > CHK include/generated/uapi/linux/version.h > DATAREL arch/x86/boot/compressed/vmlinux > LD arch/x86/boot/setup.elf > OBJCOPY arch/x86/boot/setup.bin > OBJCOPY arch/x86/boot/vmlinux.bin > objcopy:arch/x86/boot/vmlinux.bin[.rodata..compressed]: No space left on device > arch/x86/boot/Makefile:86: recipe for target 'arch/x86/boot/vmlinux.bin' failed > make[1]: *** [arch/x86/boot/vmlinux.bin] Error 1 > arch/x86/Makefile:307: recipe for target 'bzImage' failed > make: *** [bzImage] Error 2 Whoops. Reclaimed some space, rerun queued. -Arek
On 14/04/18 02:12, Chris Wilson wrote: > We have to cleanup after i915_perf_init(), even on the error path, as it > passes a pointer into the module to the sysfs core. If we fail to > unregister the sysctl table, we leave a dangling pointer which then may > explode anytime later the sysctl table, we leave a dangling pointer > which then may explode anytime later. > > Fixes: 9f9b2792b6d3 ("drm/i915/perf: reuse timestamp frequency from device info") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Matthew Auld <matthew.auld@intel.com> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Thanks a lot! > --- > drivers/gpu/drm/i915/i915_drv.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 9944a03cdea6..beac565ddfe0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1109,30 +1109,30 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) > > ret = i915_ggtt_probe_hw(dev_priv); > if (ret) > - return ret; > + goto err_perf; > > /* WARNING: Apparently we must kick fbdev drivers before vgacon, > * otherwise the vga fbdev driver falls over. */ > ret = i915_kick_out_firmware_fb(dev_priv); > if (ret) { > DRM_ERROR("failed to remove conflicting framebuffer drivers\n"); > - goto out_ggtt; > + goto err_ggtt; > } > > ret = i915_kick_out_vgacon(dev_priv); > if (ret) { > DRM_ERROR("failed to remove conflicting VGA console\n"); > - goto out_ggtt; > + goto err_ggtt; > } > > ret = i915_ggtt_init_hw(dev_priv); > if (ret) > - return ret; > + goto err_ggtt; > > ret = i915_ggtt_enable_hw(dev_priv); > if (ret) { > DRM_ERROR("failed to enable GGTT\n"); > - goto out_ggtt; > + goto err_ggtt; > } > > pci_set_master(pdev); > @@ -1143,7 +1143,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) > if (ret) { > DRM_ERROR("failed to set DMA mask\n"); > > - goto out_ggtt; > + goto err_ggtt; > } > } > > @@ -1161,7 +1161,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) > if (ret) { > DRM_ERROR("failed to set DMA mask\n"); > > - goto out_ggtt; > + goto err_ggtt; > } > } > > @@ -1197,13 +1197,14 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) > > ret = intel_gvt_init(dev_priv); > if (ret) > - goto out_ggtt; > + goto err_ggtt; > > return 0; > > -out_ggtt: > +err_ggtt: > i915_ggtt_cleanup_hw(dev_priv); > - > +err_perf: > + i915_perf_fini(dev_priv); > return ret; > } >
On Sat, 14 Apr 2018 11:12:33 +0200, Chris Wilson <chris@chris-wilson.co.uk> wrote: > We have to cleanup after i915_perf_init(), even on the error path, as it > passes a pointer into the module to the sysfs core. If we fail to > unregister the sysctl table, we leave a dangling pointer which then may > explode anytime later the sysctl table, we leave a dangling pointer > which then may explode anytime later. no need to repeat, we're smart enough to get it after first time ;) > > Fixes: 9f9b2792b6d3 ("drm/i915/perf: reuse timestamp frequency from > device info") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index 9944a03cdea6..beac565ddfe0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1109,30 +1109,30 @@ static int i915_driver_init_hw(struct > drm_i915_private *dev_priv) > ret = i915_ggtt_probe_hw(dev_priv); > if (ret) > - return ret; > + goto err_perf; > /* WARNING: Apparently we must kick fbdev drivers before vgacon, > * otherwise the vga fbdev driver falls over. */ btw, as you're doing labels cleanup, maybe you can also fix above comment to follow multi-line style? > ret = i915_kick_out_firmware_fb(dev_priv); > if (ret) { > DRM_ERROR("failed to remove conflicting framebuffer drivers\n"); > - goto out_ggtt; > + goto err_ggtt; > } > ret = i915_kick_out_vgacon(dev_priv); > if (ret) { > DRM_ERROR("failed to remove conflicting VGA console\n"); > - goto out_ggtt; > + goto err_ggtt; > } > ret = i915_ggtt_init_hw(dev_priv); > if (ret) > - return ret; > + goto err_ggtt; > ret = i915_ggtt_enable_hw(dev_priv); > if (ret) { > DRM_ERROR("failed to enable GGTT\n"); > - goto out_ggtt; > + goto err_ggtt; > } > pci_set_master(pdev); > @@ -1143,7 +1143,7 @@ static int i915_driver_init_hw(struct > drm_i915_private *dev_priv) > if (ret) { > DRM_ERROR("failed to set DMA mask\n"); > - goto out_ggtt; > + goto err_ggtt; > } > } > @@ -1161,7 +1161,7 @@ static int i915_driver_init_hw(struct > drm_i915_private *dev_priv) > if (ret) { > DRM_ERROR("failed to set DMA mask\n"); > - goto out_ggtt; > + goto err_ggtt; > } > } > @@ -1197,13 +1197,14 @@ static int i915_driver_init_hw(struct > drm_i915_private *dev_priv) > ret = intel_gvt_init(dev_priv); > if (ret) > - goto out_ggtt; > + goto err_ggtt; > return 0; > -out_ggtt: > +err_ggtt: > i915_ggtt_cleanup_hw(dev_priv); > - > +err_perf: > + i915_perf_fini(dev_priv); > return ret; > } > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Quoting Michal Wajdeczko (2018-04-14 16:59:13) > On Sat, 14 Apr 2018 11:12:33 +0200, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > We have to cleanup after i915_perf_init(), even on the error path, as it > > passes a pointer into the module to the sysfs core. If we fail to > > unregister the sysctl table, we leave a dangling pointer which then may > > explode anytime later the sysctl table, we leave a dangling pointer > > which then may explode anytime later. > > no need to repeat, we're smart enough to get it after first time ;) Smarter than me for sure, I didn't even notice I hit '.' too late. Fixed up the commitmsg and old comment style, and pushed (and forgot to say I pushed). Thanks all for the review, -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9944a03cdea6..beac565ddfe0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1109,30 +1109,30 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) ret = i915_ggtt_probe_hw(dev_priv); if (ret) - return ret; + goto err_perf; /* WARNING: Apparently we must kick fbdev drivers before vgacon, * otherwise the vga fbdev driver falls over. */ ret = i915_kick_out_firmware_fb(dev_priv); if (ret) { DRM_ERROR("failed to remove conflicting framebuffer drivers\n"); - goto out_ggtt; + goto err_ggtt; } ret = i915_kick_out_vgacon(dev_priv); if (ret) { DRM_ERROR("failed to remove conflicting VGA console\n"); - goto out_ggtt; + goto err_ggtt; } ret = i915_ggtt_init_hw(dev_priv); if (ret) - return ret; + goto err_ggtt; ret = i915_ggtt_enable_hw(dev_priv); if (ret) { DRM_ERROR("failed to enable GGTT\n"); - goto out_ggtt; + goto err_ggtt; } pci_set_master(pdev); @@ -1143,7 +1143,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) if (ret) { DRM_ERROR("failed to set DMA mask\n"); - goto out_ggtt; + goto err_ggtt; } } @@ -1161,7 +1161,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) if (ret) { DRM_ERROR("failed to set DMA mask\n"); - goto out_ggtt; + goto err_ggtt; } } @@ -1197,13 +1197,14 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv) ret = intel_gvt_init(dev_priv); if (ret) - goto out_ggtt; + goto err_ggtt; return 0; -out_ggtt: +err_ggtt: i915_ggtt_cleanup_hw(dev_priv); - +err_perf: + i915_perf_fini(dev_priv); return ret; }
We have to cleanup after i915_perf_init(), even on the error path, as it passes a pointer into the module to the sysfs core. If we fail to unregister the sysctl table, we leave a dangling pointer which then may explode anytime later the sysctl table, we leave a dangling pointer which then may explode anytime later. Fixes: 9f9b2792b6d3 ("drm/i915/perf: reuse timestamp frequency from device info") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Cc: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)