Fix mlx4_pci_table's .driver_data

[decui_microsoft.com]

Summary

When playing with Mellanox ConnectX-3 virtual function (Device ID = 0x1004),
I found the driver could't be loaded due to
kernel: mlx4_core0: Missing DCS, aborting.(driver_data: 0x0, pci_resource_flags(pdev, 0):0x0)

This is because MLX4_PCI_DEV_IS_VF is assigned to the "class_mask" member
of the struct pci_device_id. Let's fix this by explicit assignment.

And, we don't use ".driver_data = 0" for some lines, because by default the
member is initialized with 0.

[cem]

How does this change fix the problem described?

sys/compat/linuxkpi/common/include/linux/pci.h

61 ↗

(On Diff #22907)

What does this fix? What uses this?

93 ↗

(On Diff #22907)

Non-present fields are zero initialized by default. So this is a no-op?

If you want to explicitly initialize those two fields, at least use .class = ..., .class_mask = ....

96 ↗

(On Diff #22907)

Should PCI_DEVICE() also explicitly initialize these fields?

[decui_microsoft.com]

sys/compat/linuxkpi/common/include/linux/pci.h

61 ↗

(On Diff #22907)

Actually the "class" and "class_mask" fields in the struct are not used at present.

In sys/dev/mlx4/mlx4_core/mlx4_main.c:

static DEFINE_PCI_DEVICE_TABLE(mlx4_pci_table) = {
<... snipped... >

/* MT27500 Family [ConnectX-3 Virtual Function] */
{ PCI_VDEVICE(MELLANOX, 0x1004), MLX4_PCI_DEV_IS_VF },

Here MLX4_PCI_DEV_IS_VF should be assigned to the "driver_data" field in the struct pci_device_id -- without the patch, it's assigned to the "class_mask" field...

So in __mlx4_init_one() we get the error:

if (!(pci_dev_data & MLX4_PCI_DEV_IS_VF) &&
     !(pci_resource_flags(pdev, 0) & IORESOURCE_MEM)) {
         dev_err(&pdev->dev, "Missing DCS, aborting."
                 "(driver_data: 0x%x, pci_resource_flags(pdev, 0):0x%x)\n",
                 pci_dev_data, pci_resource_flags(pdev, 0));
         err = -ENODEV;
         goto err_disable_pdev;
 }

93 ↗

(On Diff #22907)

Yes, the style you suggested is better.
Here I'm trying to use the same style in the related Linux side. I suppose the linuxkpi code should be as close to the related Linux code as possible?

96 ↗

(On Diff #22907)

The only user of PCI_DEVICE() is ofed/drivers/infiniband/hw/mthca/mthca_main.c, which doesn't have a bug.

Personally I'd like to keep it as is, since the Linux side's related definition is the same.

[• hselasky]

sys/compat/linuxkpi/common/include/linux/pci.h

61 ↗

(On Diff #22907)

I think you should also change the "mlx4_pci_table()" to use .driver_data = MLX4_PCI_DEV_IS_VF so that we are not dependent on the compiler.

93 ↗

(On Diff #22907)

In FreeBSD record initialization of structures is preferred. It makes updating the structures easier. The LinuxKPI doesn't have to be identical to Linux. It's quirks and caveats should be understood before using it with real Linux driver code :-)

[decui_microsoft.com]

sys/compat/linuxkpi/common/include/linux/pci.h

61 ↗

(On Diff #22907)

Ok. So I should not change sys/compat/linuxkpi/common/include/linux/pci.h, and I should fix mlx4_pci_table.
Will update the patch.

93 ↗

(On Diff #22907)

Got it.

[• hselasky]

sys/compat/linuxkpi/common/include/linux/pci.h

61 ↗

(On Diff #22907)

Yes, that's correct.

[decui_microsoft.com]

updated the patch.

[• hselasky]

Looks good. Don't forget to MFC to 9/10 and 11. Note that the directory layout is different in 9/10/11 so you'll need to manually extract the patch and apply it. If you think this is difficult I can do the commit for you giving you the credit and handle the MFC-ing.

[decui_microsoft.com]

In D8791#182029, @hselasky wrote:

Looks good. Don't forget to MFC to 9/10 and 11. Note that the directory layout is different in 9/10/11 so you'll need to manually extract the patch and apply it. If you think this is difficult I can do the commit for you giving you the credit and handle the MFC-ing.

Please help to do all these since I know you're much more familiar with the code. Thank you! I have little confidence to cope with the build system. :-)

[• hselasky]

Submitted as r310058 to FreeBSD head:
https://svnweb.freebsd.org/changeset/base/310058

[cem]

LGTM as is. Yeah, we *don't* want to just copy Linux code verbatim for licensing reasons. In this case, I think designated initializers are also usually the better approach.