| Message ID | 1459855602-16727-4-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Please use a more descriptive title. Suggest "megasas: Fix Cao jin <caoj.fnst@cn.fujitsu.com> 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 <caoj.fnst@cn.fujitsu.com> > CC: Hannes Reinecke <hare@suse.de> > CC: Paolo Bonzini <pbonzini@redhat.com> > --- > 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.
On 04/08/2016 03:16 PM, Markus Armbruster wrote: > Please use a more descriptive title. Suggest "megasas: Fix > > Cao jin <caoj.fnst@cn.fujitsu.com> 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, FYI: if realize it again, I guess msi_init() won`t be executed again, because megasas_use_msi() will fail first. > 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). >
On 04/08/2016 10:16 AM, Markus Armbruster wrote: > Please use a more descriptive title. Suggest "megasas: Fix > > Cao jin <caoj.fnst@cn.fujitsu.com> 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 <caoj.fnst@cn.fujitsu.com> >> CC: Hannes Reinecke <hare@suse.de> >> CC: Paolo Bonzini <pbonzini@redhat.com> >> --- >> 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 >
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) &&
msi_init returns non-zero value on both failure and success Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> CC: Hannes Reinecke <hare@suse.de> CC: Paolo Bonzini <pbonzini@redhat.com> --- hw/scsi/megasas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)