kernel: match pci devs or drvs against DT nodes

[InsaneKnight]

A TP-LINK archer C7 v2 device running OpenWRT v22.03+ will drop into reboot loop after its internal but socketed ath10k mini PCIe wlan card gets replaced with an ath9k one (namely, an ar9580 working well on my laptop). Further observation in OpenWRT's failsafe mode shows that the router reboots right after the ath9k driver is manually loaded, but after a firmware image built with the only change being the node corresponding to the wlan card (the node wifi@0,0) removed from the device tree is flashed, everything works just fine.

The node wifi@0,0 should only compatible with "qcom,ath10k", but the reboot seems to be caused by the "calibration" data (targeting ath10k devices) declared in this node being loaded by the ath9k driver for the ath9k card installed on its place, regardless the compatibility string.

In order to resolve such issue, a PCI driver should check whether its
positionally corresponding device tree nodes is compatible before loading
any data from the node.

This commit contains 3 kernel patches:
931 adds routines to match pci devices or drivers against device tree nodes,
602 and 991 add checks to ath9k and ath10k respectively.

Signed-off-by: Edward Chow equu@openmail.cc

[hauke]

I do not think we will be able to get this integrated into the upstream Linux kernel.
ath10k and ath9k should run into a kernel panic when you provide the wrong calibration data. The drivers should validate the data and then just fail to load. This looks like a bug in the driver.

When you add a compatibility string it should only load for one card when when it is possible to add multiple PCIe cards into the slot: See: compatible = "pci168c,0033";

[InsaneKnight]

I do not think we will be able to get this integrated into the upstream Linux kernel. ath10k and ath9k should run into a kernel panic when you provide the wrong calibration data. The drivers should validate the data and then just fail to load. This looks like a bug in the driver. When you add a compatibility string it should only load for one card when when it is possible to add multiple PCIe cards into the slot: See: |compatible = "pci168c,0033";|

Yes. The main issue here may be that ath9k blindly loads `nvmem-cell-names = "calibration";` from the node ***@***.*** without noticing `compatible = "qcom,ath10k";`. Which functions are responsible for checking compatibility strings?

[InsaneKnight]

When you add a compatibility string it should only load for one card when when it is possible to add multiple PCIe cards into the slot: See: |compatible = "pci168c,0033";|

Besides, there seems no code to match either `|compatible = "pcivvvv,dddd";` or `compatible = "qcom,ath10k";` in Linux kernel. |

[mpratt14]

something more simple is wrong with the DTS

nvmem-cell-names gives a categorical label to the cell, it's not meant to be a unique identifier like what's called a "label" in DTS

all device nodes are instantiated as kernel objects at some point, and they are all unique pointers regardless of what names are used, and standard DTS labels are unique in order to point to a specific node without using properties.

nodes dont even need a name, but they need a "compatible" and a name for each cell it has a pointer to so the driver knows which is which.

I recommend you just post your DTS somewhere like on the forum and ask for help

I just made the switch on a device I have and I had no issues with the following format

&art {
	compatible = "nvmem-cells";

	macaddr_art_0: macaddr@0 {
		reg = <0x0 0x6>;
	};

	calibration_ath9k: calibration@1000 {
		reg = <0x1000 0x440>;
	};

	calibration_ath10k: calibration@5000 {
		reg = <0x5000 0x844>;
	};
};

[InsaneKnight]

@mpratt14 You seem not get my point. tp-link archer c7 v2 has the following DT definition:

&pcie1 {         status = "okay";         ***@***.***,0 {                 compatible = "qcom,ath10k";                 reg = <0 0 0 0 0>;                 mac-address-increment = <(-1)>;                 nvmem-cells = <&macaddr_uboot_1fc00>, <&calibration_art_5000>;                 nvmem-cell-names = "mac-address", "calibration";         }; };

but what node "wifi" represents is actually a PCIe expansion card. After it is replaced with a stand-along ath9k card (e.g. ar9580) with calibration data engraved in, ath9k will try to load the content of nvmem-cell named "calibration" (<&calibration_art_5000>) as the calibration data for the card, regardless that the compatibility string `compatible = "qcom,ath10k";` says it is not for ath9k, and kernel panics for loading calibration data for ath10k to an ath9k device.

[InsaneKnight]

Besides, the old approach loading cal data from file system does not have this issue, because ath9k driver will not load a blob named for ath10k.

[john-tho]

