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

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

[Abdelrahman Fekry]

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>
---
v2:
- Change the place of ssize_t offset.
- Addressing a possible change in behaviour if sysfs_emit fails.
v1: https://lore.kernel.org/all/20250621062944.168386-1-abdelrahmanfekry375@gmail.com/
- Convert scnprintf with sysfs_emit/sysfs_emit_at in bo_show func.
 drivers/staging/media/atomisp/pci/hmm/hmm.c | 25 +++++++--------------
 1 file changed, 8 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..b5d0516e36dc 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
@@ -37,51 +37,42 @@ 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;
+	ssize_t offset = 0;
 
-	ret = scnprintf(buf, PAGE_SIZE, "type pgnr\n");
-	if (ret <= 0)
-		return 0;
-
-	index1 += ret;
+	/* Changing to sysfs_emit changes the behaviour if failed*/
+	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 accumulated length */
+	return offset;
 }
 
 static ssize_t active_bo_show(struct device *dev, struct device_attribute *attr,
-- 
2.25.1

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

[Andy Shevchenko]

On Sun, Jun 22, 2025 at 9:54 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.

...

> v2:

> - Addressing a possible change in behaviour if sysfs_emit fails.

I don't see it being done. Can you elaborate, please?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Jun 22, 2025 at 9:54 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.

...

> +       /* Changing to sysfs_emit changes the behaviour if failed*/

This comment, besides missing the space, is useless here. You need to
use the comment block for the proposed change.

> +       offset += sysfs_emit(buf, "type pgnr\n");


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Jun 22, 2025 at 11:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Jun 22, 2025 at 9:54 AM Abdelrahman Fekry
> <abdelrahmanfekry375@gmail.com> wrote:

...

> > +       /* Changing to sysfs_emit changes the behaviour if failed*/
>
> This comment, besides missing the space, is useless here. You need to
> use the comment block for the proposed change.

Also the text of the comment is a noise. You need to explain better
what's going on here.

-- 
With Best Regards,
Andy Shevchenko

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

[Abdelrahman Fekry]

On Sun, Jun 22, 2025 at 11:39 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 22, 2025 at 11:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Jun 22, 2025 at 9:54 AM Abdelrahman Fekry
> > <abdelrahmanfekry375@gmail.com> wrote:
>
> ...
>
> > > +       /* Changing to sysfs_emit changes the behaviour if failed*/
> >
> > This comment, besides missing the space, is useless here. You need to
> > use the comment block for the proposed change.
>
> Also the text of the comment is a noise. You need to explain better
> what's going on here.
>
Sorry for the mess, so the comment is enough if well written or i
should do something else?
Thanks!
> --
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Jun 22, 2025 at 11:42 PM Abdelrahman Fekry
<abdelrahmanfekry375@gmail.com> wrote:
> On Sun, Jun 22, 2025 at 23:39 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Sun, Jun 22, 2025 at 11:37 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>> > On Sun, Jun 22, 2025 at 9:54 AM Abdelrahman Fekry
>> > <abdelrahmanfekry375@gmail.com> wrote:

...

>> > > +       /* Changing to sysfs_emit changes the behaviour if failed*/
>> >
>> > This comment, besides missing the space, is useless here. You need to
>> > use the comment block for the proposed change.
>>
>> Also the text of the comment is a noise. You need to explain better
>> what's going on here.
>
> Noted , so the comment is enough if well written or i should do something else?

First of all, it's a bare minimum, which means that you should go
deeper into the code to understand the issue to begin with. Second,
the comment should be put in the proper place. In the code it's
useless as it describes something that is absent in the code for odd
reasons. Talk to your mentors and ask them for help because explaining
more is basically doing your job for you. And IIUC the purpose of
mentoring is to make sure you learn something and have acknowledged
this in practice.

-- 
With Best Regards,
Andy Shevchenko

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

[Abdelrahman Fekry]

On Sun, Jun 22, 2025 at 11:52 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 22, 2025 at 11:42 PM Abdelrahman Fekry
> <abdelrahmanfekry375@gmail.com> wrote:
> > On Sun, Jun 22, 2025 at 23:39 Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >> On Sun, Jun 22, 2025 at 11:37 PM Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:
> >> > On Sun, Jun 22, 2025 at 9:54 AM Abdelrahman Fekry
> >> > <abdelrahmanfekry375@gmail.com> wrote:


> First of all, it's a bare minimum, which means that you should go
> deeper into the code to understand the issue to begin with. Second,
> the comment should be put in the proper place. In the code it's
> useless as it describes something that is absent in the code for odd
> reasons. Talk to your mentors and ask them for help because explaining
> more is basically doing your job for you. And IIUC the purpose of
> mentoring is to make sure you learn something and have acknowledged
> this in practice.
>
Thanks , i will come back with v3 with more explanation and details.
> --
> With Best Regards,
> Andy Shevchenko