* Re: Re: [PATCH] staging: greybus: audio: Check null pointer
@ 2021-12-28 1:57 Jiasheng Jiang
0 siblings, 0 replies; 2+ messages in thread
From: Jiasheng Jiang @ 2021-12-28 1:57 UTC (permalink / raw)
To: vaibhav.sr, mgreer, johan, elder, gregkh
Cc: greybus-dev, linux-staging, linux-kernel, Jiasheng Jiang
On Mon, Dec 27, 2021 at 11:54:10PM +0800, Alex Elder wrote:
> I think this is a good change, but I would like you to improve
> the description, and fix some different bugs introduced by your
> change.
>
> What you are specifically doing is checking for a null return
> from devm_kcalloc() in gb_generate_enum_strings(), and are
> returning the NULL pointer if that occurs. That means you
> need to update all the callers of gb_generate_enum_strings()
> to also handle a possible null return value.
>
> The fix does a good thing, and your description is correct
> about what you are fixing. But it should supply more
> complete context for the change.
Thanks for your advice, I will correct my description in next version.
But I still have some question about the devm_kzalloc().
> You can't simply return here. If you look a bit above this,
> where the call to allocate a control structure is done, you
> see that a NULL return there jumps to the "error" label, so
> any already allocated and initialized control widgets get
> cleaned up before returning.
Actually, I have already thought of whether it needs to free after the devm_kzalloc().
As we can find in the gbaudio_tplg_create_widget(), the widget_kctls is allocated by devm_kzalloc(), but isn't released when gbaudio_tplg_create_wcontrol() fails and goto error.
And I check of the comment of the devm_kmalloc() in `drivers/base/devres.c`, because devm_kzalloc() returns devm_kmalloc().
And it says that "Memory allocated with this function is automatically freed on driver detach."
So there is no need to free the memory manually.
Is that right?
And I am sorry again because of the lack of the above explanation in my commit message.
I will also add to my new commit.
Thanks,
Jiang
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: Re: [PATCH] staging: greybus: audio: Check null pointer
@ 2021-12-28 2:02 Jiasheng Jiang
0 siblings, 0 replies; 2+ messages in thread
From: Jiasheng Jiang @ 2021-12-28 2:02 UTC (permalink / raw)
To: vaibhav.sr, mgreer, johan, elder, gregkh
Cc: greybus-dev, linux-staging, linux-kernel, Jiasheng Jiang
Sorry the previous email is forgetten to wrap line.
This email is corrected and the content is the same.
On Mon, Dec 27, 2021 at 11:54:10PM +0800, Alex Elder wrote:
> I think this is a good change, but I would like you to improve
> the description, and fix some different bugs introduced by your
> change.
>
> What you are specifically doing is checking for a null return
> from devm_kcalloc() in gb_generate_enum_strings(), and are
> returning the NULL pointer if that occurs. That means you
> need to update all the callers of gb_generate_enum_strings()
> to also handle a possible null return value.
>
> The fix does a good thing, and your description is correct
> about what you are fixing. But it should supply more
> complete context for the change.
Thanks for your advice, I will correct my description in next version.
But I still have some question about the devm_kzalloc().
> You can't simply return here. If you look a bit above this,
> where the call to allocate a control structure is done, you
> see that a NULL return there jumps to the "error" label, so
> any already allocated and initialized control widgets get
> cleaned up before returning.
Actually, I have already thought of whether it needs to free after the
devm_kzalloc().
As we can find in the gbaudio_tplg_create_widget(), the widget_kctls is
allocated by devm_kzalloc(), but isn't released when
gbaudio_tplg_create_wcontrol() fails and goto error.
And I check of the comment of the devm_kmalloc() in `drivers/base/devres.c`,
because devm_kzalloc() returns devm_kmalloc().
And it says that "Memory allocated with this function is automatically
freed on driver detach."
So there is no need to free the memory manually.
Is that right?
And I am sorry again because of the lack of the above explanation in my
commit message.
I will also add to my new commit.
Thanks,
Jiang
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-12-28 2:04 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-28 1:57 Re: [PATCH] staging: greybus: audio: Check null pointer Jiasheng Jiang
2021-12-28 2:02 Jiasheng Jiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.