Hi Steven, On 14.04.2025 10:50, Steven Price wrote: > Hi Adrián, > > On 11/04/2025 16:03, Adrián Larumbe wrote: > > Add a new character string Panthor BO field, and a function that allows > > setting it from within the driver. > > > > Driver takes care of freeing the string when it's replaced or no longer > > needed at object destruction time, but allocating it is the responsibility > > of callers. > > > > Signed-off-by: Adrián Larumbe <adrian.laru...@collabora.com> > > Reviewed-by: Boris Brezillon <boris.brezil...@collabora.com> > > --- > > drivers/gpu/drm/panthor/panthor_gem.c | 39 +++++++++++++++++++++++++++ > > drivers/gpu/drm/panthor/panthor_gem.h | 17 ++++++++++++ > > 2 files changed, 56 insertions(+) > > > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c > > b/drivers/gpu/drm/panthor/panthor_gem.c > > index 8244a4e6c2a2..af0ac17f357f 100644 > > --- a/drivers/gpu/drm/panthor/panthor_gem.c > > +++ b/drivers/gpu/drm/panthor/panthor_gem.c > > @@ -2,6 +2,7 @@ > > /* Copyright 2019 Linaro, Ltd, Rob Herring <r...@kernel.org> */ > > /* Copyright 2023 Collabora ltd. */ > > > > +#include <linux/cleanup.h> > > #include <linux/dma-buf.h> > > #include <linux/dma-mapping.h> > > #include <linux/err.h> > > @@ -18,6 +19,14 @@ static void panthor_gem_free_object(struct > > drm_gem_object *obj) > > struct panthor_gem_object *bo = to_panthor_bo(obj); > > struct drm_gem_object *vm_root_gem = bo->exclusive_vm_root_gem; > > > > + /* > > + * Label might have been allocated with kstrdup_const(), > > + * we need to take that into account when freeing the memory > > + */ > > + kfree_const(bo->label.str); > > + > > + mutex_destroy(&bo->label.lock); > > + > > drm_gem_free_mmap_offset(&bo->base.base); > > mutex_destroy(&bo->gpuva_list_lock); > > drm_gem_shmem_free(&bo->base); > > @@ -196,6 +205,7 @@ struct drm_gem_object *panthor_gem_create_object(struct > > drm_device *ddev, size_t > > obj->base.map_wc = !ptdev->coherent; > > mutex_init(&obj->gpuva_list_lock); > > drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock); > > + mutex_init(&obj->label.lock); > > > > return &obj->base.base; > > } > > @@ -247,3 +257,32 @@ panthor_gem_create_with_handle(struct drm_file *file, > > > > return ret; > > } > > + > > +void > > +panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label) > > +{ > > + struct panthor_gem_object *bo = to_panthor_bo(obj); > > + const char *old_label; > > + > > + scoped_guard(mutex, &bo->label.lock) { > > + old_label = bo->label.str; > > + bo->label.str = label; > > + } > > + > > + kfree(old_label); > > Shouldn't this be kfree_const()? I suspect as things stand we can't > trigger this bug but it's inconsistent.
This could only be called either from the set_label() ioctl, in which case old_label could be NULL or a pointer to a string obtained from strdup(); or from panthor_gem_kernel_bo_set_label(). In the latter case, it could only ever be NULL, since we don't reassign kernel BO labels, so it'd be safe too. However I do agree that it's not consistent, and then in the future perhaps relabelling kernel BO's might be justified, so I'll change it to kfree_const(). > > +} > > + > > +void > > +panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char > > *label) > > +{ > > + const char *str; > > + > > + str = kstrdup_const(label, GFP_KERNEL); > > + if (!str) { > > In the next patch you permit user space to clear the label > (args->size==0) which means label==NULL and AFAICT kstrdup_const() will > return NULL in this case triggering this warning. Kernel and UM-exposed BO's should never experience cross-labelling, so in theory this scenario ought to be impossible. The only way panthor_gem_kernel_bo_set_label() might take NULL in the 'label' argument is that someone called panthor_kernel_bo_create() with NULL for its name 'argument'. I think as a defensive check, I could do something as follows void panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label) { const char *str; /* We should never attempt labelling a UM-exposed GEM object */ if (drm_WARN_ON(bo->obj->dev, &bo->obj->handle_count > 0)) return; if (!label) return; [...] } > Thanks, > Steve > > > + /* Failing to allocate memory for a label isn't a fatal > > condition */ > > + drm_warn(bo->obj->dev, "Not enough memory to allocate BO > > label"); > > + return; > > + } > > + > > + panthor_gem_bo_set_label(bo->obj, str); > > +} > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h > > b/drivers/gpu/drm/panthor/panthor_gem.h > > index 1a363bb814f4..af0d77338860 100644 > > --- a/drivers/gpu/drm/panthor/panthor_gem.h > > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > > @@ -46,6 +46,20 @@ struct panthor_gem_object { > > > > /** @flags: Combination of drm_panthor_bo_flags flags. */ > > u32 flags; > > + > > + /** > > + * @label: BO tagging fields. The label can be assigned within the > > + * driver itself or through a specific IOCTL. > > + */ > > + struct { > > + /** > > + * @label.str: Pointer to NULL-terminated string, > > + */ > > + const char *str; > > + > > + /** @lock.str: Protects access to the @label.str field. */ > > + struct mutex lock; > > + } label; > > }; > > > > /** > > @@ -91,6 +105,9 @@ panthor_gem_create_with_handle(struct drm_file *file, > > struct panthor_vm *exclusive_vm, > > u64 *size, u32 flags, uint32_t *handle); > > > > +void panthor_gem_bo_set_label(struct drm_gem_object *obj, const char > > *label); > > +void panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const > > char *label); > > + > > static inline u64 > > panthor_kernel_bo_gpuva(struct panthor_kernel_bo *bo) > > { Adrian Larumbe