From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59552) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ap9ys-0008RA-4M for qemu-devel@nongnu.org; Sun, 10 Apr 2016 03:40:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ap9yn-0000zQ-R9 for qemu-devel@nongnu.org; Sun, 10 Apr 2016 03:40:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39375) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ap9yn-0000z8-Iw for qemu-devel@nongnu.org; Sun, 10 Apr 2016 03:40:21 -0400 References: <1459855602-16727-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1459855602-16727-4-git-send-email-caoj.fnst@cn.fujitsu.com> <87a8l4tvry.fsf@dusky.pond.sub.org> From: Marcel Apfelbaum Message-ID: <570A0357.9030106@redhat.com> Date: Sun, 10 Apr 2016 10:40:07 +0300 MIME-Version: 1.0 In-Reply-To: <87a8l4tvry.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 3/5] megasas: bugfix List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Cao jin Cc: qemu-devel@nongnu.org, mst@redhat.com, jasowang@redhat.com, alex.williamson@redhat.com, hare@suse.de, dmitry@daynix.com, pbonzini@redhat.com, jsnow@redhat.com, kraxel@redhat.com On 04/08/2016 10:16 AM, Markus Armbruster wrote: > 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? Your analysis is accurate, I didn't even look so hard at consequences, this is a clear bug that needs to be fixed. However, now I looked into it and your explanation shows why it even works... > >> 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. > I agree we should follow the same convention. > Marcel, this difference between msi_init() and msix_init() is just mean. It keeps us alert :) > Please clean it up. Sure, I'll take care of it. Thanks, Marcel >