On Tue, 12 Oct 2021 15:50:48 +0200 Cornelia Huck <cohuck@xxxxxxxxxx> 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
