When gk20a_clk_ctor() returns an error code, pointer "clk" should be released.
Such an information is reasonable.
It's the same when gm20b_clk_new() returns from elsewhere following this call.
I suggest to reconsider the interpretation of the software situation once more. Can it be that the allocated clock object should be kept usable even after a successful return from this function?
Would you like to add the tag “Fixes” to the commit message?
Regards, Markus
It's the same when gm20b_clk_new() returns from elsewhere following this call.
I suggest to reconsider the interpretation of the software situation once more. Can it be that the allocated clock object should be kept usable even after a successful return from this function?
It's possible that we expect an usable clk pointer, though I could not find the exact usage yet. For security, I will release this pointer only on error paths in this function.
Would you like to add the tag “Fixes” to the commit message?
Thank you for your advice! I will add this tag in the next version of patch.
Regards, Dinghao
It's possible that we expect an usable clk pointer, though I could not find the exact usage yet.
I am curious if another developer would like to add helpful background information.
For security, I will release this pointer only on error paths in this function.
Do you tend to release objects (which are referenced by pointers)?
Regards, Markus
For security, I will release this pointer only on error paths in this function.
Do you tend to release objects (which are referenced by pointers)?
I just found that clk is referenced by pclk in this function. When clk is freed, pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk in this function and there is no bug here. Thank you for reminding me!
Regards, Dinghao
I just found that clk is referenced by pclk in this function. When clk is freed, pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk in this function and there is no bug here.
Can there be a need to release a clock object after a failed gk20a_clk_ctor() call?
Regards, Markus
I just found that clk is referenced by pclk in this function. When clk is freed, pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk in this function and there is no bug here.
Can there be a need to release a clock object after a failed gk20a_clk_ctor() call?
I think this mainly depends on pclk pointer. It seems that the caller of gm20b_clk_new() always expects pclk to be allocated unless it returns -ENOMEM, which means kzalloc() failed. If gk20a_clk_ctor() never returns such an error code, we may need not to release this clock object.
Regards, Dinghao
If gk20a_clk_ctor() never returns such an error code, we may need not to release this clock object.
Would you like to achieve complete exception handling also for this function implementation?
It seems that it's possible to get -ENOMEM from gk20a_clk_ctor(). The call chain is as follows: gk20a_clk_ctor() <- nvkm_clk_ctor() <- nvkm_notify_init()
When nvkm_notify_init() returns -ENOMEM, all of its callers (and callers of callers) will be influenced if there is a failed kzalloc inside which.
In this case, maybe we should check the return value of gk20a_clk_ctor() and release clk if it returns -ENOMEM. And many other functions also have the same issue (e.g., gm20b_clk_new_speedo0). Do you have any idea about this problem?
Regards, Dinghao
In this case, maybe we should check the return value of gk20a_clk_ctor() and release clk if it returns -ENOMEM.
All error situations (including failed memory allocations) can matter here.
And many other functions also have the same issue (e.g. gm20b_clk_new_speedo0).
I recommend to increase the error detection and improve the desired exception handling accordingly.
Regards, Markus
The original patch was basically fine. Just add a Fixes tag and resend.
regards, dan carpenter
The original patch was basically fine.
I propose to reconsider the interpretation of the software situation once more.
* Should the allocated clock object be kept usable even after a successful return from this function?
* How much do “destructor” calls matter here for (sub)devices?
Just add a Fixes tag and resend.
This tag can help also.
Regards, Markus
On Tue, Jun 02, 2020 at 01:10:34PM +0200, Markus Elfring wrote:
The original patch was basically fine.
I propose to reconsider the interpretation of the software situation once more.
- Should the allocated clock object be kept usable even after a successful return from this function?
Heh. You're right. The patch is freeing "clk" on the success path so that doesn't work.
regards, dan carpenter
On Tue, Jun 02, 2020 at 01:10:34PM +0200, Markus Elfring wrote:
The original patch was basically fine.
I propose to reconsider the interpretation of the software situation once more.
- Should the allocated clock object be kept usable even after a successful return from this function?
Heh. You're right. The patch is freeing "clk" on the success path so that doesn't work.
Ben has explained this problem: https://lore.kernel.org/patchwork/patch/1249592/ Since the caller will check "pclk" on failure, we don't need to free "clk" in gm20b_clk_new() and I think this patch is no longer needed.
Regards, Dinghao
Ben has explained this problem: https://lore.kernel.org/patchwork/patch/1249592/ Since the caller will check "pclk" on failure, we don't need to free "clk" in gm20b_clk_new() and I think this patch is no longer needed.
* I am curious if it can become easier to see the relationships for these variables according to mentioned “destructor” calls.
* Did you notice opportunities to improve source code analysis (or software documentation) accordingly?
Regards, Markus
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
dinghao.liu＠zju.edu.cn -
Markus Elfring