[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 3/5] megasas: bugfix
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v4 3/5] megasas: bugfix |
Date: |
Fri, 08 Apr 2016 09:16:49 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Please use a more descriptive title. Suggest "megasas: Fix
Cao jin <address@hidden> 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 <address@hidden>
> CC: Hannes Reinecke <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> ---
> 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.
- [Qemu-devel] [PATCH v4 0/5] Add param Error ** for msi_init(), Cao jin, 2016/04/05
- [Qemu-devel] [PATCH v4 2/5] change pvscsi_init_msi() type to void, Cao jin, 2016/04/05
- [Qemu-devel] [PATCH v4 4/5] mptsas: change .realize function name, Cao jin, 2016/04/05
- [Qemu-devel] [PATCH v4 1/5] fix some coding style problems, Cao jin, 2016/04/05
- [Qemu-devel] [PATCH v4 3/5] megasas: bugfix, Cao jin, 2016/04/05
- Re: [Qemu-devel] [PATCH v4 3/5] megasas: bugfix,
Markus Armbruster <=
- [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init(), Cao jin, 2016/04/05
- Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init(), Markus Armbruster, 2016/04/08
- Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init(), Cao jin, 2016/04/09
- Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init(), Cao jin, 2016/04/09
- Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init(), Marcel Apfelbaum, 2016/04/10
- Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init(), Cao jin, 2016/04/10
- Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init(), Marcel Apfelbaum, 2016/04/11
- Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init(), Cao jin, 2016/04/11