Page MenuHomeFreeBSD

Fix mlx4_pci_table's .driver_data
ClosedPublic

Authored by decui_microsoft.com on Dec 14 2016, 10:46 AM.

Details

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.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

decui_microsoft.com retitled this revision from to linuxkpi: Fix the struct pci_device_id.
decui_microsoft.com updated this object.
decui_microsoft.com edited the test plan for this revision. (Show Details)
cem edited edge metadata.Dec 14 2016, 10:51 AM

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?

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 added inline comments.Dec 14 2016, 12:25 PM
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 :-)

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 added inline comments.Dec 14 2016, 1:04 PM
sys/compat/linuxkpi/common/include/linux/pci.h
61 ↗(On Diff #22907)

Yes, that's correct.

decui_microsoft.com retitled this revision from linuxkpi: Fix the struct pci_device_id to Fix mlx4_pci_table's .driver_data.
decui_microsoft.com updated this object.
decui_microsoft.com edited edge metadata.

updated the patch.

hselasky accepted this revision.Dec 14 2016, 1:41 PM
hselasky edited edge metadata.

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.

This revision is now accepted and ready to land.Dec 14 2016, 1:41 PM

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. :-)

cem added a comment.Dec 14 2016, 7:34 PM

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.