Re: [Intel-gfx] [PATCH] drm/i915: Call i915_perf_fini() on init_hw error unwind

2018-04-17 Thread Chris Wilson 
Quoting Michal Wajdeczko (2018-04-14 16:59:13)
> On Sat, 14 Apr 2018 11:12:33 +0200, 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.
> 
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Call i915_perf_fini() on init_hw error unwind

2018-04-14 Thread Michal Wajdeczko 
On Sat, 14 Apr 2018 11:12:33 +0200, 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.


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 
Cc: Lionel Landwerlin 
Cc: Matthew Auld 
---
 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 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Call i915_perf_fini() on init_hw error unwind

2018-04-14 Thread Lionel Landwerlin 

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 
Cc: Lionel Landwerlin 
Cc: Matthew Auld 


Reviewed-by: Lionel Landwerlin 

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;
  }
  



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Call i915_perf_fini() on init_hw error unwind

2018-04-14 Thread Chris Wilson 
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 
Cc: Lionel Landwerlin 
Cc: Matthew Auld 
---
 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;
 }
 
-- 
2.17.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx