staging: media: atomisp: refactor sizeof(struct type) to sizeof(*ptr)

[PATCH] staging: media: atomisp: refactor sizeof(struct type) to sizeof(*ptr)

Posted by Anubhav Kokane 3 weeks, 4 days ago

Use the variable name in sizeof() operator instead of the struct type.
This prevents errors if the type of the variable is changed in future.

This fixes checkpatch checks:
"CHECK: Prefer kzalloc(sizeof(*ptr)...) over
kzalloc(sizeof(struct type)...)"

Signed-off-by: Anubhav Kokane <dev.anubhavk@gmail.com>
---
 drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index bb8b2f2213b0..60f705fac3c7 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -696,7 +696,7 @@ int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
 			ATOMISP_S3A_BUF_QUEUE_DEPTH_FOR_HAL;
 		dev_dbg(isp->dev, "allocating %d 3a buffers\n", count);
 		while (count--) {
-			s3a_buf = kzalloc(sizeof(struct atomisp_s3a_buf), GFP_KERNEL);
+			s3a_buf = kzalloc(sizeof(*s3a_buf), GFP_KERNEL);
 			if (!s3a_buf)
 				goto error;
 
@@ -715,7 +715,7 @@ int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
 		count = ATOMISP_CSS_Q_DEPTH + 1;
 		dev_dbg(isp->dev, "allocating %d dis buffers\n", count);
 		while (count--) {
-			dis_buf = kzalloc(sizeof(struct atomisp_dis_buf), GFP_KERNEL);
+			dis_buf = kzalloc(sizeof(*dis_buf), GFP_KERNEL);
 			if (!dis_buf)
 				goto error;
 			if (atomisp_css_allocate_stat_buffers(
@@ -737,7 +737,7 @@ int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
 			dev_dbg(isp->dev, "allocating %d metadata buffers for type %d\n",
 				count, i);
 			while (count--) {
-				md_buf = kzalloc(sizeof(struct atomisp_metadata_buf),
+				md_buf = kzalloc(sizeof(*md_buf),
 						 GFP_KERNEL);
 				if (!md_buf)
 					goto error;
-- 
2.43.0

Re: [PATCH] staging: media: atomisp: refactor sizeof(struct type) to sizeof(*ptr)

Posted by Andy Shevchenko 3 weeks, 4 days ago

On Mon, Jan 12, 2026 at 07:00:53PM +0000, Anubhav Kokane wrote:
> Use the variable name in sizeof() operator instead of the struct type.
> This prevents errors if the type of the variable is changed in future.
> 
> This fixes checkpatch checks:
> "CHECK: Prefer kzalloc(sizeof(*ptr)...) over
> kzalloc(sizeof(struct type)...)"

Yeah, but in all cases the whole buffers are less than a page and hence instead
of doing that the preferred way is to switch to use kcalloc() in all three
places.

So you should have something like

		x = kcalloc(count, sizeof(*...), ...);
		if (!x)
			...handle error...

		while (count--) {
			...
		}
		...
err:
		list_for_each... {
			...
		}
		kfree(x);


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: media: atomisp: refactor sizeof(struct type) to sizeof(*ptr)

Posted by Anubhav Kokane 3 weeks, 4 days ago

On Tue, Jan 13, 2026 at 12:44 AM Andy Shevchenko  wrote:
> the preferred way is to switch to use kcalloc() in all three places.
>
>                 x = kcalloc(count, sizeof(*...), ...);
>                 if (!x)
>                         ...handle error...
>
>                 while (count--) {
>                         ...
>                 }
>                 ...
> err:
>                 list_for_each... {
>                         ...
>                 }
>                 kfree(x);
>
Hi Andy,
Thank you for the feedback.
I looked into implementing kcalloc() as suggested. But the issue is struct
atomisp_s3a_buf (and the other buffers) are defined as list nodes with
struct list_head list embedded in them. The driver relies on adding these
individually to asd->s3a_stats and freeing them individually using kfree()
in multiple cleanup paths (including error path here).

Switching to kcalloc() would mean the s3a_buf is no longer a standalone
object but a slice of an array. This would lead to invalid or double frees
if the existing code tries kfree() on this array element.

Addressing this requires a larger refactor of the buffer management logic
across the driver, would you prefer I stick to the sizeof(*ptr) hardening for
now to fix the checkpatch warning?

Regards,
Anubhav

Re: [PATCH] staging: media: atomisp: refactor sizeof(struct type) to sizeof(*ptr)

Posted by Andy Shevchenko 3 weeks, 4 days ago

On Tue, Jan 13, 2026 at 12:37 PM Anubhav Kokane <dev.anubhavk@gmail.com> wrote:
> On Tue, Jan 13, 2026 at 12:44 AM Andy Shevchenko  wrote:
> > the preferred way is to switch to use kcalloc() in all three places.
> >
> >                 x = kcalloc(count, sizeof(*...), ...);
> >                 if (!x)
> >                         ...handle error...
> >
> >                 while (count--) {
> >                         ...
> >                 }
> >                 ...
> > err:
> >                 list_for_each... {
> >                         ...
> >                 }
> >                 kfree(x);

> Thank you for the feedback.

You're welcome!

> I looked into implementing kcalloc() as suggested. But the issue is struct
> atomisp_s3a_buf (and the other buffers) are defined as list nodes with
> struct list_head list embedded in them.

Yes, and how does it affect the allocation?

> The driver relies on adding these
> individually to asd->s3a_stats and freeing them individually using kfree()
> in multiple cleanup paths (including error path here).

Is it the issue? Instead of incrementing by a pointer size, you will
increment an address by a structure size, this is how + operator works
in C from the beginning (or close enough to that time).

> Switching to kcalloc() would mean the s3a_buf is no longer a standalone
> object but a slice of an array. This would lead to invalid or double frees
> if the existing code tries kfree() on this array element.

How? As I showed above you need to carefully move and replace
individual handling by a common one. So, instead of allocation per
item it will be an allocation per bucket.

> Addressing this requires a larger refactor of the buffer management logic
> across the driver,

Exactly! And that's what I think is the best way moving forward. You
will kill two birds with one stone: fixing the issue at hand and
improving the memory allocations in the driver in this area a lot.

> would you prefer I stick to the sizeof(*ptr) hardening for
> now to fix the checkpatch warning?

See above. As now I think this is unneeded churn as the idea would
still be the same — moving towards kcalloc().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: media: atomisp: refactor sizeof(struct type) to sizeof(*ptr)

Posted by Anubhav Kokane 3 weeks, 4 days ago

On Tue, Jan 13, 2026 at 4:16 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> > I looked into implementing kcalloc() as suggested. But the issue is struct
> > atomisp_s3a_buf (and the other buffers) are defined as list nodes with
> > struct list_head list embedded in them.
>
> Yes, and how does it affect the allocation?
>
> > The driver relies on adding these
> > individually to asd->s3a_stats and freeing them individually using kfree()
> > in multiple cleanup paths (including error path here).
>
> Is it the issue? Instead of incrementing by a pointer size, you will
> increment an address by a structure size, this is how + operator works
> in C from the beginning (or close enough to that time).
>
> > Switching to kcalloc() would mean the s3a_buf is no longer a standalone
> > object but a slice of an array. This would lead to invalid or double frees
> > if the existing code tries kfree() on this array element.
>
> How? As I showed above you need to carefully move and replace
> individual handling by a common one. So, instead of allocation per
> item it will be an allocation per bucket.
>
> > Addressing this requires a larger refactor of the buffer management logic
> > across the driver,
>
> Exactly! And that's what I think is the best way moving forward. You
> will kill two birds with one stone: fixing the issue at hand and
> improving the memory allocations in the driver in this area a lot.
>
> > would you prefer I stick to the sizeof(*ptr) hardening for
> > now to fix the checkpatch warning?
>
> See above. As now I think this is unneeded churn as the idea would
> still be the same — moving towards kcalloc().

Hi Andy,

Thanks for the explanation regarding the pointer arithmetic and bucket
allocation.

I understand the approach now, will work on refactoring the allocation
to use kcalloc() and updating the cleanup paths to handle the array
correctly.

I'll send a v2 once I have verified the changes, though it might take me
a little time to ensure the cleanup logic is robust.

Regards,
Anubhav

Re: [PATCH] staging: media: atomisp: refactor sizeof(struct type) to sizeof(*ptr)

Posted by Anubhav Kokane 3 weeks, 2 days ago

Hello Andy,

I appreciate you taking time to explain to me the refactor using kcalloc.

After reviewing the requirements of the task, I realized that this is
beyond my current understanding of the kernel. As this is my first
contribution, I would prefer to build up more foundational knowledge
before tackling a refactor of this scope.

I am stepping back from this patch for now and working on simpler
contributions while I learn more about kernel development. If this issue
is still open after I have gained more experience, I would be interested
in revisiting it.

Thank you for your time and the learning opportunity.

Best regards,
Anubhav

On Tue, Jan 13, 2026 at 9:38 PM Anubhav Kokane <dev.anubhavk@gmail.com> wrote:
>
> On Tue, Jan 13, 2026 at 4:16 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > > I looked into implementing kcalloc() as suggested. But the issue is struct
> > > atomisp_s3a_buf (and the other buffers) are defined as list nodes with
> > > struct list_head list embedded in them.
> >
> > Yes, and how does it affect the allocation?
> >
> > > The driver relies on adding these
> > > individually to asd->s3a_stats and freeing them individually using kfree()
> > > in multiple cleanup paths (including error path here).
> >
> > Is it the issue? Instead of incrementing by a pointer size, you will
> > increment an address by a structure size, this is how + operator works
> > in C from the beginning (or close enough to that time).
> >
> > > Switching to kcalloc() would mean the s3a_buf is no longer a standalone
> > > object but a slice of an array. This would lead to invalid or double frees
> > > if the existing code tries kfree() on this array element.
> >
> > How? As I showed above you need to carefully move and replace
> > individual handling by a common one. So, instead of allocation per
> > item it will be an allocation per bucket.
> >
> > > Addressing this requires a larger refactor of the buffer management logic
> > > across the driver,
> >
> > Exactly! And that's what I think is the best way moving forward. You
> > will kill two birds with one stone: fixing the issue at hand and
> > improving the memory allocations in the driver in this area a lot.
> >
> > > would you prefer I stick to the sizeof(*ptr) hardening for
> > > now to fix the checkpatch warning?
> >
> > See above. As now I think this is unneeded churn as the idea would
> > still be the same — moving towards kcalloc().
>
> Hi Andy,
>
> Thanks for the explanation regarding the pointer arithmetic and bucket
> allocation.
>
> I understand the approach now, will work on refactoring the allocation
> to use kcalloc() and updating the cleanup paths to handle the array
> correctly.
>
> I'll send a v2 once I have verified the changes, though it might take me
> a little time to ensure the cleanup logic is robust.
>
> Regards,
> Anubhav