Re: [RFC PATCH 1/1] s390/cio: make ccw_device_dma_* more robust

From: Cornelia Huck <cohuck@redhat.com>
To: Halil Pasic <pasic@linux.ibm.com>
Cc: Pierre Morel <pmorel@linux.ibm.com>,
	Vineeth Vijayan <vneethv@linux.ibm.com>,
	Peter Oberparleiter <oberpar@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Michael Mueller <mimu@linux.ibm.com>,
	linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org, bfu@redhat.com,
	Halil Pasic <pasic@linux.ibm.com>
Subject: Re: [RFC PATCH 1/1] s390/cio: make ccw_device_dma_* more robust
Date: Wed, 13 Oct 2021 08:51:54 +0200	[thread overview]
Message-ID: <87mtndwh6d.fsf@redhat.com> (raw)
In-Reply-To: <20211013003714.1c411f0b.pasic@linux.ibm.com>

On Wed, Oct 13 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> 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.

So, I think that a function returning NULL/valid pointer is the more
expected case, and functions that can return an error as well should
document this. But it's not really worth arguing more about this, as
this is not my code anyway, and your patch does look correct.

Acked-by: Cornelia Huck <cohuck@redhat.com>