Message ID | 20181025085301.8217-1-david1.zhou@amd.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm: fix call_kern.cocci warnings v2 | expand |
Am 25.10.18 um 10:53 schrieb Chunming Zhou: > drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function drm_syncobj_find_signal_pt_for_point called on line 390 inside lock on line 389 but uses GFP_KERNEL > > Find functions that refer to GFP_KERNEL but are called with locks held. > > Semantic patch information: > The proposed change of converting the GFP_KERNEL is not necessarily the > correct one. It may be desired to unlock the lock, or to not call the > function under the lock in the first place. > > Generated by: scripts/coccinelle/locks/call_kern.cocci That comment is no longer true. > > v2: > syncobj->timeline still needs protect. > > Signed-off-by: Chunming Zhou <david1.zhou@amd.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org > Cc: Christian König <easy2remember.chk@googlemail.com> > --- > drivers/gpu/drm/drm_syncobj.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index b7eaa603f368..b843d4c2778c 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -111,15 +111,16 @@ static struct dma_fence > uint64_t point) > { > struct drm_syncobj_signal_pt *signal_pt; > + struct dma_fence *f = NULL; > + struct drm_syncobj_stub_fence *fence = > + kzalloc(sizeof(struct drm_syncobj_stub_fence), > + GFP_KERNEL); > > + if (!fence) > + return NULL; > + spin_lock(&syncobj->pt_lock); How about using a single static stub fence like I suggested? Christian. > if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) && > (point <= syncobj->timeline)) { > - struct drm_syncobj_stub_fence *fence = > - kzalloc(sizeof(struct drm_syncobj_stub_fence), > - GFP_KERNEL); > - > - if (!fence) > - return NULL; > spin_lock_init(&fence->lock); > dma_fence_init(&fence->base, > &drm_syncobj_stub_fence_ops, > @@ -128,6 +129,7 @@ static struct dma_fence > point); > > dma_fence_signal(&fence->base); > + spin_unlock(&syncobj->pt_lock); > return &fence->base; > } > > @@ -137,9 +139,12 @@ static struct dma_fence > if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) && > (point != signal_pt->value)) > continue; > - return dma_fence_get(&signal_pt->fence_array->base); > + f = dma_fence_get(&signal_pt->fence_array->base); > + break; > } > - return NULL; > + spin_unlock(&syncobj->pt_lock); > + kfree(fence); > + return f; > } > > static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj, > @@ -166,9 +171,7 @@ static void drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, > } > > mutex_lock(&syncobj->cb_mutex); > - spin_lock(&syncobj->pt_lock); > *fence = drm_syncobj_find_signal_pt_for_point(syncobj, pt_value); > - spin_unlock(&syncobj->pt_lock); > if (!*fence) > drm_syncobj_add_callback_locked(syncobj, cb, func); > mutex_unlock(&syncobj->cb_mutex); > @@ -379,11 +382,9 @@ drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags, > if (ret < 0) > return ret; > } > - spin_lock(&syncobj->pt_lock); > *fence = drm_syncobj_find_signal_pt_for_point(syncobj, point); > if (!*fence) > ret = -EINVAL; > - spin_unlock(&syncobj->pt_lock); > return ret; > } >
Am 25.10.18 um 11:03 schrieb zhoucm1: On 2018年10月25日 16:56, Christian König wrote: +++ b/drivers/gpu/drm/drm_syncobj.c @@ -111,15 +111,16 @@ static struct dma_fence uint64_t point) { struct drm_syncobj_signal_pt *signal_pt; + struct dma_fence *f = NULL; + struct drm_syncobj_stub_fence *fence = + kzalloc(sizeof(struct drm_syncobj_stub_fence), + GFP_KERNEL); + if (!fence) + return NULL; + spin_lock(&syncobj->pt_lock); How about using a single static stub fence like I suggested? Sorry, I don't get your meanings, how to do that? Add a new function drm_syncobj_stub_fence_init() which is called from drm_core_init() when the module is loaded. In drm_syncobj_stub_fence_init() you initialize one static stub_fence which is then used over and over again. Since its reference count never goes down to zero it should never be freed. In doubt maybe add a .free callback which just calls BUG() to catch reference count issues. Christian. Thanks, David <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 25.10.18 um 11:03 schrieb zhoucm1:<br> </div> <blockquote type="cite" cite="mid:14a02772-5277-3ef2-91f8-1ef3858c1bbe@amd.com"> <p><br> </p> <br> <div class="moz-cite-prefix">On 2018年10月25日 16:56, Christian König wrote:<br> </div> <blockquote type="cite" cite="mid:6f63db7a-e51b-5829-9eb0-265ebd6dd99c@gmail.com"> <blockquote type="cite" style="color: #000000;">+++ b/drivers/gpu/drm/drm_syncobj.c <br> @@ -111,15 +111,16 @@ static struct dma_fence <br> uint64_t point) <br> { <br> struct drm_syncobj_signal_pt *signal_pt; <br> + struct dma_fence *f = NULL; <br> + struct drm_syncobj_stub_fence *fence = <br> + kzalloc(sizeof(struct drm_syncobj_stub_fence), <br> + GFP_KERNEL); <br> + if (!fence) <br> + return NULL; <br> + spin_lock(&syncobj->pt_lock); <br> </blockquote> <br> How about using a single static stub fence like I suggested? </blockquote> Sorry, I don't get your meanings, how to do that?<br> </blockquote> <br> Add a new function drm_syncobj_stub_fence_init() which is called from drm_core_init() when the module is loaded.<br> <br> In drm_syncobj_stub_fence_init() you initialize one static stub_fence which is then used over and over again.<br> <br> Since its reference count never goes down to zero it should never be freed. In doubt maybe add a .free callback which just calls BUG() to catch reference count issues.<br> <br> Christian.<br> <br> <blockquote type="cite" cite="mid:14a02772-5277-3ef2-91f8-1ef3858c1bbe@amd.com"><br> Thanks,<br> David <br> </blockquote> <br> </body> </html>
Op 25-10-18 om 10:53 schreef Chunming Zhou: > drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function drm_syncobj_find_signal_pt_for_point called on line 390 inside lock on line 389 but uses GFP_KERNEL > > Find functions that refer to GFP_KERNEL but are called with locks held. > > Semantic patch information: > The proposed change of converting the GFP_KERNEL is not necessarily the > correct one. It may be desired to unlock the lock, or to not call the > function under the lock in the first place. > > Generated by: scripts/coccinelle/locks/call_kern.cocci > > v2: > syncobj->timeline still needs protect. > > Signed-off-by: Chunming Zhou <david1.zhou@amd.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org > Cc: Christian König <easy2remember.chk@googlemail.com> > --- > drivers/gpu/drm/drm_syncobj.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index b7eaa603f368..b843d4c2778c 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -111,15 +111,16 @@ static struct dma_fence > uint64_t point) > { > struct drm_syncobj_signal_pt *signal_pt; > + struct dma_fence *f = NULL; > + struct drm_syncobj_stub_fence *fence = > + kzalloc(sizeof(struct drm_syncobj_stub_fence), > + GFP_KERNEL); > > + if (!fence) > + return NULL; > + spin_lock(&syncobj->pt_lock); > if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) && > (point <= syncobj->timeline)) { > - struct drm_syncobj_stub_fence *fence = > - kzalloc(sizeof(struct drm_syncobj_stub_fence), > - GFP_KERNEL); > - > - if (!fence) > - return NULL; > spin_lock_init(&fence->lock); > dma_fence_init(&fence->base, > &drm_syncobj_stub_fence_ops, > @@ -128,6 +129,7 @@ static struct dma_fence > point); > > dma_fence_signal(&fence->base); > + spin_unlock(&syncobj->pt_lock); > return &fence->base; > } > > @@ -137,9 +139,12 @@ static struct dma_fence > if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) && > (point != signal_pt->value)) > continue; > - return dma_fence_get(&signal_pt->fence_array->base); > + f = dma_fence_get(&signal_pt->fence_array->base); > + break; > } > - return NULL; > + spin_unlock(&syncobj->pt_lock); > + kfree(fence); > + return f; > } > > static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj, > @@ -166,9 +171,7 @@ static void drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, > } > > mutex_lock(&syncobj->cb_mutex); > - spin_lock(&syncobj->pt_lock); > *fence = drm_syncobj_find_signal_pt_for_point(syncobj, pt_value); > - spin_unlock(&syncobj->pt_lock); > if (!*fence) > drm_syncobj_add_callback_locked(syncobj, cb, func); > mutex_unlock(&syncobj->cb_mutex); Maybe slightly offtopic, but seems that this function should return an error if fence == NULL, at least when it's a result of -ENOMEM. It would be nice if this was sent as a separate patch, instead of waiting indefinitely.. > @@ -379,11 +382,9 @@ drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags, > if (ret < 0) > return ret; > } > - spin_lock(&syncobj->pt_lock); > *fence = drm_syncobj_find_signal_pt_for_point(syncobj, point); > if (!*fence) > ret = -EINVAL; There's no way to make a destinction between -ENOMEM and -EINVAL, I think it would be good to use ERR_PTR, and then return PTR_ERR_OR_ZERO().. I would also change the subject line to something more descriptive, rather than the generic one it's now. But otherwise looks good to me. :)
Am 25.10.18 um 11:20 schrieb zhoucm1: On 2018年10月25日 17:11, Koenig, Christian wrote: Am 25.10.18 um 11:03 schrieb zhoucm1: On 2018年10月25日 16:56, Christian König wrote: +++ b/drivers/gpu/drm/drm_syncobj.c @@ -111,15 +111,16 @@ static struct dma_fence uint64_t point) { struct drm_syncobj_signal_pt *signal_pt; + struct dma_fence *f = NULL; + struct drm_syncobj_stub_fence *fence = + kzalloc(sizeof(struct drm_syncobj_stub_fence), + GFP_KERNEL); + if (!fence) + return NULL; + spin_lock(&syncobj->pt_lock); How about using a single static stub fence like I suggested? Sorry, I don't get your meanings, how to do that? Add a new function drm_syncobj_stub_fence_init() which is called from drm_core_init() when the module is loaded. In drm_syncobj_stub_fence_init() you initialize one static stub_fence which is then used over and over again. Seems it would not work, we could need more than one stub fence. Mhm, why? I mean it is just a signaled fence, context and sequence number are irrelevant. Christian. David Since its reference count never goes down to zero it should never be freed. In doubt maybe add a .free callback which just calls BUG() to catch reference count issues. Christian. Thanks, David <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 25.10.18 um 11:20 schrieb zhoucm1:<br> </div> <blockquote type="cite" cite="mid:a95c3630-f8b8-b7d4-2354-64d8c091ecb7@amd.com"> <p><br> </p> <br> <div class="moz-cite-prefix">On 2018年10月25日 17:11, Koenig, Christian wrote:<br> </div> <blockquote type="cite" cite="mid:0d33f030-aa41-0cdc-bd47-9018436d3e73@amd.com"> <div class="moz-cite-prefix">Am 25.10.18 um 11:03 schrieb zhoucm1:<br> </div> <blockquote type="cite" cite="mid:14a02772-5277-3ef2-91f8-1ef3858c1bbe@amd.com"> <p><br> </p> <br> <div class="moz-cite-prefix">On 2018年10月25日 16:56, Christian König wrote:<br> </div> <blockquote type="cite" cite="mid:6f63db7a-e51b-5829-9eb0-265ebd6dd99c@gmail.com"> <blockquote type="cite" style="color: #000000;">+++ b/drivers/gpu/drm/drm_syncobj.c <br> @@ -111,15 +111,16 @@ static struct dma_fence <br> uint64_t point) <br> { <br> struct drm_syncobj_signal_pt *signal_pt; <br> + struct dma_fence *f = NULL; <br> + struct drm_syncobj_stub_fence *fence = <br> + kzalloc(sizeof(struct drm_syncobj_stub_fence), <br> + GFP_KERNEL); <br> + if (!fence) <br> + return NULL; <br> + spin_lock(&syncobj->pt_lock); <br> </blockquote> <br> How about using a single static stub fence like I suggested? </blockquote> Sorry, I don't get your meanings, how to do that?<br> </blockquote> <br> Add a new function drm_syncobj_stub_fence_init() which is called from drm_core_init() when the module is loaded.<br> <br> In drm_syncobj_stub_fence_init() you initialize one static stub_fence which is then used over and over again.<br> </blockquote> Seems it would not work, we could need more than one stub fence.<br> </blockquote> <br> Mhm, why? I mean it is just a signaled fence, context and sequence number are irrelevant.<br> <br> Christian.<br> <br> <blockquote type="cite" cite="mid:a95c3630-f8b8-b7d4-2354-64d8c091ecb7@amd.com"><br> David <br> <blockquote type="cite" cite="mid:0d33f030-aa41-0cdc-bd47-9018436d3e73@amd.com"><br> Since its reference count never goes down to zero it should never be freed. In doubt maybe add a .free callback which just calls BUG() to catch reference count issues.<br> <br> Christian.<br> <br> <blockquote type="cite" cite="mid:14a02772-5277-3ef2-91f8-1ef3858c1bbe@amd.com"><br> Thanks,<br> David <br> </blockquote> <br> </blockquote> <br> </blockquote> <br> </body> </html>
Am 25.10.18 um 11:28 schrieb zhoucm1: On 2018年10月25日 17:23, Koenig, Christian wrote: Am 25.10.18 um 11:20 schrieb zhoucm1: On 2018年10月25日 17:11, Koenig, Christian wrote: Am 25.10.18 um 11:03 schrieb zhoucm1: On 2018年10月25日 16:56, Christian König wrote: +++ b/drivers/gpu/drm/drm_syncobj.c @@ -111,15 +111,16 @@ static struct dma_fence uint64_t point) { struct drm_syncobj_signal_pt *signal_pt; + struct dma_fence *f = NULL; + struct drm_syncobj_stub_fence *fence = + kzalloc(sizeof(struct drm_syncobj_stub_fence), + GFP_KERNEL); + if (!fence) + return NULL; + spin_lock(&syncobj->pt_lock); How about using a single static stub fence like I suggested? Sorry, I don't get your meanings, how to do that? Add a new function drm_syncobj_stub_fence_init() which is called from drm_core_init() when the module is loaded. In drm_syncobj_stub_fence_init() you initialize one static stub_fence which is then used over and over again. Seems it would not work, we could need more than one stub fence. Mhm, why? I mean it is just a signaled fence, If A gets the global stub fence, doesn't put it yet, then B is coming, how does B re-use the global stub fence? anything I misunderstand? dma_fence_get()? The whole thing is reference counted, every time you need it you grab another reference. Since we globally initialize it the reference never becomes zero, so it is never released. Christian. David context and sequence number are irrelevant. Christian. David Since its reference count never goes down to zero it should never be freed. In doubt maybe add a .free callback which just calls BUG() to catch reference count issues. Christian. Thanks, David <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 25.10.18 um 11:28 schrieb zhoucm1:<br> </div> <blockquote type="cite" cite="mid:b8d900fc-43e6-e3cc-4573-1f91660be37f@amd.com"> <p><br> </p> <br> <div class="moz-cite-prefix">On 2018年10月25日 17:23, Koenig, Christian wrote:<br> </div> <blockquote type="cite" cite="mid:01456f6c-88c3-5b4e-bc07-d3a202ae2dd3@amd.com"> <div class="moz-cite-prefix">Am 25.10.18 um 11:20 schrieb zhoucm1:<br> </div> <blockquote type="cite" cite="mid:a95c3630-f8b8-b7d4-2354-64d8c091ecb7@amd.com"> <p><br> </p> <br> <div class="moz-cite-prefix">On 2018年10月25日 17:11, Koenig, Christian wrote:<br> </div> <blockquote type="cite" cite="mid:0d33f030-aa41-0cdc-bd47-9018436d3e73@amd.com"> <div class="moz-cite-prefix">Am 25.10.18 um 11:03 schrieb zhoucm1:<br> </div> <blockquote type="cite" cite="mid:14a02772-5277-3ef2-91f8-1ef3858c1bbe@amd.com"> <p><br> </p> <br> <div class="moz-cite-prefix">On 2018年10月25日 16:56, Christian König wrote:<br> </div> <blockquote type="cite" cite="mid:6f63db7a-e51b-5829-9eb0-265ebd6dd99c@gmail.com"> <blockquote type="cite" style="color: #000000;">+++ b/drivers/gpu/drm/drm_syncobj.c <br> @@ -111,15 +111,16 @@ static struct dma_fence <br> uint64_t point) <br> { <br> struct drm_syncobj_signal_pt *signal_pt; <br> + struct dma_fence *f = NULL; <br> + struct drm_syncobj_stub_fence *fence = <br> + kzalloc(sizeof(struct drm_syncobj_stub_fence), <br> + GFP_KERNEL); <br> + if (!fence) <br> + return NULL; <br> + spin_lock(&syncobj->pt_lock); <br> </blockquote> <br> How about using a single static stub fence like I suggested? </blockquote> Sorry, I don't get your meanings, how to do that?<br> </blockquote> <br> Add a new function drm_syncobj_stub_fence_init() which is called from drm_core_init() when the module is loaded.<br> <br> In drm_syncobj_stub_fence_init() you initialize one static stub_fence which is then used over and over again.<br> </blockquote> Seems it would not work, we could need more than one stub fence.<br> </blockquote> <br> Mhm, why? I mean it is just a signaled fence,</blockquote> <br> If A gets the global stub fence, doesn't put it yet, then B is coming, how does B re-use the global stub fence? anything I misunderstand?<br> </blockquote> <br> dma_fence_get()? The whole thing is reference counted, every time you need it you grab another reference.<br> <br> Since we globally initialize it the reference never becomes zero, so it is never released.<br> <br> Christian.<br> <br> <blockquote type="cite" cite="mid:b8d900fc-43e6-e3cc-4573-1f91660be37f@amd.com"><br> David<br> <blockquote type="cite" cite="mid:01456f6c-88c3-5b4e-bc07-d3a202ae2dd3@amd.com">context and sequence number are irrelevant.<br> <br> Christian.<br> <br> <blockquote type="cite" cite="mid:a95c3630-f8b8-b7d4-2354-64d8c091ecb7@amd.com"><br> David <br> <blockquote type="cite" cite="mid:0d33f030-aa41-0cdc-bd47-9018436d3e73@amd.com"><br> Since its reference count never goes down to zero it should never be freed. In doubt maybe add a .free callback which just calls BUG() to catch reference count issues.<br> <br> Christian.<br> <br> <blockquote type="cite" cite="mid:14a02772-5277-3ef2-91f8-1ef3858c1bbe@amd.com"><br> Thanks,<br> David <br> </blockquote> <br> </blockquote> <br> </blockquote> <br> </blockquote> <br> </blockquote> <br> </body> </html>
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index b7eaa603f368..b843d4c2778c 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -111,15 +111,16 @@ static struct dma_fence uint64_t point) { struct drm_syncobj_signal_pt *signal_pt; + struct dma_fence *f = NULL; + struct drm_syncobj_stub_fence *fence = + kzalloc(sizeof(struct drm_syncobj_stub_fence), + GFP_KERNEL); + if (!fence) + return NULL; + spin_lock(&syncobj->pt_lock); if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) && (point <= syncobj->timeline)) { - struct drm_syncobj_stub_fence *fence = - kzalloc(sizeof(struct drm_syncobj_stub_fence), - GFP_KERNEL); - - if (!fence) - return NULL; spin_lock_init(&fence->lock); dma_fence_init(&fence->base, &drm_syncobj_stub_fence_ops, @@ -128,6 +129,7 @@ static struct dma_fence point); dma_fence_signal(&fence->base); + spin_unlock(&syncobj->pt_lock); return &fence->base; } @@ -137,9 +139,12 @@ static struct dma_fence if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) && (point != signal_pt->value)) continue; - return dma_fence_get(&signal_pt->fence_array->base); + f = dma_fence_get(&signal_pt->fence_array->base); + break; } - return NULL; + spin_unlock(&syncobj->pt_lock); + kfree(fence); + return f; } static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj, @@ -166,9 +171,7 @@ static void drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, } mutex_lock(&syncobj->cb_mutex); - spin_lock(&syncobj->pt_lock); *fence = drm_syncobj_find_signal_pt_for_point(syncobj, pt_value); - spin_unlock(&syncobj->pt_lock); if (!*fence) drm_syncobj_add_callback_locked(syncobj, cb, func); mutex_unlock(&syncobj->cb_mutex); @@ -379,11 +382,9 @@ drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags, if (ret < 0) return ret; } - spin_lock(&syncobj->pt_lock); *fence = drm_syncobj_find_signal_pt_for_point(syncobj, point); if (!*fence) ret = -EINVAL; - spin_unlock(&syncobj->pt_lock); return ret; }
drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function drm_syncobj_find_signal_pt_for_point called on line 390 inside lock on line 389 but uses GFP_KERNEL Find functions that refer to GFP_KERNEL but are called with locks held. Semantic patch information: The proposed change of converting the GFP_KERNEL is not necessarily the correct one. It may be desired to unlock the lock, or to not call the function under the lock in the first place. Generated by: scripts/coccinelle/locks/call_kern.cocci v2: syncobj->timeline still needs protect. Signed-off-by: Chunming Zhou <david1.zhou@amd.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org Cc: Christian König <easy2remember.chk@googlemail.com> --- drivers/gpu/drm/drm_syncobj.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)