linuxkpi: Add more pci functions needed by DRM

[manu]

Summary

• pci_get_class : This function search for a matching pci device based on the class/subclass and returns a newly created pci_dev.
• pci_{save,restore}_state : This is analogous to ours with the same name
• pci_is_root_bus : Return true if this is the root bus
• pci_get_domain_bus_and_slot : This function search for a matching pci device based on domain, bus and slot/function concat into a single unsigned int (devfn) and returns a newly created pci_dev
• pci_bus_{read,write}_config* : Read/Write to the config space.
• Change PCI_DEVFN/PCI_FUNC/PCI_SLOT Since we use pci_find_dbsf to find the correct device we need to pass the full function address in case the device is an ARI one.

While here add some helper function to alloc and fill the pci_dev struct.

Test Plan

Tested with https://github.com/freebsd/drm-kmod/commit/725d6101dbccb3b2fbb415a0672e00b64535838e on amd and intel.

[bz]

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

720

Do we need to have the defines for inline functions? I thought in that case we could just use the original names directly?

1085

long line.

1090

Is there are reason this is not an inline function?

sys/compat/linuxkpi/common/src/linux_pci.c

264

Given the two blocks from malloc() down are the same apart from "devfn", can they be one internal function or two as the lower half is another copy from linux_pci_attach_device() if I am not mistaken?

I was looking at that for linux_pci_attach_device() yesterday as for the dev the release function doesn't exist and it's highly confusing how the pci or the dev get allocated and the naming of things. So I am happy the less duplicated "alloc" code we have.

Not need to do, if you don't want to, just a suggestion in this case.

422

I am still pondering if this will work in all cases; I'll go and try to try/check tonight.

[manu]

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

720

Nope, this was a copy paste from drm-kmod but this should be directly using the linux name. Will change that.

1090

Guess it could be. Will change.

sys/compat/linuxkpi/common/src/linux_pci.c

264

Yeah I can do a generic alloc function.

422

Why will it not work in some case ?

[bz]

sys/compat/linuxkpi/common/src/linux_pci.c

422

Because from what I can see on network drivers pci_dev_get() and pci_dev_put() are reference operations, I'd assume around the device and not direct operations. I also see pdev accesses after pci_dev_put() so it cannot be freed at that point:

iwlwifi/pcie/trans.c::iwl_trans_pcie_removal_wk()
        pci_dev_put(pdev);
        pci_stop_and_remove_bus_device(pdev);

It's the only call I have there so I had ignored it so far.

I assume the above just works for you in your special case.

I further assume the normal implementation would be similar to device_get/put():

static __inline void /* XXX-BZ returns something? */
pci_dev_get(struct pci_dev *pdev)
{
    if (pdev != NULL)
        device_get(&pdev->dev);
}

static __inline void
pci_dev_put(struct pci_dev *pdev)
{
        if (pdev != NULL)
           device_put(&pdev->dev);
}

I believe to achieve what you seem to want/need you'd want to do the following:

Rename this function to

static void
linuxkpi_pci_release_direct(struct device *dev)
(
... fix/cast dev to pdev
}

and move it up before your two new linuxkpi_pci_get_* functions or a new alloc function; there for the struct pci_dev *dev you want to set:

pdev->dev.release = linuxkpi_pci_release_direct;

But that means that you probably also have to set the parent and init the kobj in your two get_* or alloc functions above as does linux_pci_attach_device(). That means embracing the struct device which embedded in pci_dev as first argument rather than working around it. And you don't have to use it much more than that I'd assume.

I haven't tried this; only put it down from the top of my head.

PS: yes, linux_pci_attach_device() also lacks a release function for devres; this is how I got to figure that out yesterday.

[manu]

sys/compat/linuxkpi/common/src/linux_pci.c

422

Yeah I had a look at what linux is doing and you're right, it's just device_put.
I'll see at how to fix that.

[manu]

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

720

Well no it's needed as we have the same functions defined in pcivar.h so I'll just rename them to linuxkpi_

