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

PATCH In review Anubhav Kokane 3 participants 6 messages Jan 12 – Jan 15

Subsystems (4)

Media (V4L/DVB) Staging Intel MID (Mobile Internet Device) platform Staging - atomisp driver

lore

Share & tools

lore

Anubhav Kokane

author

2026-01-12 19:01 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/staging/media/atomisp/pci/atomisp_ioctl.c +3 -3

index bb8b2f2213b0..60f705fac3c7 100644
@@ -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;

Signature · 1 line

2.43.0

Andy Shevchenko

2026-01-12 19:14 UTC #1 lore raw

Quoted 6 lines · 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)...)"

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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Anubhav Kokane

author

2026-01-13 10:37 UTC #2 lore raw

Quoted 16 lines · Andy Shevchenko

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

Andy Shevchenko

2026-01-13 10:46 UTC #3 lore raw

Quoted 17 lines · Anubhav Kokane

Quoted 15 lines · Andy Shevchenko

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

Quoted 1 line

Thank you for the feedback.

You're welcome!

Quoted 3 lines

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?

Quoted 3 lines

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).

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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().

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Anubhav Kokane

author

2026-01-13 16:09 UTC #4 lore raw

Quoted 29 lines · Andy Shevchenko

Quoted 3 lines
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?
Quoted 3 lines
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).
Quoted 3 lines
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.
Quoted 2 lines
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.

Quoted 5 lines

Quoted 2 lines
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

Anubhav Kokane

author

2026-01-15 11:21 UTC #5 lore raw

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

Quoted 52 lines · Anubhav Kokane

Quoted 29 lines · Andy Shevchenko
Quoted 3 lines
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?
Quoted 3 lines
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).
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 5 lines
Quoted 2 lines
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