Re: [Qemu-devel] [PATCH v4 3/5] megasas: bugfix

[Marcel Apfelbaum <marcel@redhat.com>]

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

>