| Message ID | alpine.DEB.2.21.1810241955110.3776@hadrien (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fix call_kern.cocci warnings (fwd) | expand |
Reviewed-by: Chunming Zhou <david1.zhou@amd.com> > -----Original Message----- > From: Julia Lawall <julia.lawall@lip6.fr> > Sent: Thursday, October 25, 2018 2:57 AM > To: Zhou, David(ChunMing) <David1.Zhou@amd.com> > Cc: kbuild-all@01.org; intel-gfx@lists.freedesktop.org; dri- > devel@lists.freedesktop.org; Christian König > <easy2remember.chk@googlemail.com>; Gustavo Padovan > <gustavo@padovan.org>; Maarten Lankhorst > <maarten.lankhorst@linux.intel.com>; Sean Paul <sean@poorly.run>; David > Airlie <airlied@linux.ie>; dri-devel@lists.freedesktop.org; linux- > kernel@vger.kernel.org > Subject: [PATCH] drm: fix call_kern.cocci warnings (fwd) > > The containing function is called with a spin_lock held, so GFP_KERNEL can't > be used. > > julia > > ---------- Forwarded message ---------- > Date: Tue, 23 Oct 2018 17:14:25 +0800 > From: kbuild test robot <lkp@intel.com> > To: kbuild@01.org > Cc: Julia Lawall <julia.lawall@lip6.fr> > Subject: [PATCH] drm: fix call_kern.cocci warnings > > CC: kbuild-all@01.org > CC: intel-gfx@lists.freedesktop.org > CC: dri-devel@lists.freedesktop.org > TO: Chunming Zhou <david1.zhou@amd.com> > CC: "Christian König" <easy2remember.chk@googlemail.com> > CC: Gustavo Padovan <gustavo@padovan.org> > CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > CC: Sean Paul <sean@poorly.run> > CC: David Airlie <airlied@linux.ie> > CC: dri-devel@lists.freedesktop.org > CC: linux-kernel@vger.kernel.org > > From: kbuild test robot <fengguang.wu@intel.com> > > 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 > > Fixes: 48197bc564c7 ("drm: add syncobj timeline support v9") > CC: Chunming Zhou <david1.zhou@amd.com> > Signed-off-by: kbuild test robot <fengguang.wu@intel.com> > --- > > tree: git://anongit.freedesktop.org/drm/drm-tip drm-tip > head: 8d7ffd2298c607c3e1a16f94d51450d7940fd6a7 > commit: 48197bc564c7a1888c86024a1ba4f956e0ec2300 [1968/2033] drm: add > syncobj timeline support v9 > :::::: branch date: 4 hours ago > :::::: commit date: 5 days ago > > Please take the patch only if it's a positive warning. Thanks! > > drm_syncobj.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -199,7 +199,7 @@ static struct dma_fence > (point <= syncobj->timeline)) { > struct drm_syncobj_stub_fence *fence = > kzalloc(sizeof(struct drm_syncobj_stub_fence), > - GFP_KERNEL); > + GFP_ATOMIC); > > if (!fence) > return NULL;
Am 25.10.18 um 03:28 schrieb Zhou, David(ChunMing): > Reviewed-by: Chunming Zhou <david1.zhou@amd.com> NAK, GFP_ATOMIC should be avoided. The correct solution is to move the allocation out of the spinlock or drop the lock and reacquire. Christian. > >> -----Original Message----- >> From: Julia Lawall <julia.lawall@lip6.fr> >> Sent: Thursday, October 25, 2018 2:57 AM >> To: Zhou, David(ChunMing) <David1.Zhou@amd.com> >> Cc: kbuild-all@01.org; intel-gfx@lists.freedesktop.org; dri- >> devel@lists.freedesktop.org; Christian König >> <easy2remember.chk@googlemail.com>; Gustavo Padovan >> <gustavo@padovan.org>; Maarten Lankhorst >> <maarten.lankhorst@linux.intel.com>; Sean Paul <sean@poorly.run>; David >> Airlie <airlied@linux.ie>; dri-devel@lists.freedesktop.org; linux- >> kernel@vger.kernel.org >> Subject: [PATCH] drm: fix call_kern.cocci warnings (fwd) >> >> The containing function is called with a spin_lock held, so GFP_KERNEL can't >> be used. >> >> julia >> >> ---------- Forwarded message ---------- >> Date: Tue, 23 Oct 2018 17:14:25 +0800 >> From: kbuild test robot <lkp@intel.com> >> To: kbuild@01.org >> Cc: Julia Lawall <julia.lawall@lip6.fr> >> Subject: [PATCH] drm: fix call_kern.cocci warnings >> >> CC: kbuild-all@01.org >> CC: intel-gfx@lists.freedesktop.org >> CC: dri-devel@lists.freedesktop.org >> TO: Chunming Zhou <david1.zhou@amd.com> >> CC: "Christian König" <easy2remember.chk@googlemail.com> >> CC: Gustavo Padovan <gustavo@padovan.org> >> CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> CC: Sean Paul <sean@poorly.run> >> CC: David Airlie <airlied@linux.ie> >> CC: dri-devel@lists.freedesktop.org >> CC: linux-kernel@vger.kernel.org >> >> From: kbuild test robot <fengguang.wu@intel.com> >> >> 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 >> >> Fixes: 48197bc564c7 ("drm: add syncobj timeline support v9") >> CC: Chunming Zhou <david1.zhou@amd.com> >> Signed-off-by: kbuild test robot <fengguang.wu@intel.com> >> --- >> >> tree: git://anongit.freedesktop.org/drm/drm-tip drm-tip >> head: 8d7ffd2298c607c3e1a16f94d51450d7940fd6a7 >> commit: 48197bc564c7a1888c86024a1ba4f956e0ec2300 [1968/2033] drm: add >> syncobj timeline support v9 >> :::::: branch date: 4 hours ago >> :::::: commit date: 5 days ago >> >> Please take the patch only if it's a positive warning. Thanks! >> >> drm_syncobj.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> --- a/drivers/gpu/drm/drm_syncobj.c >> +++ b/drivers/gpu/drm/drm_syncobj.c >> @@ -199,7 +199,7 @@ static struct dma_fence >> (point <= syncobj->timeline)) { >> struct drm_syncobj_stub_fence *fence = >> kzalloc(sizeof(struct drm_syncobj_stub_fence), >> - GFP_KERNEL); >> + GFP_ATOMIC); >> >> if (!fence) >> return NULL;
Op 24-10-18 om 20:57 schreef Julia Lawall: > The containing function is called with a spin_lock held, so GFP_KERNEL > can't be used. > > julia > > ---------- Forwarded message ---------- > Date: Tue, 23 Oct 2018 17:14:25 +0800 > From: kbuild test robot <lkp@intel.com> > To: kbuild@01.org > Cc: Julia Lawall <julia.lawall@lip6.fr> > Subject: [PATCH] drm: fix call_kern.cocci warnings > > CC: kbuild-all@01.org > CC: intel-gfx@lists.freedesktop.org > CC: dri-devel@lists.freedesktop.org > TO: Chunming Zhou <david1.zhou@amd.com> > CC: "Christian König" <easy2remember.chk@googlemail.com> > CC: Gustavo Padovan <gustavo@padovan.org> > CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > CC: Sean Paul <sean@poorly.run> > CC: David Airlie <airlied@linux.ie> > CC: dri-devel@lists.freedesktop.org > CC: linux-kernel@vger.kernel.org > > From: kbuild test robot <fengguang.wu@intel.com> > > 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 > > Fixes: 48197bc564c7 ("drm: add syncobj timeline support v9") > CC: Chunming Zhou <david1.zhou@amd.com> > Signed-off-by: kbuild test robot <fengguang.wu@intel.com> The issue appears to be real and the patch looks sane. Chunming Zhou, do you want to fix it like this, or preallocate a fence obj? If former, just ack. :) ~Maarten > > tree: git://anongit.freedesktop.org/drm/drm-tip drm-tip > head: 8d7ffd2298c607c3e1a16f94d51450d7940fd6a7 > commit: 48197bc564c7a1888c86024a1ba4f956e0ec2300 [1968/2033] drm: add syncobj timeline support v9 > :::::: branch date: 4 hours ago > :::::: commit date: 5 days ago > > Please take the patch only if it's a positive warning. Thanks! > > drm_syncobj.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -199,7 +199,7 @@ static struct dma_fence > (point <= syncobj->timeline)) { > struct drm_syncobj_stub_fence *fence = > kzalloc(sizeof(struct drm_syncobj_stub_fence), > - GFP_KERNEL); > + GFP_ATOMIC); > > if (!fence) > return NULL;
Op 25-10-18 om 08:53 schreef Christian König: > Am 25.10.18 um 03:28 schrieb Zhou, David(ChunMing): >> Reviewed-by: Chunming Zhou <david1.zhou@amd.com> > > NAK, GFP_ATOMIC should be avoided. > > The correct solution is to move the allocation out of the spinlock or drop the lock and reacquire. Yeah +1. Especially in a case like this where it's obvious to prevent. :)
Am 25.10.18 um 09:50 schrieb Maarten Lankhorst: > Op 24-10-18 om 20:57 schreef Julia Lawall: >> The containing function is called with a spin_lock held, so GFP_KERNEL >> can't be used. >> >> julia >> >> ---------- Forwarded message ---------- >> Date: Tue, 23 Oct 2018 17:14:25 +0800 >> From: kbuild test robot <lkp@intel.com> >> To: kbuild@01.org >> Cc: Julia Lawall <julia.lawall@lip6.fr> >> Subject: [PATCH] drm: fix call_kern.cocci warnings >> >> CC: kbuild-all@01.org >> CC: intel-gfx@lists.freedesktop.org >> CC: dri-devel@lists.freedesktop.org >> TO: Chunming Zhou <david1.zhou@amd.com> >> CC: "Christian König" <easy2remember.chk@googlemail.com> >> CC: Gustavo Padovan <gustavo@padovan.org> >> CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> CC: Sean Paul <sean@poorly.run> >> CC: David Airlie <airlied@linux.ie> >> CC: dri-devel@lists.freedesktop.org >> CC: linux-kernel@vger.kernel.org >> >> From: kbuild test robot <fengguang.wu@intel.com> >> >> 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 >> >> Fixes: 48197bc564c7 ("drm: add syncobj timeline support v9") >> CC: Chunming Zhou <david1.zhou@amd.com> >> Signed-off-by: kbuild test robot <fengguang.wu@intel.com> > The issue appears to be real and the patch looks sane. Chunming Zhou, do you want to fix it like this, or preallocate > a fence obj? If former, just ack. :) Well, wait a second with that advise1 The problem here is that userspace (indirectly) controls when this allocation is made. So what you could do is to construct some code to force the kernel into doing a *lot* of GFP_ATOMIC allocations. And when the kernel runs out of GFP_ATOMIC space, then well bad things start to happen :) Christian. > > ~Maarten >> tree: git://anongit.freedesktop.org/drm/drm-tip drm-tip >> head: 8d7ffd2298c607c3e1a16f94d51450d7940fd6a7 >> commit: 48197bc564c7a1888c86024a1ba4f956e0ec2300 [1968/2033] drm: add syncobj timeline support v9 >> :::::: branch date: 4 hours ago >> :::::: commit date: 5 days ago >> >> Please take the patch only if it's a positive warning. Thanks! >> >> drm_syncobj.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> --- a/drivers/gpu/drm/drm_syncobj.c >> +++ b/drivers/gpu/drm/drm_syncobj.c >> @@ -199,7 +199,7 @@ static struct dma_fence >> (point <= syncobj->timeline)) { >> struct drm_syncobj_stub_fence *fence = >> kzalloc(sizeof(struct drm_syncobj_stub_fence), >> - GFP_KERNEL); >> + GFP_ATOMIC); >> >> if (!fence) >> return NULL; >
Am 25.10.18 um 09:51 schrieb Maarten Lankhorst: > Op 25-10-18 om 08:53 schreef Christian König: >> Am 25.10.18 um 03:28 schrieb Zhou, David(ChunMing): >>> Reviewed-by: Chunming Zhou <david1.zhou@amd.com> >> NAK, GFP_ATOMIC should be avoided. >> >> The correct solution is to move the allocation out of the spinlock or drop the lock and reacquire. > Yeah +1. Especially in a case like this where it's obvious to prevent. :) Another possibility would to not allocate the dummy fence at all. E.g. we just need a global instance of that which is always signaled and has a reference count of +1. Christian.
will send a fix soon. Thanks, David On 2018年10月25日 15:57, Koenig, Christian wrote: > Am 25.10.18 um 09:51 schrieb Maarten Lankhorst: >> Op 25-10-18 om 08:53 schreef Christian König: >>> Am 25.10.18 um 03:28 schrieb Zhou, David(ChunMing): >>>> Reviewed-by: Chunming Zhou <david1.zhou@amd.com> >>> NAK, GFP_ATOMIC should be avoided. >>> >>> The correct solution is to move the allocation out of the spinlock or drop the lock and reacquire. >> Yeah +1. Especially in a case like this where it's obvious to prevent. :) > Another possibility would to not allocate the dummy fence at all. > > E.g. we just need a global instance of that which is always signaled and > has a reference count of +1. > > Christian.
--- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -199,7 +199,7 @@ static struct dma_fence (point <= syncobj->timeline)) { struct drm_syncobj_stub_fence *fence = kzalloc(sizeof(struct drm_syncobj_stub_fence), - GFP_KERNEL); + GFP_ATOMIC); if (!fence) return NULL;