On 15/01/2026 17:45, Liviu Dudau wrote: > On Thu, Jan 15, 2026 at 06:27:00PM +0100, Boris Brezillon wrote: >> On Thu, 15 Jan 2026 16:51:31 +0000 >> Liviu Dudau <[email protected]> wrote: >> >>> On Fri, Jan 09, 2026 at 02:07:57PM +0100, Boris Brezillon wrote: [...] >>>> +static void * >>>> +panthor_gem_vmap_get_locked(struct panthor_gem_object *bo) >>>> +{ >>>> + pgprot_t prot = PAGE_KERNEL; >>>> + void *vaddr; >>>> + int ret; >>>> + >>>> + dma_resv_assert_held(bo->base.resv); >>>> + >>>> + if (drm_WARN_ON_ONCE(bo->base.dev, drm_gem_is_imported(&bo->base))) >>>> + return ERR_PTR(-EINVAL); >>>> + >>>> + if (refcount_inc_not_zero(&bo->cmap.vaddr_use_count)) { >>>> + drm_WARN_ON_ONCE(bo->base.dev, !bo->cmap.vaddr); >>> >>> Is this drm_WARN_ON_ONCE() necessary? I can't see how we can ever trigger >>> it. >> >> I know it's not supposed to happen, but isn't WARN_ON() exactly about >> catching unexpected situations? :p > > Agree, but I'm not convinced the WARN_ON() can be triggered at all as > cmap.vaddr > should not be zero if cmap.vaddr_use_count is non-zero.
If you can ever see how a WARN_ON can be triggered, then there's a bug to fix. If we went around removing WARN_ON()s that we think cannot be triggered then we'd have no WARN_ON()s left. A better argument here is that we could handle the error. At the moment we'd end up returning NULL, but the caller (panthor_gem_vmap_locked()) checks IS_ERR. So either we could return a ERR_PTR in this case, or handle the NULL in the caller. [Although given this is a "can never happen" case I don't tend to worry too much about the error handling...] Thanks, Steve
