Date: Tue, 13 Nov 2018 19:11:11 +0100 From: Harry Schmalzbauer <freebsd@omnilan.de> To: Scott Long <scottl@samsco.org> Cc: scsi@freebsd.org, freebsd-stable <freebsd-stable@freebsd.org> Subject: Re: MSI allocation regression, still to be corrected in HEAD and please MFC before release/12.0 gets branched Message-ID: <d28eee74-99dc-296b-fd19-ea2c1adec0ee@omnilan.de> In-Reply-To: <05D94EC4-D797-4246-96B5-6DC1494E7EA9@samsco.org> References: <201707300653.v6U6rwLN099096@repo.freebsd.org> <597DA578.6030101@omnilan.de> <597F56A8.1060603@omnilan.de> <D18DFAD4-6E93-4AE2-BE15-EFF4D8ABCB2A@samsco.org> <59804C8C.1020003@omnilan.de> <e7d94e6a-89e8-ffa1-40da-7fb67e6bfc2b@omnilan.de> <78611650-D7A4-4B1D-A254-DB058E1AC1C6@samsco.org> <d99e383d-b09a-f3bd-f1e2-a6a808016347@omnilan.de> <A2AECF9F-2BB1-4FC0-8330-658336A3A4F0@samsco.org> <6e1e5f9f-4ece-dc9d-b059-08d52c9e6965@omnilan.de> <042090da-f73f-ce80-517c-0b1729d3d6e1@omnilan.de> <05D94EC4-D797-4246-96B5-6DC1494E7EA9@samsco.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Am 13.11.2018 um 19:02 schrieb Scott Long:
>
>
>> On Nov 12, 2018, at 10:03 AM, Harry Schmalzbauer <freebsd@omnilan.de> wrote:
>>
>> Am 11.06.2018 um 20:28 schrieb Harry Schmalzbauer:
>>> Am 05.06.2018 um 19:54 schrieb Scott Long:
>>> …
>>>>>>> Late in the 11.2 phase, I identified this commit as a regression for MSI (non-x) alloctaion.
>>>>>>> I have an idea what probably causes the problem here (INTx allocation, although MSI (and MSI-x) capability):
>>>>>>> disable_msix is not 0 (I need to disable MSI-x because of ESXi-passthru…).
>>>>>>>
>>>>>>> Corresponding lines:
>>>>>>> {
>>>>>>> device_t dev;
>>>>>>> int error, msgs;
>>>>>>>
>>>>>>> dev = sc->mps_dev;
>>>>>>> error = 0;
>>>>>>> msgs = 0;
>>>>>>>
>>>>>>> if ((sc->disable_msix == 0) &&
>>>>>>> ((msgs = pci_msix_count(dev)) >= MPS_MSI_COUNT))
>>>>>>> error = mps_alloc_msix(sc, MPS_MSI_COUNT);
>>>>>>> if ((error != 0) && (sc->disable_msi == 0) &&
>>>>>>> ((msgs = pci_msi_count(dev)) >= MPS_MSI_COUNT))
>>>>>>> error = mps_alloc_msi(sc, MPS_MSI_COUNT);
>>>>>>> if (error != 0)
>>>>>>> msgs = 0;
>>>>>>>
>>>>>>> sc->msi_msgs = msgs;
>>>>>>> return (error);
>>>>>>> }
>>>>>>>
>> …
>>>>>> Hi Harry,
>>>>>> You are correct about the bug. Please change the line at the top of the function that reads
>>>>>> error = 0;
>>>>>> to
>>>>>> error = ENXIO;
>>>>>> Let me know if that fixes the MSI problem for you.
>>>
>>> …
>>>
>> …
>>> Index: src/sys/dev/mps/mps_pci.c
>>> ===================================================================
>>> --- sys/dev/mps/mps_pci.c (Revision 334948)
>>> +++ sys/dev/mps/mps_pci.c (Arbeitskopie)
>>> @@ -244,7 +244,7 @@
>>> int error, msgs;
>>>
>>> dev = sc->mps_dev;
>>> - error = 0;
>>> + error = ENXIO;
>>> msgs = 0;
>>>
>>> if ((sc->disable_msix == 0) &&
>>>
>>
>> To my understanding, it's obvious that the way mps_pci_alloc_interrupts() currently works is unintended.
>> This might not affect too many people, but is there a reason not to fix it?
>>
>> I already created a coresponding problem report: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229267
>> Anything else I should do?
>>
>
> Hi Harry,
>
> Sorry for ignoring this for so long. I’m going to commit a fix today, but it won’t be the same one-line change.
> Upon reviewing the code, I’d going to refactor it so it’s not so confusing and prone to these kinds of mistakes.
> Thank you for the continued reminders to finish this.
Hi Scott,
thanks a lot, in fact I'm not surprised that you come up with a better
solution than that quick fix :-)
Had hoped someone else would do an intermediate commit to get it into
12.0 in time, so you won't feel any time pressure - good job needs the
time it needs, as long as the right person is doing the job.
Unfortunately I don't have a non-productive setup where I could test
before release/12.0 will be branched – might be subject to change...
best,
-harry
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?d28eee74-99dc-296b-fd19-ea2c1adec0ee>