Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752186AbcKIDfE (ORCPT ); Tue, 8 Nov 2016 22:35:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34002 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751716AbcKIDfC (ORCPT ); Tue, 8 Nov 2016 22:35:02 -0500 Date: Tue, 8 Nov 2016 20:35:00 -0700 From: Alex Williamson To: Alexey Kardashevskiy Cc: Kirti Wankhede , pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, jike.song@intel.com, bjsdjshi@linux.vnet.ibm.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() Message-ID: <20161108203500.526244d6@t450s.home> In-Reply-To: <26a4171a-e68f-c8e5-af39-b5af2c1645fe@ozlabs.ru> References: <1478293856-8191-1-git-send-email-kwankhede@nvidia.com> <1478293856-8191-16-git-send-email-kwankhede@nvidia.com> <26a4171a-e68f-c8e5-af39-b5af2c1645fe@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 09 Nov 2016 03:35:02 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4356 Lines: 134 On Wed, 9 Nov 2016 14:07:58 +1100 Alexey Kardashevskiy wrote: > On 09/11/16 07:22, Kirti Wankhede wrote: > > On 11/8/2016 2:16 PM, Alexey Kardashevskiy wrote: > >> On 05/11/16 08:10, Kirti Wankhede wrote: > >>> Vendor driver using mediated device framework would use same mechnism to > >>> validate and prepare IRQs. Introducing this function to reduce code > >>> replication in multiple drivers. > >>> > >>> Signed-off-by: Kirti Wankhede > >>> Signed-off-by: Neo Jia > >>> Change-Id: Ie201f269dda0713ca18a07dc4852500bd8b48309 > >>> --- > >>> drivers/vfio/vfio.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > >>> include/linux/vfio.h | 4 ++++ > >>> 2 files changed, 52 insertions(+) > >>> > >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > >>> index 9a03be0942a1..ed2361e4b904 100644 > >>> --- a/drivers/vfio/vfio.c > >>> +++ b/drivers/vfio/vfio.c > >>> @@ -1858,6 +1858,54 @@ int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id, > >>> } > >>> EXPORT_SYMBOL(vfio_info_add_capability); > >>> > >>> +int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int num_irqs, > >>> + int max_irq_type, size_t *data_size) > >>> +{ > >>> + unsigned long minsz; > >>> + size_t size; > >>> + > >>> + minsz = offsetofend(struct vfio_irq_set, count); > >>> + > >>> + if ((hdr->argsz < minsz) || (hdr->index >= max_irq_type) || > >>> + (hdr->count >= (U32_MAX - hdr->start)) || > >>> + (hdr->flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK | > >>> + VFIO_IRQ_SET_ACTION_TYPE_MASK))) > >>> + return -EINVAL; > >>> + > >>> + if (data_size) > >> > >> Pointless check, the callers will pass non null pointer with value > >> initialized to 0 anyway. > >> > > > > Not always, When VFIO_IRQ_SET_DATA_NONE flag is set, caller can pass > > data_size = NULL. > > > Today data_size is not NULL in all cases and the way it is used now (ioctl > VFIO_DEVICE_SET_IRQS) gives me an idea that this is not going to change. > > > > >> > >>> + *data_size = 0; > >>> + > >>> + if (hdr->start >= num_irqs || hdr->start + hdr->count > num_irqs) > >>> + return -EINVAL; > >>> + > >>> + switch (hdr->flags & VFIO_IRQ_SET_DATA_TYPE_MASK) { > >>> + case VFIO_IRQ_SET_DATA_NONE: > >>> + size = 0; > >>> + break; > >>> + case VFIO_IRQ_SET_DATA_BOOL: > >>> + size = sizeof(uint8_t); > >>> + break; > >>> + case VFIO_IRQ_SET_DATA_EVENTFD: > >>> + size = sizeof(int32_t); > >>> + break; > >>> + default: > >>> + return -EINVAL; > >>> + } > >>> + > >>> + if (size) { > >> > >> The whole branch would even work for size == 0. > >> > > > > In that case below check (!data_size) might result in error if data_size > > == NULL, whereas its not error case when size == 0, i.e. > > VFIO_IRQ_SET_DATA_NONE flag set. > > > >>> + if (hdr->argsz - minsz < hdr->count * size) > >>> + return -EINVAL; > >>> + > >>> + if (!data_size) > >>> + return -EINVAL; > >> > >> Redundant check as well. > >> > > > > This is not redundant. If you see above check, it sets its init value to > > 0 but doesn't fail. > > > >>> + > >>> + *data_size = hdr->count * size; > >>> + } > >>> + > >>> + return 0; > >>> +} > >> > >> It does not really prepare anything as the name suggests. It looks like > >> this is 2 different helpers actually: > >> > >> int vfio_set_irqs_validate() > >> and > >> size_t vfio_set_irqs_hdr_to_data_size() > >> > > > > Later one is the prepare. > > > Does not like it prepares anything, just a simple converter. > > > >> And it would make it easier to review/bisect if 16/22 and 17/22 were merged > >> into this one as this patch alone adds new code which it does not use and > >> all 3 patches are fairly small. > >> > > > > I do had all 3 patch merged in one in earlier version of patchset. This > > is split as per Alex's suggestion. > > I got this from another mail from Alex. Which I find strange but whatever, > this is his realm anyway :) Maybe you haven't noticed, but your patch series are often difficult to deal with, they almost always split across functional areas and maintainers. Splitting out code to common functions and _then_ updating the callers to make use of it is a common way to deal with that. We're in the same functional area here, but it's still good practice. Thanks, Alex