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

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

[Anubhav Kokane]

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)

[Andy Shevchenko]

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)

[Anubhav Kokane]

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)

[Andy Shevchenko]

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)

[Anubhav Kokane]

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)

[Anubhav Kokane]

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