Re: [PATCH 3/4] VT-d/qinval: clean up error handling

[Andrew Cooper <andrew.cooper3@citrix.com>]

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>