WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 2/3] xen-pcifront: Sanity check the MSI/MSI-X val

To: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 2/3] xen-pcifront: Sanity check the MSI/MSI-X values
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Thu, 17 Feb 2011 08:53:40 +0000
Cc: Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>, Jeremy Fitzhardinge <jeremy@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Thu, 17 Feb 2011 00:54:39 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1297894638-28007-3-git-send-email-konrad.wilk@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <1297894638-28007-1-git-send-email-konrad.wilk@xxxxxxxxxx> <1297894638-28007-3-git-send-email-konrad.wilk@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> Check the returned vector values for any values that are
> odd or plain incorrect (say vector value zero), and if so
> print a warning. Also fixup the return values.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  drivers/pci/xen-pcifront.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index 3a5a6fc..6acf6ae 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -277,13 +277,20 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
>       if (likely(!err)) {
>               if (likely(!op.value)) {
>                       /* we get the result */
> -                     for (i = 0; i < nvec; i++)
> +                     for (i = 0; i < nvec; i++) {
> +                             if (op.msix_entries[i].vector <= 0) {
> +                                     dev_warn(&dev->dev, "MSI-X entry %d" \
> +                                             " is zero!\n", i);

The test says "<= 0" but the text says "== 0". Perhaps
                                        dev_warn(&dev->dev, "MSI-X entry %d has 
invalid vector %d\n",
                                                 i, op.msix_entries[i].vector);

> 
> +                                     err = -EINVAL;
> +                                     continue;

Do we need / should we set *(*vector+i) to something to indicate its
invalidness rather than leave it potentially uninitialised?

> +                             }
>                               *(*vector+i) = op.msix_entries[i].vector;

BTW does the double indirection of the vector serve a purpose?
Everywhere I can see just updates *(*vector+i), I can't see any realloc
of the array itself etc.

Removing the extra level of indirection leads to vector[i] = foo instead
which is much easier on the eye.

> -                     return 0;
> +                     }
> +                     return err;
>               } else {
>                       printk(KERN_DEBUG "enable msix get value %x\n",
>                               op.value);
> -                     return op.value;
> +                     return err;

I think all of these "return err"s can now be pulled out to the end of
the function? makes it clearer what the return is.

>               }
>       } else {
>               dev_err(&dev->dev, "enable msix get err %x\n", err);
> @@ -325,6 +332,10 @@ static int pci_frontend_enable_msi(struct pci_dev *dev, 
> int **vector)
>       err = do_pci_op(pdev, &op);
>       if (likely(!err)) {
>               *(*vector) = op.value;
> +             if (op.value <= 0) {
> +                     dev_warn(&dev->dev, "MSI entry is zero!\n");

Same comment re <= vs == 0.

> +                     err = -EINVAL;
> +             }
>       } else {
>               dev_err(&dev->dev, "pci frontend enable msi failed for dev "
>                                   "%x:%x\n", op.bus, op.devfn);



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel