From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49811) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aoQf2-0004qu-CE for qemu-devel@nongnu.org; Fri, 08 Apr 2016 03:16:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aoQey-0002Tc-CA for qemu-devel@nongnu.org; Fri, 08 Apr 2016 03:16:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56935) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aoQey-0002Sw-7A for qemu-devel@nongnu.org; Fri, 08 Apr 2016 03:16:52 -0400 From: Markus Armbruster References: <1459855602-16727-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1459855602-16727-4-git-send-email-caoj.fnst@cn.fujitsu.com> Date: Fri, 08 Apr 2016 09:16:49 +0200 In-Reply-To: <1459855602-16727-4-git-send-email-caoj.fnst@cn.fujitsu.com> (Cao jin's message of "Tue, 5 Apr 2016 19:26:40 +0800") Message-ID: <87a8l4tvry.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v4 3/5] megasas: bugfix List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin Cc: qemu-devel@nongnu.org, mst@redhat.com, jasowang@redhat.com, marcel@redhat.com, alex.williamson@redhat.com, hare@suse.de, dmitry@daynix.com, pbonzini@redhat.com, jsnow@redhat.com, kraxel@redhat.com Please use a more descriptive title. Suggest "megasas: Fix Cao jin writes: > msi_init returns non-zero value on both failure and success This is a sentence, should end with a period. Bug's impact? Here's my guess. msi_init() either succeeds and returns 0x50, or fails and returns a negative errno. If it succeeds, we mistakenly clear MEGASAS_MASK_USE_MSI. Its only use is in megasas_scsi_uninit(), via megasas_use_msi(). There, we fail to msi_uninit() on unrealize due to the bug. I figure that's harmless if we destroy the device next. This is the common case. If we don't destroy it, and then realize it again, msi_init() fails, because there's no space at 0x50: the MSI capability we neglected to delete is still there. We report the problem to the user, then realize the device anyway (I hate that, but it's a separate issue). Marcel, can you confirm my analysis? > Signed-off-by: Cao jin > CC: Hannes Reinecke > CC: Paolo Bonzini > --- > hw/scsi/megasas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index a63a581..56fb645 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -2348,7 +2348,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) > "megasas-queue", 0x40000); > > if (megasas_use_msi(s) && > - msi_init(dev, 0x50, 1, true, false)) { > + msi_init(dev, 0x50, 1, true, false) < 0) { > s->flags &= ~MEGASAS_MASK_USE_MSI; > } > if (megasas_use_msix(s) && msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { s->flags &= ~MEGASAS_MASK_USE_MSIX; } This looks like the same bug, but it's actually okay, since msix_init() returns 0 on success. Suggest to test < 0 anyway so that future readers don't get misled into thinking there's a bug like I was. Marcel, this difference between msi_init() and msix_init() is just mean. Please clean it up.