Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Krystian Hebel, Kyösti Mälkki, Michał Żygowski.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77338?usp=email )
Change subject: device/pciexp_device.c: Fix setting Max Payload Size ......................................................................
Patch Set 15:
(6 comments)
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/4032840c_b5bef0c3 : PS11, Line 598: awlays Thanks for the update and your patience.
Just to confirm we are on the same page. Patchset 14 currently does the following (in chronological order of execution):
- Sets the bridge's MPS to its max cap in pciexp_scan_bus.
- Sets device MPS to its max cap in pciexp_tune_dev.
- In pciexp_tune_dev, traverses the topology up to eventually limit the currently configured MPS to the lower MPS of the device and its parent.
Explicitly walking up is probably not necessary, as pciexp_tune_dev() is already called per level up until the root port. I'll comment on that inline. Please take it with a grain of salt, I'll look into it again tomorrow after a night's sleep.
- At the end of pciexp_scan_bus the MPS of root port is programmed to all devices beneath it.
I have removed the state tracking as per your idea:
On the way down, set every device to its own maximum. On the way up, propagate the minimum from the endpoints up to the root port
This would work without additional state tracking.
Is there any way to make it simpler? According to your comment:
With the max_payload_set state, we can also do everything on the way up.
Then we would have to look at the already set value and the cap maximum (like the current implementation does).
it looks like there is such a way. Although, I can't see how "do everything the way up" can be done, so any further suggestions are appreciated.
I see a way, but it wouldn't be simpler, I guess. With `max_payload_set` we could dynamically decide if we have to read the cap of a bridge or the current value. But that's another `if` in the code that's probably better avoided.
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/f3866d3e_7b7a22dd : PS15, Line 638: pciexp_dev_set_max_payload_size(child, max_payload); Shouldn't be necessary as that's what we'll do with pciexp_sync_max_payload_size().
https://review.coreboot.org/c/coreboot/+/77338/comment/ca013069_2718eeec : PS15, Line 641: if (max_payload != child_max_payload) : printk(BIOS_INFO, "%s: Max_Payload_Size adjusted to %d\n", dev_path(child), : (1 << (max_payload + 7))); This too could be dropped then.
https://review.coreboot.org/c/coreboot/+/77338/comment/78200e1a_e3c3bbce : PS15, Line 691: }; I still don't see why we would need a loop. pciexp_tune_dev() will be called for each level until the root. So calling pciexp_set_max_payload_size() in pciexp_tune_dev() directly, should be enough. Unless I miss something?
Now that I looked into pciexp_tune_dev() again, I can't say often enough how confusing it is that the parent is named 'root' there. Hope that's not the source of any trouble. That function definitely runs for every PCIe device beside the root ports, and only on the highest level the parent (`root`) will be the root port.
https://review.coreboot.org/c/coreboot/+/77338/comment/d474a022_cf3addc0 : PS15, Line 728: pciexp_dev_set_max_payload_size(dev, pciexp_dev_get_max_payload_size_cap(dev)); If I'm not mistaken, `dev` could be a bridge that already has its MPS constrained by a downstream device. So this should only happen at the lowest level, i.e. would need something like an `if (is_endpoint(dev))`.
https://review.coreboot.org/c/coreboot/+/77338/comment/9310afe1_bc5f5ad7 : PS15, Line 740: pciexp_dev_set_max_payload_size(bus->dev, max_payload); This step seems redundant. Because for the first call of pciexp_sync_max_payload_size() `bus->dev` will be the one device where we looked up the `max_payload`. And for all recursive calls we'll have set it right before in line 747.