staging: vt6656: replace kzalloc with kmalloc
PATCH 1/1 staging: vt6656: replace kzalloc with kmalloc
All fields of allocated structure are filled in directly following the allocate, so no need to initialize it to zero.
Use preferred kmalloc() sizeof style to silence:
CHECK: Prefer kmalloc(sizeof(*priv->rcb[ii])...)
--- drivers/staging/vt6656/main_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vt6656/main_usb.c +1 -1
@@ -449,7 +449,7 @@ static bool vnt_alloc_bufs(struct vnt_private *priv) }for (ii =0 ; ii < priv->num_rcb; ii++) { - priv->rcb[ii] =kzalloc (sizeof (struct vnt_rcb ),GFP_KERNEL ); + priv->rcb[ii] =kmalloc (sizeof (*priv->rcb[ii]),GFP_KERNEL );if (!priv->rcb[ii]) {dev_err (&priv->usb->dev,"failed to allocate rcb no %d\n" , ii);
Signature · 1 line
2.1.4
Hello,
Quoted 16 lines · Alison Schofield
All fields of allocated structure are filled in directly following the allocate, so no need to initialize it to zero.
Use preferred
kmalloc()sizeof style to silence:
CHECK: Prefer kmalloc(sizeof(*priv->rcb[ii])...)Signed-off-by Alison Schofield <[email protected]>--- drivers/staging/vt6656/main_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/vt6656/main_usb.c +0
-0
drivers/staging/vt6656/main_usb.c static bool vnt_alloc_bufs(struct vnt_private *priv) · line 452
Quoted 5 lines
drivers/staging/vt6656/main_usb.c +1
-1@@ -449,7 +449,7 @@ static bool vnt_alloc_bufs(struct vnt_private *priv) }for (ii =0 ; ii < priv->num_rcb; ii++) { - priv->rcb[ii] =kzalloc (sizeof (struct vnt_rcb ),GFP_KERNEL ); + priv->rcb[ii] =kmalloc (sizeof (*priv->rcb[ii]),GFP_KERNEL );
I don't know. It's on the init path which is extremely cold and zeroing followed by inits are extremely cheap as the inits end up hitting the same cachelines (sometimes even cheaper than not doing it as zeroing often doesn't even need to read in the cachelines, so it just becomes cacheline allocation). It's nice to zero this type of structures as later field additions or code modifications may lead to surprises.
This doesn't mean kzalloc is always better than kmalloc or vice-versa and what to use when can be a surprisingly tricky question. I'd suggest staying away from mechanical conversions between kmalloc and kzalloc.
Thanks.
Signature · 1 line
tejun
Quoted 33 lines · Tejun Heo
Hello,
Quoted 16 lines · Alison Schofield
All fields of allocated structure are filled in directly following the allocate, so no need to initialize it to zero.
Use preferred
kmalloc()sizeof style to silence:
CHECK: Prefer kmalloc(sizeof(*priv->rcb[ii])...)Signed-off-by Alison Schofield <[email protected]>--- drivers/staging/vt6656/main_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/vt6656/main_usb.c +0
-0drivers/staging/vt6656/main_usb.c static bool vnt_alloc_bufs(struct vnt_private *priv) · line 452
Quoted 5 lines
drivers/staging/vt6656/main_usb.c +1
-1@@ -449,7 +449,7 @@ static bool vnt_alloc_bufs(struct vnt_private *priv) }for (ii =0 ; ii < priv->num_rcb; ii++) { - priv->rcb[ii] =kzalloc (sizeof (struct vnt_rcb ),GFP_KERNEL ); + priv->rcb[ii] =kmalloc (sizeof (*priv->rcb[ii]),GFP_KERNEL );I don't know. It's on the init path which is extremely cold and zeroing followed by inits are extremely cheap as the inits end up hitting the same cachelines (sometimes even cheaper than not doing it as zeroing often doesn't even need to read in the cachelines, so it just becomes cacheline allocation). It's nice to zero this type of structures as later field additions or code modifications may lead to surprises.
Quoted 9 lines
This doesn't mean kzalloc is always better than kmalloc or vice-versa and what to use when can be a surprisingly tricky question. I'd suggest staying away from mechanical conversions between kmalloc and kzalloc.
Thanks.
Signature · 1 line
tejun
Thanks for the review and detailed comments. This caught my eye because it differed from the other kmallocs in same function. I found that it had been changed to kZalloc a few years ago via a patch that replaced kmalloc+memset with kzalloc. Unfortunately there was no mention of why the zeroing was needed in the first place. I saw that to be a "mechanial conversion" and so pursued further.
Each piece of this struct is initialized immediately after the alloc, so usage before initialization is not the issue. As far as being on a cold path...that's where I lost the trail. It was not clear to me that the ".start" op is only done at probe. I guess I will drop it based on that fact.
(cachelines and protecting future changes were not on my radar)
Thanks,
alison