| Message ID | 20240911085903.1496-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] dma-buf: add WARN_ON() illegal dma-fence signaling | expand |
On Wed, 2024-09-11 at 10:58 +0200, Christian König wrote: > Calling the signaling a NULL fence is obviously a coding error in a > driver. Those functions unfortunately just returned silently without > raising a warning. Good catch > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-fence.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > fence.c > index 0393a9bba3a8..325a263ac798 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -412,7 +412,7 @@ int dma_fence_signal_timestamp(struct dma_fence > *fence, ktime_t timestamp) > unsigned long flags; > int ret; > > - if (!fence) > + if (WARN_ON(!fence)) > return -EINVAL; While one can do that, as far as I can see there are only a hand full of users of that function anyways. Couldn't one (additionally) add the error check of dma_fenc_signal_timestapm() to those? Like in dma_fenc_allocate_private_stub(). It seems some of them are void functions, though. Hm. There is also the attribute __must_check that could be considered now or in the future for such functions. Regards, P. > > spin_lock_irqsave(fence->lock, flags); > @@ -464,7 +464,7 @@ int dma_fence_signal(struct dma_fence *fence) > int ret; > bool tmp; > > - if (!fence) > + if (WARN_ON(!fence)) > return -EINVAL; > > tmp = dma_fence_begin_signalling();
Am 11.09.24 um 11:44 schrieb Philipp Stanner: > On Wed, 2024-09-11 at 10:58 +0200, Christian König wrote: >> Calling the signaling a NULL fence is obviously a coding error in a >> driver. Those functions unfortunately just returned silently without >> raising a warning. > Good catch > >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/dma-fence.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- >> fence.c >> index 0393a9bba3a8..325a263ac798 100644 >> --- a/drivers/dma-buf/dma-fence.c >> +++ b/drivers/dma-buf/dma-fence.c >> @@ -412,7 +412,7 @@ int dma_fence_signal_timestamp(struct dma_fence >> *fence, ktime_t timestamp) >> unsigned long flags; >> int ret; >> >> - if (!fence) >> + if (WARN_ON(!fence)) >> return -EINVAL; > While one can do that, as far as I can see there are only a hand full > of users of that function anyways. The dma_fence_signal() function has tons of users, it's basically the core of the DMA-buf framework. > Couldn't one (additionally) add the error check of > dma_fenc_signal_timestapm() to those? Like in > dma_fenc_allocate_private_stub(). > > It seems some of them are void functions, though. Hm. > There is also the attribute __must_check that could be considered now > or in the future for such functions. I actually want to remove the error return from dma_fence_signal() and the other variants. There is no valid reason that those functions should fail. The only user is some obscure use case in AMDs KFD driver and I would rather like to clean that one up. Regards, Christian. > > Regards, > P. > > >> >> spin_lock_irqsave(fence->lock, flags); >> @@ -464,7 +464,7 @@ int dma_fence_signal(struct dma_fence *fence) >> int ret; >> bool tmp; >> >> - if (!fence) >> + if (WARN_ON(!fence)) >> return -EINVAL; >> >> tmp = dma_fence_begin_signalling();
On Thu, 2024-09-12 at 16:55 +0200, Christian König wrote: > Am 11.09.24 um 11:44 schrieb Philipp Stanner: > > On Wed, 2024-09-11 at 10:58 +0200, Christian König wrote: > > > Calling the signaling a NULL fence is obviously a coding error in > > > a > > > driver. Those functions unfortunately just returned silently > > > without > > > raising a warning. > > Good catch > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > --- > > > drivers/dma-buf/dma-fence.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > > fence.c > > > index 0393a9bba3a8..325a263ac798 100644 > > > --- a/drivers/dma-buf/dma-fence.c > > > +++ b/drivers/dma-buf/dma-fence.c > > > @@ -412,7 +412,7 @@ int dma_fence_signal_timestamp(struct > > > dma_fence > > > *fence, ktime_t timestamp) > > > unsigned long flags; > > > int ret; > > > > > > - if (!fence) > > > + if (WARN_ON(!fence)) > > > return -EINVAL; > > While one can do that, as far as I can see there are only a hand > > full > > of users of that function anyways. > > The dma_fence_signal() function has tons of users, it's basically the > core of the DMA-buf framework. I meant dma_fence_signal_timestamp() itself. > > > Couldn't one (additionally) add the error check of > > dma_fenc_signal_timestapm() to those? Like in > > dma_fenc_allocate_private_stub(). > > > > It seems some of them are void functions, though. Hm. > > There is also the attribute __must_check that could be considered > > now > > or in the future for such functions. > > I actually want to remove the error return from dma_fence_signal() > and > the other variants. There is no valid reason that those functions > should > fail. Makes sense to me. +1 P. > > The only user is some obscure use case in AMDs KFD driver and I would > rather like to clean that one up. > > Regards, > Christian. > > > > > Regards, > > P. > > > > > > > > > > spin_lock_irqsave(fence->lock, flags); > > > @@ -464,7 +464,7 @@ int dma_fence_signal(struct dma_fence *fence) > > > int ret; > > > bool tmp; > > > > > > - if (!fence) > > > + if (WARN_ON(!fence)) > > > return -EINVAL; > > > > > > tmp = dma_fence_begin_signalling(); >
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 0393a9bba3a8..325a263ac798 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -412,7 +412,7 @@ int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp) unsigned long flags; int ret; - if (!fence) + if (WARN_ON(!fence)) return -EINVAL; spin_lock_irqsave(fence->lock, flags); @@ -464,7 +464,7 @@ int dma_fence_signal(struct dma_fence *fence) int ret; bool tmp; - if (!fence) + if (WARN_ON(!fence)) return -EINVAL; tmp = dma_fence_begin_signalling();
Calling the signaling a NULL fence is obviously a coding error in a driver. Those functions unfortunately just returned silently without raising a warning. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-fence.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)