[v1] staging: media: atomisp: Replace scnprintf with sysfs_emit in bo_show

[PATCH] staging: media: atomisp: Replace scnprintf with sysfs_emit in bo_show

Posted by Abdelrahman Fekry 3 months, 2 weeks ago

Convert buffer output to use sysfs_emit/sysfs_emit_at API for safer
PAGE_SIZE handling and standardized sysfs output.

Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
---
 drivers/staging/media/atomisp/pci/hmm/hmm.c | 24 ++++++---------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
index 84102c3aaf97..cae1fccd06af 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
@@ -37,51 +37,41 @@ 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;
+	ssize_t offset = 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;
+	offset += sysfs_emit(buf, "type pgnr\n");
 
 	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,
+			offset += sysfs_emit_at(buf, offset,
 					"%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,
+			offset += sysfs_emit_at(buf,
+					offset,
 					"%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;
+	/* Direct return of accumlated length */
+	return offset;
 }
 
 static ssize_t active_bo_show(struct device *dev, struct device_attribute *attr,
-- 
2.25.1

Re: [PATCH] staging: media: atomisp: Replace scnprintf with sysfs_emit in bo_show

Posted by Hans de Goede 3 months, 2 weeks ago

Hi,

On 21-Jun-25 8:29 AM, Abdelrahman Fekry wrote:
> Convert buffer output to use sysfs_emit/sysfs_emit_at API for safer
> PAGE_SIZE handling and standardized sysfs output.
> 
> Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
> ---
>  drivers/staging/media/atomisp/pci/hmm/hmm.c | 24 ++++++---------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> index 84102c3aaf97..cae1fccd06af 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> @@ -37,51 +37,41 @@ 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)
>  {

<snip>

As Andy already mentioned you really should try to first better understand
the code before changing it.

In this case this code is used for 2 custom sysfs attributes called
"active_bo" and "free_bo".

sysfs attributes are custom userspace API and we really want to remove
all custom userspace APIs from this driver before moving it out of
drivers/staging

Instead everything should be done through existing standard kernels
API, mostly the standard v4l2 API.

Note this is already mentioned in drivers/staging/media/atomisp/TODO
although these specific sysfs attributes are not named:

"""
TODO
====

1. Items which MUST be fixed before the driver can be moved out of staging:

* Remove/disable private IOCTLs

* Remove/disable custom v4l2-ctrls

* Remove custom sysfs files created by atomisp_drvfs.c
"""

In this case the "active_bo" and "free_bo" sysfs attributes seem
to be there for debugging purposes only and they can simply be removed.

So instead of replacing scnprintf you should write a new patch
removing everything starting at:

<--- start removing code here --->
/*
 * p: private
 * v: vmalloc

...

static struct attribute_group atomisp_attribute_group[] = {
        {.attrs = sysfs_attrs_ctrl },
};
<--- end removing code here --->

and then also remove the sysfs_create_group() and
sysfs_remove_group() calls.

After writing that patch maybe you can also take a look at tackling
the "Remove custom sysfs files created by atomisp_drvfs.c" TODO
list item?

Regards,

Hans

Re: [PATCH] staging: media: atomisp: Replace scnprintf with sysfs_emit in bo_show

Posted by Abdelrahman Fekry 3 months, 2 weeks ago

On Mon, Jun 23, 2025 at 9:31 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Hi,
> As Andy already mentioned you really should try to first better understand
> the code before changing it.
>
> In this case this code is used for 2 custom sysfs attributes called
> "active_bo" and "free_bo".
>
> sysfs attributes are custom userspace API and we really want to remove
> all custom userspace APIs from this driver before moving it out of
> drivers/staging
>
> Instead everything should be done through existing standard kernels
> API, mostly the standard v4l2 API.
>
> Note this is already mentioned in drivers/staging/media/atomisp/TODO
> although these specific sysfs attributes are not named:
>
> """
> TODO
> ====
>
> 1. Items which MUST be fixed before the driver can be moved out of staging:
>
> * Remove/disable private IOCTLs
>
> * Remove/disable custom v4l2-ctrls
>
> * Remove custom sysfs files created by atomisp_drvfs.c
> """
>
> In this case the "active_bo" and "free_bo" sysfs attributes seem
> to be there for debugging purposes only and they can simply be removed.
>
> So instead of replacing scnprintf you should write a new patch
> removing everything starting at:
>
> <--- start removing code here --->
> /*
>  * p: private
>  * v: vmalloc
>
> ...
>
> static struct attribute_group atomisp_attribute_group[] = {
>         {.attrs = sysfs_attrs_ctrl },
> };
> <--- end removing code here --->
>
> and then also remove the sysfs_create_group() and
> sysfs_remove_group() calls.
>
> After writing that patch maybe you can also take a look at tackling
> the "Remove custom sysfs files created by atomisp_drvfs.c" TODO
> list item?
>
> Regards,
>
> Hans
>

Thank you so much Hans for this informative feedback , i now see the
whole picture ,
i will submit a new patch that removes the two custom attributes
active_bo and free_bo
and then will proceed with the TODO list item " Remove custom sysfs
files created by atomisp_drvfs.c"

Re: [PATCH] staging: media: atomisp: Replace scnprintf with sysfs_emit in bo_show

Posted by Andy Shevchenko 3 months, 2 weeks ago

On Sat, Jun 21, 2025 at 9:30 AM Abdelrahman Fekry
<abdelrahmanfekry375@gmail.com> wrote:
>
> Convert buffer output to use sysfs_emit/sysfs_emit_at API for safer
> PAGE_SIZE handling and standardized sysfs output.

...

> -       ssize_t ret = 0;
> +       ssize_t offset = 0;

It would be good to move this...

>         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;

...to be here.

>
> -       ret = scnprintf(buf, PAGE_SIZE, "type pgnr\n");
> -       if (ret <= 0)
> -               return 0;
> -
> -       index1 += ret;
> +       offset += sysfs_emit(buf, "type pgnr\n");

This changes the behaviour in case the sysfs_emit() fails. Not that
this is a big issue, but it should be pointed out somewhere.

...

> +       /* Direct return of accumlated length */

accumulated

Don't forget to run a spell-checker.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: media: atomisp: Replace scnprintf with sysfs_emit in bo_show

Posted by Dan Carpenter 3 months, 2 weeks ago

On Sat, Jun 21, 2025 at 09:24:40PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 21, 2025 at 9:30 AM Abdelrahman Fekry
> >
> > -       ret = scnprintf(buf, PAGE_SIZE, "type pgnr\n");
> > -       if (ret <= 0)
> > -               return 0;
> > -
> > -       index1 += ret;
> > +       offset += sysfs_emit(buf, "type pgnr\n");
> 
> This changes the behaviour in case the sysfs_emit() fails. Not that
> this is a big issue, but it should be pointed out somewhere.
> 
> ...

Neither scnprintf() nor sysfs_emit() can return negatives.

regards,
dan carpenter

Re: [PATCH] staging: media: atomisp: Replace scnprintf with sysfs_emit in bo_show

Posted by Andy Shevchenko 3 months, 2 weeks ago

On Mon, Jun 23, 2025 at 05:16:57PM +0300, Dan Carpenter wrote:
> On Sat, Jun 21, 2025 at 09:24:40PM +0300, Andy Shevchenko wrote:
> > On Sat, Jun 21, 2025 at 9:30 AM Abdelrahman Fekry

...

> > > -       ret = scnprintf(buf, PAGE_SIZE, "type pgnr\n");
> > > -       if (ret <= 0)
> > > -               return 0;
> > > -
> > > -       index1 += ret;
> > > +       offset += sysfs_emit(buf, "type pgnr\n");
> > 
> > This changes the behaviour in case the sysfs_emit() fails. Not that
> > this is a big issue, but it should be pointed out somewhere.
> 
> Neither scnprintf() nor sysfs_emit() can return negatives.

Good, that's what I asked the author to investigate and add the respective
comment / update commit message accordingly.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: media: atomisp: Replace scnprintf with sysfs_emit in bo_show

Posted by Abdelrahman Fekry 3 months, 2 weeks ago

Thanks for the review
On Sat, Jun 21, 2025 at 9:25 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Jun 21, 2025 at 9:30 AM Abdelrahman Fekry
> <abdelrahmanfekry375@gmail.com> wrote:
> >
> > Convert buffer output to use sysfs_emit/sysfs_emit_at API for safer
> > PAGE_SIZE handling and standardized sysfs output.
>
> ...
>
> > -       ssize_t ret = 0;
> > +       ssize_t offset = 0;
>
> It would be good to move this...
>
> >         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;
>
> ...to be here.

noted , thanks.

> >
> > -       ret = scnprintf(buf, PAGE_SIZE, "type pgnr\n");
> > -       if (ret <= 0)
> > -               return 0;
> > -
> > -       index1 += ret;
> > +       offset += sysfs_emit(buf, "type pgnr\n");
>
> This changes the behaviour in case the sysfs_emit() fails. Not that
> this is a big issue, but it should be pointed out somewhere.

noted , will make a comment about it.
> ...
>
> > +       /* Direct return of accumlated length */
>
> accumulated
>
> Don't forget to run a spell-checker.
>
noted .
> --
> With Best Regards,
> Andy Shevchenko