[bz]

I haven't tested it but this looks good, and thank you for harmonising the alloc! Much appreciated!
I think you may want to wait for @hselasky or @kib for the mlx[45] changes as well. (I mention as neither of them is a linuxkpi member yet from what I can see).

sys/compat/linuxkpi/common/src/linux_pci.c

264

THANK YOU!

[manu]

In D27550#616107, @bz wrote:

I haven't tested it but this looks good, and thank you for harmonising the alloc! Much appreciated!
I think you may want to wait for @hselasky or @kib for the mlx[45] changes as well. (I mention as neither of them is a linuxkpi member yet from what I can see).

They were in the subscriber (I guess herald rules on mlx path) but I've added them to the reviewer.
Thanks for you time reviewing this.

[• hselasky]

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

1114

This doesn't look right. I think you need a temporary uint32_t variable before storing into uint16_t.

Ditto below.

[kib]

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

1106

This breaks for ARI devices. In particular, PCI_FUNC() is broken, it truncates the function address.

Arguably it is more an issue with PCI_FUNC() macro.

1114

Yes this only work on little-endian.

1132

Same.

[manu]

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

1106

What's ARI devices ?

1114

Right ok, will fix.
This is the only part not tested as it's only used by the radeon and I don't have such device.

[kib]

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

1106

ARI is PCIe cap which indicates that instead of using the second byte of the device address for slot:func, whole second byte is dedicated to the function number. This way device might have 255 siblings instead of 31, which is useful/needed for things like SRIOV.

[• hselasky]

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

100

These definitions do no longer match what Linux defines them to be.
Are you sure this is OK?

[• hselasky]

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

100

See the mlx4 driver for example, how it assumes how the current macros work.

static int extended_func_num(struct pci_dev *pdev)
{
        return PCI_SLOT(pdev->devfn) * 8 + PCI_FUNC(pdev->devfn);
}

[manu]

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

100

It doesn't matter as we have setup the devfn with the new macro.

[kib]

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

1139

This does not work with ARI.

[manu]

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

1139

Ok, how so ?

[kib]

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

101

This does not work with non-ARI devices.

1139

I see, you changed PCI_SLOT()/PCI_FUNC() definitions. But now PCI_SLOT() would break
for non-ARI.

With this KPI, I believe you must use PCIB_ARI_ENABLED() to get it right.

[manu]

Simplify pci_bus_{read,write}_config
Revert PCI_FUNC/PCI_SLOT changes as it's not needed anymore.

[bz]

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

100

Is there a reason we do lose the & 0x07 on the (func) ?

720

Can we make the macros arguments for both of these also pdev instead dev to avoid possible confusion?

1094

return (pbus->domain);

It is now initialised in lkpifill_pci_dev() so should be done for all paths, so we can simply return it.

[kib]

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

100

For ARI devices. slot is zero and function takes all lower byte. So these definitions are compatible both with usual (legacy bleh) addressing and with ARI.

[manu]

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

1094

True, will change.

[manu]

@kib @hselasky is the code ok for you now ?
I think I've address everything ARI related.

[• hselasky]

I'm good with the changes, but please wait for Konstantin to accept it too. He is the PCI expert.

[manu]

In D27550#624206, @hselasky wrote:

I'm good with the changes, but please wait for Konstantin to accept it too. He is the PCI expert.

I won't commit directly anyway, since the changes breaks drm-kmod I want to commit everything that I have in review for linuxkpi and update the port only one time.
Thanks

[kib]

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

1094

So this is still not done ?

1099

Style: 4 spaces indent for continuation line.

1102

I actually do not understand this function. What devfn argument is for ?

sys/compat/linuxkpi/common/src/linux_pci.c

219

I think you should use PCI_GET_ID() there instead of PCI_DEVFN.

236

Style: space around binary op (' | ').

271

I suggest to add assert that returned device indeed has slot/function number as requested. This should simplify further debugging for some cases.