From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752816AbeB1SKb (ORCPT ); Wed, 28 Feb 2018 13:10:31 -0500 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:58692 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751803AbeB1SK3 (ORCPT ); Wed, 28 Feb 2018 13:10:29 -0500 Date: Wed, 28 Feb 2018 19:10:19 +0100 From: Cornelia Huck To: Tony Krowiak Cc: Pierre Morel , linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, freude@de.ibm.com, schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, borntraeger@de.ibm.com, kwankhede@nvidia.com, bjsdjshi@linux.vnet.ibm.com, pbonzini@redhat.com, alex.williamson@redhat.com, alifm@linux.vnet.ibm.com, mjrosato@linux.vnet.ibm.com, jjherne@linux.vnet.ibm.com, thuth@redhat.com, pasic@linux.vnet.ibm.com, fiuczy@linux.vnet.ibm.com, buendgen@de.ibm.com Subject: Re: [PATCH v2 05/15] s390: vfio-ap: base implementation of VFIO AP device driver Message-ID: <20180228191019.20507888.cohuck@redhat.com> In-Reply-To: References: <1519741693-17440-1-git-send-email-akrowiak@linux.vnet.ibm.com> <1519741693-17440-6-git-send-email-akrowiak@linux.vnet.ibm.com> Organization: Red Hat GmbH MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 28 Feb 2018 11:43:37 -0500 Tony Krowiak wrote: > On 02/28/2018 10:33 AM, Pierre Morel wrote: > > On 27/02/2018 15:28, Tony Krowiak wrote: (...) > >> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig > >> index cbe1d97..9f23caf 100644 > >> --- a/arch/s390/Kconfig > >> +++ b/arch/s390/Kconfig > >> @@ -771,6 +771,14 @@ config VFIO_CCW > >> To compile this driver as a module, choose M here: the > >> module will be called vfio_ccw. > >> > >> +config VFIO_AP > >> + def_tristate m Any reason you default to m instead of n here? > >> + prompt "Support for virtual Adjunct Processor device interface" > > > > The VFIO AP devices are not virtual. > > What about > > "VFIO support for AP devices" > Sounds good. +1 > > > >> + depends on ZCRYPT && VFIO_MDEV_DEVICE > >> + help > >> + driver grants access to Adjunct Processor (AP) devices s/driver/This driver/ > >> + via the VFIO mediated device interface. You also might want to add "To compile this driver as a module, choose M here: the module will be called..." > >> + > >> endmenu It's a tad confusing to find this in the I/O submenu, but I don't really have a better idea. > >> > >> menu "Dump support" > >> diff --git a/arch/s390/configs/default_defconfig > >> b/arch/s390/configs/default_defconfig > >> index 5af8458..40fa3f6 100644 > >> --- a/arch/s390/configs/default_defconfig > >> +++ b/arch/s390/configs/default_defconfig > > > > Not sure that this file belongs to this patch > Neither am I, but at the time I inserted this here - well before August > of last year - I was using vfio-ccw as a model. > If someone can verify this does not belong here, I'd be more than happy > to remove it. I don't see any entry for VFIO_CCW in there? > > > > > >> @@ -719,3 +719,6 @@ CONFIG_APPLDATA_BASE=y > >> CONFIG_KVM=m > >> CONFIG_KVM_S390_UCONTROL=y > >> CONFIG_VHOST_NET=m > >> +VFIO_MDEV=m > >> +VFIO_MDEV_DEVICE=m > >> +CONFIG_VFIO_AP=m > > > > What is your goal when modifying this three files? > > Could you add a comment in the commit message? > As stated above, this was originally based on the vfio-ccw model and has > been in the > patch series since its inception. I'd be happy to remove it if it is not > necessary. I'd vote for removing it. (...) > >> +static int vfio_ap_matrix_dev_create(void) > >> +{ > >> + int ret; > >> + > >> + vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME); > >> + > >> + ret = PTR_ERR_OR_ZERO(vfio_ap_root_device); > > > > IS_ERR() is enough, root_device_register() never return NULL. > I searched the kernel code to look at other places the > root_device_register() > function is called to see how the return value is handled. I've seen all > of the > following used: > if (IS_ERR()) > ret = PTR_ERR() > PTR_ERR() > PTR_ERR_OR_ZERO() > > I'm not sure why this is a concern, but I'll use the first option above > since PTR_ERR_OR_ZERO() also embeds the first option. PTR_ERR_OR_ZERO() seems like the best choice for the way the return code is processed here. (It's just unfortunate that its name conjures up connotations of NULL-pointer handling.) > >> + if (ret) > >> + goto done;