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 needs to be removed before this
driver can 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>
---
v2:
- 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 | 92 +--------------------
 1 file changed, 1 insertion(+), 91 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
index 84102c3aaf97..0a4d7dd5e6c4 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,7 @@ 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

On Tue, Jun 24, 2025 at 05:49:43PM +0300, Abdelrahman Fekry wrote:
>  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;


It's really unclear how this "return ret;" is supposed to work.  Was
that part of the sysfs_create_group()?

>  }
>  
> @@ -145,7 +55,7 @@ void hmm_cleanup(void)
>  {
>  	if (dummy_ptr == mmgr_EXCEPTION)
>  		return;
> -	sysfs_remove_group(&atomisp_dev->kobj, atomisp_attribute_group);
> +
>  

You've introduced two blank lines in a row here.  (checkpatch.pl -f
will complain).

regards,
dan carpenter

>  	/* free dummy memory first */
>  	hmm_free(dummy_ptr);
> -- 
> 2.25.1
>

On Tue, Jun 24, 2025 at 7:31 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Tue, Jun 24, 2025 at 05:49:43PM +0300, Abdelrahman Fekry wrote:
> >  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;
>
>
> It's really unclear how this "return ret;" is supposed to work.  Was
> that part of the sysfs_create_group()?
>
yes , but still it can be set by hmm_bo_device_init so even after removing
sysfs_create_group , ret value depends on another function.


> >  }
> >
> > @@ -145,7 +55,7 @@ void hmm_cleanup(void)
> >  {
> >       if (dummy_ptr == mmgr_EXCEPTION)
> >               return;
> > -     sysfs_remove_group(&atomisp_dev->kobj, atomisp_attribute_group);
> > +
> >
>
> You've introduced two blank lines in a row here.  (checkpatch.pl -f
> will complain).
>
noted , will fix it , and send v3
> regards,
> dan carpenter
>
> >       /* free dummy memory first */
> >       hmm_free(dummy_ptr);
> > --
> > 2.25.1
> >

On Tue, Jun 24, 2025 at 07:51:10PM +0300, Abdelrahman Fekry wrote:
> On Tue, Jun 24, 2025 at 7:31 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Tue, Jun 24, 2025 at 05:49:43PM +0300, Abdelrahman Fekry wrote:
> > >  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;
> >
> >
> > It's really unclear how this "return ret;" is supposed to work.  Was
> > that part of the sysfs_create_group()?
> >
> yes , but still it can be set by hmm_bo_device_init so even after removing
> sysfs_create_group , ret value depends on another function.
> 

You're in too big of a hurry.  Wait for a day between resending patches.
I have looked at this some more and it turns out that nothing checks the
error code so the "return ret;" doesn't work.  What do you think we
should do?

regards,
dan carpenter

On Tue, Jun 24, 2025 at 8:32 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Tue, Jun 24, 2025 at 07:51:10PM +0300, Abdelrahman Fekry wrote:
> > On Tue, Jun 24, 2025 at 7:31 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > >
> > > On Tue, Jun 24, 2025 at 05:49:43PM +0300, Abdelrahman Fekry wrote:
> > > >  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;
> > >
> > >
> > > It's really unclear how this "return ret;" is supposed to work.  Was
> > > that part of the sysfs_create_group()?
> > >
> > yes , but still it can be set by hmm_bo_device_init so even after removing
> > sysfs_create_group , ret value depends on another function.
> >
>
> You're in too big of a hurry.  Wait for a day between resending patches.
> I have looked at this some more and it turns out that nothing checks the
> error code so the "return ret;" doesn't work.  What do you think we
> should do?
>
sorry , will keep the time issue in mind.
regarding the "return ret", its now basically returning the error code of
hmm_bo_device_init () inside the function , but outside the function
scope like you mentioned, no function call to the hmm_init() checks
the error code. Thats what you mean right ?

> regards,
> dan carpenter
>

On Tue, Jun 24, 2025 at 08:49:36PM +0300, Abdelrahman Fekry wrote:
> On Tue, Jun 24, 2025 at 8:32 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Tue, Jun 24, 2025 at 07:51:10PM +0300, Abdelrahman Fekry wrote:
> > > On Tue, Jun 24, 2025 at 7:31 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > > >
> > > > On Tue, Jun 24, 2025 at 05:49:43PM +0300, Abdelrahman Fekry wrote:
> > > > >  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;
> > > >
> > > >
> > > > It's really unclear how this "return ret;" is supposed to work.  Was
> > > > that part of the sysfs_create_group()?
> > > >
> > > yes , but still it can be set by hmm_bo_device_init so even after removing
> > > sysfs_create_group , ret value depends on another function.
> > >
> >
> > You're in too big of a hurry.  Wait for a day between resending patches.
> > I have looked at this some more and it turns out that nothing checks the
> > error code so the "return ret;" doesn't work.  What do you think we
> > should do?
> >
> sorry , will keep the time issue in mind.
> regarding the "return ret", its now basically returning the error code of
> hmm_bo_device_init () inside the function , but outside the function
> scope like you mentioned, no function call to the hmm_init() checks
> the error code. Thats what you mean right ?

