| Message ID | 20241013084529.377488-3-umang.jain@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: vchiq_arm: Fix drv_mgmt leak | expand |
Hi Umang, Am 13.10.24 um 10:45 schrieb Umang Jain: > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> except of the missing commit message, this patch looks good to me. I understand the concerns about devm_kzalloc, but I think this doesn't apply in this case. Since this should be treated as RFC, is it already tested? Regards > --- > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > index e780ed714a14..334fb7037766 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > @@ -1345,7 +1345,7 @@ static int vchiq_probe(struct platform_device *pdev) > return -ENOENT; > } > > - mgmt = kzalloc(sizeof(*mgmt), GFP_KERNEL); > + mgmt = devm_kzalloc(&pdev->dev, sizeof(*mgmt), GFP_KERNEL); > if (!mgmt) > return -ENOMEM; > > @@ -1403,8 +1403,6 @@ static void vchiq_remove(struct platform_device *pdev) > > arm_state = vchiq_platform_get_arm_state(&mgmt->state); > kthread_stop(arm_state->ka_thread); > - > - kfree(mgmt); > } > > static struct platform_driver vchiq_driver = {
On 13/10/24 2:43 pm, Stefan Wahren wrote: > Hi Umang, > > Am 13.10.24 um 10:45 schrieb Umang Jain: >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > except of the missing commit message, this patch looks good to me. I > understand the concerns about devm_kzalloc, but I think this doesn't > apply in this case. That's what I was wondering as well, since I tried module unloading and with the cdev also goes away? So shouldn't be conern, right ? > > Since this should be treated as RFC, is it already tested? yes, it was tested > > Regards >> --- >> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git >> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> index e780ed714a14..334fb7037766 100644 >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >> @@ -1345,7 +1345,7 @@ static int vchiq_probe(struct platform_device >> *pdev) >> return -ENOENT; >> } >> >> - mgmt = kzalloc(sizeof(*mgmt), GFP_KERNEL); >> + mgmt = devm_kzalloc(&pdev->dev, sizeof(*mgmt), GFP_KERNEL); >> if (!mgmt) >> return -ENOMEM; >> >> @@ -1403,8 +1403,6 @@ static void vchiq_remove(struct platform_device >> *pdev) >> >> arm_state = vchiq_platform_get_arm_state(&mgmt->state); >> kthread_stop(arm_state->ka_thread); >> - >> - kfree(mgmt); >> } >> >> static struct platform_driver vchiq_driver = { >
Am 13.10.24 um 12:36 schrieb Umang Jain: > > > On 13/10/24 2:43 pm, Stefan Wahren wrote: >> Hi Umang, >> >> Am 13.10.24 um 10:45 schrieb Umang Jain: >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> except of the missing commit message, this patch looks good to me. I >> understand the concerns about devm_kzalloc, but I think this doesn't >> apply in this case. > > That's what I was wondering as well, since I tried module unloading > and with the cdev also goes away? So shouldn't be conern, right ? AFAIU the problem would be if you bind the resources to the cdev, but this isn't the case here. Btw I missed to mention that this is considered as a fix and deserves a Fixes tag. > >> >> Since this should be treated as RFC, is it already tested? > > yes, it was tested >> >> Regards >>> --- >>> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git >>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >>> index e780ed714a14..334fb7037766 100644 >>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >>> @@ -1345,7 +1345,7 @@ static int vchiq_probe(struct platform_device >>> *pdev) >>> return -ENOENT; >>> } >>> >>> - mgmt = kzalloc(sizeof(*mgmt), GFP_KERNEL); >>> + mgmt = devm_kzalloc(&pdev->dev, sizeof(*mgmt), GFP_KERNEL); >>> if (!mgmt) >>> return -ENOMEM; >>> >>> @@ -1403,8 +1403,6 @@ static void vchiq_remove(struct >>> platform_device *pdev) >>> >>> arm_state = vchiq_platform_get_arm_state(&mgmt->state); >>> kthread_stop(arm_state->ka_thread); >>> - >>> - kfree(mgmt); >>> } >>> >>> static struct platform_driver vchiq_driver = { >> >
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c index e780ed714a14..334fb7037766 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c @@ -1345,7 +1345,7 @@ static int vchiq_probe(struct platform_device *pdev) return -ENOENT; } - mgmt = kzalloc(sizeof(*mgmt), GFP_KERNEL); + mgmt = devm_kzalloc(&pdev->dev, sizeof(*mgmt), GFP_KERNEL); if (!mgmt) return -ENOMEM; @@ -1403,8 +1403,6 @@ static void vchiq_remove(struct platform_device *pdev) arm_state = vchiq_platform_get_arm_state(&mgmt->state); kthread_stop(arm_state->ka_thread); - - kfree(mgmt); } static struct platform_driver vchiq_driver = {
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)