Hi Bjorn,
On 11/10/2016 5:00 PM, Bjorn Helgaas wrote:
> On Thu, Nov 10, 2016 at 02:46:40PM +1100, Gavin Shan wrote:
>> On Wed, Nov 09, 2016 at 12:22:32PM -0600, Bjorn Helgaas wrote:
>>> [+cc Gavin, Amir, Majd, Alex]
>>>
>>> Hi Noa,
>>>
>>> On Tue, Nov 01, 2016 at 04:47:24PM +0200, Noa Osherovich wrote:
>>>> Mellanox devices were marked as having INTx masking ability broken.
>>> This was done by 11e42532ada3 ("PCI: Assume all Mellanox devices have
>>> broken INTx masking"), by Gavin Shan. Possibly that was a little too
>>> aggressive. I cc'd Gavin and Amir so they can comment.
>>>
>> I cannot know more HW details than Mellanox guys. So I assume the logic
>> in the code changes are correct. I totally agree with Bjron suggested
>> except one minor point: There will have two quirks for HEADER and FINAL
>> fixup separately as Bjorn suggested. One entry for one combination of
>> vendor/device will be put into section .pci_fixup_{header | final}.
>> The vendor/device IDs in the entries are compared with every PCI deivce
>> and the callbacks are invoked if they match. When number of entries in
>> section .pci_fixup_{header | final} increases, more time used to iterate
>> the section in pci_do_fixups(). The time will be saved if we can just
>> have one FINAL quirk as below, which probably makes code the easier to
>> be maintained.
>>
>> static void mellanox_check_broken_intx_masking(struct pci_dev *pdev)
>> {
>> if (pdev->device in broken_device_list)
>> quirk_broken_intx_masking(pdev);
>> else if (pdev->dev is CONNECT4)
>> /* Check the firmware version to call quirk_broken_intx_masking() */
>> }
>>
>> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_MELLANOX, PCI_ANY_ID, PCI_ANY_ID,
>> mellanox_check_broken_intx_masking);
>>
>> Above comment bases on the fact: pdev->broken_intx_masking is used by
>> device driver only. We still need anohter one HEADER quirk for Mellanox
>> (not one for vendor/device ID combination) if we want to see finalized
>> pdev->broken_intx_masking before that point.
> So you're proposing to make the quirk applicable to *all* Mellanox
> devices, then check against a list inside the quirk. You still have
> to maintain a table of the broken devices. I'm OK with that, and if
> you do it that way, it probably does make sense to combine them as you
> suggest.
>
> I agree that it would be simpler if these were all FINAL quirks. I
> don't think *any* of the quirk_broken_intx_masking() quirks need to be
> HEADER quirks. All of them could be FINAL quirks instead.
>
> I do *not* want a mix of HEADER & FINAL quirks that set
> broken_intx_masking. If we have a mix, it's not clear when it's
> really needed and it's not clear how to decide which to use.
>
> Now we're up to three patches:
>
> 1) Convert all quirk_broken_intx_masking() quirks (not just the
> Mellanox ones) from HEADER to FINAL.
>
> 2) Convert the Mellanox quirks from "all Mellanox devices" to the
> listed ones only, something like this:
>
> static u16 mellanox_broken_intx[] = { ... };
>
> static void mellanox_broken_intx_quirk(struct pci_dev *dev)
> {
> int i;
>
> for (i = 0; i < ARRAY_SIZE(mellanox_broken_intx); i++) {
> if (dev->device == mellanox_broken_intx[i]) {
> dev->broken_intx_masking = 1;
> return;
> }
> }
> }
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_MELLANOX, PCI_ANY_ID, PCI_ANY_ID,
> mellanox_broken_intx_quirk);
>
> 3) Add the CONNECT4-specific firmware version checks to
> mellanox_broken_intx_quirk().
Ack.
>>>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>>>> index c58752fe16c4..1c742842e76c 100644
>>>> --- a/include/linux/pci_ids.h
>>>> +++ b/include/linux/pci_ids.h
>>>> @@ -2262,6 +2262,22 @@
>>>> #define PCI_DEVICE_ID_MELLANOX_ARBEL 0x6282
>>>> #define PCI_DEVICE_ID_MELLANOX_SINAI_OLD 0x5e8c
>>>> #define PCI_DEVICE_ID_MELLANOX_SINAI 0x6274
>>>> +#define PCI_DEVICE_ID_MELLANOX_HERMON_SDR 0x6340
>>>> +#define PCI_DEVICE_ID_MELLANOX_HERMON_DDR 0x634a
>>>> +#define PCI_DEVICE_ID_MELLANOX_HERMON_QDR 0x6354
>>>> +#define PCI_DEVICE_ID_MELLANOX_HERMON_DDR_GEN2 0x6732
>>>> +#define PCI_DEVICE_ID_MELLANOX_HERMON_QDR_GEN2 0x673c
>>>> +#define PCI_DEVICE_ID_MELLANOX_HERMON_EN 0x6368
>>>> +#define PCI_DEVICE_ID_MELLANOX_HERMON_EN_GEN2 0x6750
>>>> +#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN 0x6372
>>>> +#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_T_GEN2 0x675a
>>>> +#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_GEN2 0x6764
>>>> +#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_5_GEN2 0x6746
>>>> +#define PCI_DEVICE_ID_MELLANOX_CONNECTX2 0x676e
>>>> +#define PCI_DEVICE_ID_MELLANOX_CONNECTX3 0x1003
>>>> +#define PCI_DEVICE_ID_MELLANOX_CONNECTX3_PRO 0x1007
>>>> +#define PCI_DEVICE_ID_MELLANOX_CONNECTX4 0x1013
>>>> +#define PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX 0x1015
> I forgot to mention: we normally don't add definitions to pci_ids.h
> unless they're used in more than one place (there's a comment about
> this at the top of pci_ids.h). So these device IDs should all just be
> listed directly in the quirks or in the static table of broken
> devices.
Ack again. I'll send the patches early next week.
> Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
