Re: [PATCH v5] staging: media: atomisp: remove debug sysfs attributes active_bo and free_bo

[Hans de Goede <hansg@kernel.org>]

Hi,

On 27-Jun-25 12:06 PM, Abdelrahman Fekry wrote:
> The sysfs attributes active_bo and free_bo expose internal buffer
> state used only for debugging purposes. These are not part of
> any standard kernel ABI, and need to be removed before this
> driver may be moved out of drivers/staging.
> 
> - Remove active_bo and free_bo attributes
> - Remove group registration calls form hmm_init() and hmm_cleanup()
> 
> Suggested-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

I have merged this in my media-atomisp branch:
https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

And this patch will be included in my next
pull-request to Mauro (to media subsystem maintainer)

Regards,

Hans




> ---
> v5:
> - remove stray change
> 
> v4: https://lore.kernel.org/all/20250627072939.21447-1-abdelrahmanfekry375@gmail.com/
> - fix typos
> 
> v3: https://lore.kernel.org/all/20250624170746.47188-1-abdelrahmanfekry375@gmail.com/
> - remove blank line
> 
> v2: https://lore.kernel.org/all/20250624144943.39297-1-abdelrahmanfekry375@gmail.com/
> - add Suggested-by line
> - remove unnecessary comments
> 
> v1: https://lore.kernel.org/all/20250624130841.34693-1-abdelrahmanfekry375@gmail.com/
> 
> ---
>  drivers/staging/media/atomisp/pci/hmm/hmm.c | 91 ---------------------
>  1 file changed, 91 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> index 84102c3aaf97..f998b57f90c4 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> @@ -28,88 +28,6 @@ struct hmm_bo_device bo_device;
>  static ia_css_ptr dummy_ptr = mmgr_EXCEPTION;
>  static bool hmm_initialized;
> 
> -/*
> - * p: private
> - * v: vmalloc
> - */
> -static const char hmm_bo_type_string[] = "pv";
> -
> -static ssize_t bo_show(struct device *dev, struct device_attribute *attr,
> -		       char *buf, struct list_head *bo_list, bool active)
> -{
> -	ssize_t ret = 0;
> -	struct hmm_buffer_object *bo;
> -	unsigned long flags;
> -	int i;
> -	long total[HMM_BO_LAST] = { 0 };
> -	long count[HMM_BO_LAST] = { 0 };
> -	int index1 = 0;
> -	int index2 = 0;
> -
> -	ret = scnprintf(buf, PAGE_SIZE, "type pgnr\n");
> -	if (ret <= 0)
> -		return 0;
> -
> -	index1 += ret;
> -
> -	spin_lock_irqsave(&bo_device.list_lock, flags);
> -	list_for_each_entry(bo, bo_list, list) {
> -		if ((active && (bo->status & HMM_BO_ALLOCED)) ||
> -		    (!active && !(bo->status & HMM_BO_ALLOCED))) {
> -			ret = scnprintf(buf + index1, PAGE_SIZE - index1,
> -					"%c %d\n",
> -					hmm_bo_type_string[bo->type], bo->pgnr);
> -
> -			total[bo->type] += bo->pgnr;
> -			count[bo->type]++;
> -			if (ret > 0)
> -				index1 += ret;
> -		}
> -	}
> -	spin_unlock_irqrestore(&bo_device.list_lock, flags);
> -
> -	for (i = 0; i < HMM_BO_LAST; i++) {
> -		if (count[i]) {
> -			ret = scnprintf(buf + index1 + index2,
> -					PAGE_SIZE - index1 - index2,
> -					"%ld %c buffer objects: %ld KB\n",
> -					count[i], hmm_bo_type_string[i],
> -					total[i] * 4);
> -			if (ret > 0)
> -				index2 += ret;
> -		}
> -	}
> -
> -	/* Add trailing zero, not included by scnprintf */
> -	return index1 + index2 + 1;
> -}
> -
> -static ssize_t active_bo_show(struct device *dev, struct device_attribute *attr,
> -			      char *buf)
> -{
> -	return bo_show(dev, attr, buf, &bo_device.entire_bo_list, true);
> -}
> -
> -static ssize_t free_bo_show(struct device *dev, struct device_attribute *attr,
> -			    char *buf)
> -{
> -	return bo_show(dev, attr, buf, &bo_device.entire_bo_list, false);
> -}
> -
> -
> -static DEVICE_ATTR_RO(active_bo);
> -static DEVICE_ATTR_RO(free_bo);
> -
> -static struct attribute *sysfs_attrs_ctrl[] = {
> -	&dev_attr_active_bo.attr,
> -	&dev_attr_free_bo.attr,
> -	NULL
> -};
> -
> -static struct attribute_group atomisp_attribute_group[] = {
> -	{.attrs = sysfs_attrs_ctrl },
> -};
> -
>  int hmm_init(void)
>  {
>  	int ret;
> @@ -130,14 +48,6 @@ int hmm_init(void)
>  	 */
>  	dummy_ptr = hmm_alloc(1);
> 
> -	if (!ret) {
> -		ret = sysfs_create_group(&atomisp_dev->kobj,
> -					 atomisp_attribute_group);
> -		if (ret)
> -			dev_err(atomisp_dev,
> -				"%s Failed to create sysfs\n", __func__);
> -	}
> -
>  	return ret;
>  }
> 
> @@ -145,7 +55,6 @@ void hmm_cleanup(void)
>  {
>  	if (dummy_ptr == mmgr_EXCEPTION)
>  		return;
> -	sysfs_remove_group(&atomisp_dev->kobj, atomisp_attribute_group);
> 
>  	/* free dummy memory first */
>  	hmm_free(dummy_ptr);
> --
> 2.25.1
>