From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1E502C433F5 for ; Wed, 13 Oct 2021 06:52:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 04C9C601FF for ; Wed, 13 Oct 2021 06:52:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238046AbhJMGyF (ORCPT ); Wed, 13 Oct 2021 02:54:05 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:59321 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231638AbhJMGyE (ORCPT ); Wed, 13 Oct 2021 02:54:04 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1634107921; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=RpuzMnNyT7+4FRREcT2u3Kwwl8W2CVjy5nh9V497X+I=; b=TqoevCt0y/2MtJ+PV0n/R8K80IHJJTwq6d8wl0zBhz7olq+8EopcsCbENN/KXIinitzzWH lTaotXVqWTxKTUq71uZhMDA8XHZAZOdClbrYLVoG9H60zv5QEu24QkSE1b5JtqnEx9xFVN D8Ra2rZc+eAUTmd6q0ijN9Ia96TebPY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-458-p66QTA95N5O1G1m0Lg_AvA-1; Wed, 13 Oct 2021 02:51:58 -0400 X-MC-Unique: p66QTA95N5O1G1m0Lg_AvA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6220079EDC; Wed, 13 Oct 2021 06:51:56 +0000 (UTC) Received: from localhost (unknown [10.39.193.85]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0844B4180; Wed, 13 Oct 2021 06:51:55 +0000 (UTC) From: Cornelia Huck To: Halil Pasic Cc: Pierre Morel , Vineeth Vijayan , Peter Oberparleiter , Heiko Carstens , Vasily Gorbik , Christian Borntraeger , Michael Mueller , linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, bfu@redhat.com, Halil Pasic Subject: Re: [RFC PATCH 1/1] s390/cio: make ccw_device_dma_* more robust In-Reply-To: <20211013003714.1c411f0b.pasic@linux.ibm.com> Organization: Red Hat GmbH References: <20211011115955.2504529-1-pasic@linux.ibm.com> <466de207-e88d-ea93-beec-fbfe10e63a26@linux.ibm.com> <874k9ny6k6.fsf@redhat.com> <20211011204837.7617301b.pasic@linux.ibm.com> <87pmsawdvr.fsf@redhat.com> <20211013003714.1c411f0b.pasic@linux.ibm.com> User-Agent: Notmuch/0.32.1 (https://notmuchmail.org) Date: Wed, 13 Oct 2021 08:51:54 +0200 Message-ID: <87mtndwh6d.fsf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 13 2021, Halil Pasic wrote: > On Tue, 12 Oct 2021 15:50:48 +0200 > Cornelia Huck 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