On Tue, 12 Oct 2021 15:50:48 +0200 Cornelia Huck cohuck@redhat.com wrote:
If I read cio_gp_dma_zalloc() correctly, we either get NULL or a valid address, so yes.
I don't think the extra care will hurt us too badly. I prefer to keep the IS_ERR_OR_NULL() check because it needs less domain specific knowledge to be understood, and because it is more robust.
It feels weird, though -- I'd rather have a comment that tells me
This way the change feels simpler and safer to me. I believe I explained the why above. But if you insist I can change it. I double checked the cio_gp_dma_zalloc() code, and more or less the code called by it. So now I don't feel uncomfortable with the simpler check.
On the other hand, I'm not very happy doing changes solely based on somebody's feelings. It would feel much more comfortable with a reason based discussion.
One reason to change this to a simple NULL check, is that the IS_ERR_OR_NULL() check could upset the reader of the client code, which only checks for NULL.
On the other hand I do believe we have some risk of lumping together different errors here. E.g. dma_pool is NULL or dma ops are not set up properly. Currently we would communicate that kind of a problem as -ENOMEM, which wouldn't be a great match. But since dma_alloc_coherent() returns either NULL or a valid pointer, and furthermore this looks like a common thing in all the mm-api, I decided to be inline with that.
TLDR; If you insist, I will change this to a simple null pointer check.
exactly what cio_gp_dma_zalloc() is supposed to return; I would have expected that a _zalloc function always gives me a valid pointer or NULL.
I don't think we have such a comment for dma_alloc_coherent() or even kmalloc(). I agree, it would be nice to have this behavior documented in the apidoc all over the place. But IMHO that is a different issue.
Regards, Halil