After it is replaced with a stand-along ath9k card (e.g. ar9580)

When you replace the pcie card, update the DTS to reflect the new card? Otherwise, your DTS is (now) wrong for your hardware.

[InsaneKnight]

After it is replaced with a stand-along ath9k card (e.g. ar9580) When you replace the pcie card, update the DTS to reflect the new card? Otherwise, your DTS is (now) wrong for your hardware.

It is currently the only working approach (removing node ***@***.***,0 completely, as the card has calibration data engraved in), but DTS used not to need to be changed before commit 297bcee, as loading cal data from file system does not have the current issue, as noted in the previous reply.

[mpratt14]

ok I understand now

in my opinion the proper fix would be to edit ath9k and ath10k drivers to look for their own compatible property in the nodes and otherwise ignore the device

[InsaneKnight]

ok I understand now in my opinion the proper fix would be to edit ath9k and ath10k drivers to look for their own compatible property in the nodes and otherwise ignore the device Message ID: ***@***.***>

Agreed, but there seems no existing functions to match compatibility strings with format "pcivvvv,dddd" to real pci vid and pid, and "qcom,ath10k" seems not so meaningful as pci ids, although we can forcibly endow meaning to it. Besides, devices may change their pci ids after cal data gets applied (as many ath9k devices do), so the array of "struct pci_device_id" with driver might be consulted first.

[mpratt14]

it would not hurt to just add another generic compatible "atheros,ath9k" or similar. even if upstream wouldn't like that, its small enough to keep in the "hack" category without issues

[InsaneKnight]

it would not hurt to just add another generic compatible "atheros,ath9k" or similar. even if upstream wouldn't like that, its small enough to keep in the "hack" category without issues Message ID: ***@***.***>

Where to add? In DTS or in Linux kernel?

[InsaneKnight]

Personally I am okay to generate image via imagebuilder with tuned device tree for my archerc7v2 with replaced wlan card, as I have to use my wdr4900v2 in this way because I have equipped it with a larger rom. An not fully working solution to match a compatibility string against a pci device has been uploaded to this PR. If someone is interested in it, we could refine it together, possibly in a dedicated PR.

[InsaneKnight]

I have pushed another revision, but it seems the back ported mac80211 drivers cannot properly use newly exported symbols from the Linux kernel proper.

[mpratt14]

this looks really good now, just need to solve the build problem and test

[hauke]

Refresh the patches to make the CI happy:

make target/linux/{clean,refresh} V=99 -j5

The kernel patch in target/linux/generic/hack-5.15/931-pci-add-functions-to-match-pci-dev-against-OF-DT-node.patch is also needed for kernel 5.10 because OpenWrt currently uses both.

Could you also send this to the upstream Linux kernel community to get this integrated into mainline Linux kernel. I think this looks like a useful feature also for other Linux distributions. If you do so please share the Link to the mailing list discussion or patchwork.

[InsaneKnight]

The kernel patch in `target/linux/generic/hack-5.15/931-pci-add-functions-to-match-pci-dev-against-OF-DT-node.patch` is also needed for kernel 5.10 because OpenWrt currently uses both.

Now the patch is present in `target/linux/generic/pendin-5.10`and `target/linux/generic/pendin-5.15`.

Could you also send this to the upstream Linux kernel community to get this integrated into mainline Linux kernel. I think this looks like a useful feature also for other Linux distributions. If you do so please share the Link to the mailing list discussion or patchwork.

The patch can now be seen on https://patchwork.kernel.org/project/linux-pci/patch/ea4e2fed-383d-829d-8a2a-9239768ccd94@openmail.cc/ and https://lore.kernel.org/linux-pci/ea4e2fed-383d-829d-8a2a-9239768ccd94@openmail.cc/T/

[InsaneKnight]

Refresh the patches to make the CI happy: ```` make target/linux/{clean,refresh} V=99 -j5 ```` The kernel patch in `target/linux/generic/hack-5.15/931-pci-add-functions-to-match-pci-dev-against-OF-DT-node.patch` is also needed for kernel 5.10 because OpenWrt currently uses both. Could you also send this to the upstream Linux kernel community to get this integrated into mainline Linux kernel. I think this looks like a useful feature also for other Linux distributions. If you do so please share the Link to the mailing list discussion or patchwork.

I am upstreaming these patches and currently get stuck. Do you guys have any ideas about how to improve them for upstreaming?