From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@novell.com>
Subject: Re: [PATCH] msi: Avoid uninitialized msi
	 descriptors
Date: Fri, 13 Aug 2010 13:07:12 +0100
Message-ID: <4C655190020000780000FB29@vpn.id2.novell.com>
References: <201008111543.03377.wei.wang2@amd.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <201008111543.03377.wei.wang2@amd.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Wei Wang2 <wei.wang2@amd.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>
List-Id: xen-devel@lists.xenproject.org

>>> On 11.08.10 at 15:43, Wei Wang2 <wei.wang2@amd.com> wrote:
> static int __pci_enable_msi(struct msi_info *msi, struct msi_desc =
**desc)
> {
>     int status;
>     struct pci_dev *pdev;
>+    struct msi_desc *old_desc;
>=20
>     ASSERT(spin_is_locked(&pcidevs_lock));
>     pdev =3D pci_get_pdev(msi->bus, msi->devfn);
>     if ( !pdev )
>         return -ENODEV;
>=20
>-    if ( find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSI) )
>+    old_desc =3D find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSI);
>+    if ( old_desc )
>     {
>         dprintk(XENLOG_WARNING, "irq %d has already mapped to MSI on "
>                 "device %02x:%02x.%01x.\n", msi->irq, msi->bus,
>                 PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn));
>+        *desc =3D old_desc;

While I agree to this part, ...

>         return 0;
>     }
>=20
>-    if ( find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX) )
>+    old_desc =3D find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
>+    if ( old_desc )
>     {
>         dprintk(XENLOG_WARNING, "MSI-X is already in use on "
>                 "device %02x:%02x.%01x\n", msi->bus,
>                 PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn));
>-        return 0;
>+        pci_disable_msi(old_desc);

... I don't think this one's right: Admittedly I should have changed
the return value from 0 to an actual error (e.g. -EBUSY) - I simply
overlooked that after doing the copy-and-paste operation.

Whether disabling and switching to the alternative mechanism
is the right thing to do here I don't know. But I'm pretty certain
that old_desc may now be leaked, as msi_free_irq() isn't being
called on it and set_irq_msi() also doesn't check whether
irq_desc[].msi_desc is already non-NULL.

Same thing (obviously) for the second part of the changes.

Jan