From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH 3/4] VT-d/qinval: clean up error handling
Date: Mon, 16 Jun 2014 14:55:50 +0100
Message-ID: <539EF766.8070803@citrix.com>
References: <539F025A020000780001AA25@mail.emea.novell.com>
	<539F03D0020000780001AA3C@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <Andrew.Cooper3@citrix.com>) id 1WwXO8-00031v-Qs
	for xen-devel@lists.xenproject.org; Mon, 16 Jun 2014 13:55:57 +0000
In-Reply-To: <539F03D0020000780001AA3C@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: Yang Z Zhang <yang.z.zhang@intel.com>, xen-devel <xen-devel@lists.xenproject.org>, xiantao.zhang@intel.com
List-Id: xen-devel@lists.xenproject.org

On 16/06/14 13:48, Jan Beulich wrote:
> - neither qinval_update_qtail() nor qinval_next_index() can fail: make
>   the former return "void", and drop caller error checks for the latter
>   (all of which would otherwise return with a spin lock still held)
> - or-ing together error codes is a bad idea
>
> At once drop bogus initializers.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -58,7 +58,7 @@ static int qinval_next_index(struct iomm
>      return tail;
>  }
>  
> -static int qinval_update_qtail(struct iommu *iommu, int index)
> +static void qinval_update_qtail(struct iommu *iommu, int index)
>  {
>      u64 val;
>  
> @@ -66,7 +66,6 @@ static int qinval_update_qtail(struct io
>      ASSERT( spin_is_locked(&iommu->register_lock) );
>      val = (index + 1) % QINVAL_ENTRY_NR;
>      dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT));
> -    return 0;
>  }
>  
>  static int gen_cc_inv_dsc(struct iommu *iommu, int index,
> @@ -100,17 +99,15 @@ static int gen_cc_inv_dsc(struct iommu *
>  int queue_invalidate_context(struct iommu *iommu,
>      u16 did, u16 source_id, u8 function_mask, u8 granu)
>  {
> -    int ret = -1;
> +    int ret;
>      unsigned long flags;
>      int index = -1;

This index initialiser is also bogus.  It is assigned straight from
qinval_next_index().

But peeking ahead to your next patch, you have addressed it there.

I would suggest that the dropping of -EBUSY clauses would more logically
live with the following patch, but the end result of patches 3 + 4
appear fine.

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>