Yes.  Nothing is checking for if hmm_init() fails.  Step through the
code and verify that nothing crashes or bad happens as a result.

For example, I think hmm_bo_alloc() will print "hmm_bo_device not inited
yet." and return.  So that's kind of annoying but it's not a crash.
Search through the rest of the driver and verify how it will behave.

regards,
dan carpenter

Hello Dan , Thanks for your review

On Tue, Jun 24, 2025 at 9:31 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> Yes.  Nothing is checking for if hmm_init() fails.  Step through the
> code and verify that nothing crashes or bad happens as a result.
>
> For example, I think hmm_bo_alloc() will print "hmm_bo_device not inited
> yet." and return.  So that's kind of annoying but it's not a crash.
> Search through the rest of the driver and verify how it will behave.
>

So, I have been searching through the code as you suggested,
and found a couple of interesting things to look at.

Firstly, no function that calls hmm_init() checks its return code,
and it doesn’t crash anywhere because of this, so it's not a problem.
But the thing is, the hmm_initialized flag inside the hmm_init() function
is set even if hmm_bo_device_init() fails, and this can be misleading for
functions like __hm_alloc() that check this flag later.

Secondly, the function hmm_bo_alloc() and others don’t check
the return code of hmm_init(). Instead, they check the flag
HMM_BO_DEVICE_INITED inside bdev, which is set by the function
hmm_bo_device_init(). The problem is, if we inspect hmm_bo_device_init(),
we find that the HMM_BO_DEVICE_INITED flag is set before the calls to
kmem_cache_create(), kmem_cache_alloc(), and __bo_init().
This means that if any of these functions fail, the flag will still be set,
which can lead to misbehavior in functions that rely on it, like hmm_bo_alloc().

Should I tackle these problems after submitting the original patch of
removing the debug sysfs attr. ?

Best Regards,
Abdelrahman Fekry

Thank you.  This is good analysis.

On Thu, Jun 26, 2025 at 06:19:40PM +0300, Abdelrahman Fekry wrote:
> Hello Dan , Thanks for your review
> 
> On Tue, Jun 24, 2025 at 9:31 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > Yes.  Nothing is checking for if hmm_init() fails.  Step through the
> > code and verify that nothing crashes or bad happens as a result.
> >
> > For example, I think hmm_bo_alloc() will print "hmm_bo_device not inited
> > yet." and return.  So that's kind of annoying but it's not a crash.
> > Search through the rest of the driver and verify how it will behave.
> >
> 
> So, I have been searching through the code as you suggested,
> and found a couple of interesting things to look at.
> 
> Firstly, no function that calls hmm_init() checks its return code,
> and it doesn’t crash anywhere because of this, so it's not a problem.
> But the thing is, the hmm_initialized flag inside the hmm_init() function
> is set even if hmm_bo_device_init() fails, and this can be misleading for
> functions like __hm_alloc() that check this flag later.

Yeah.

> 
> Secondly, the function hmm_bo_alloc() and others don’t check
> the return code of hmm_init().

hmm_init() is called from two places, atomisp_pci_probe() and
__hmm_alloc().  Is it really possible to call __hmm_alloc() before
atomisp_pci_probe() has succeeded?  That's an important question.
Please try to read the code and find the answer.

It would be better to only call hmm_init() from the probe function
and delete the call in __hmm_alloc() but I don't know if that's
possible.

> Instead, they check the flag
> HMM_BO_DEVICE_INITED inside bdev, which is set by the function
> hmm_bo_device_init(). The problem is, if we inspect hmm_bo_device_init(),
> we find that the HMM_BO_DEVICE_INITED flag is set before the calls to
> kmem_cache_create(), kmem_cache_alloc(), and __bo_init().
> This means that if any of these functions fail, the flag will still be set,
> which can lead to misbehavior in functions that rely on it, like hmm_bo_alloc().
> 

Yeah.  That seems correct to me too.

> Should I tackle these problems after submitting the original patch of
> removing the debug sysfs attr. ?

Perfect.  Sounds good.

regards,
dan carpenter