Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Krystian Hebel, Kyösti Mälkki, Michał Żygowski.
6 comments:
File src/device/pciexp_device.c:
Patch Set #11, 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):
1. Sets the bridge's MPS to its max cap in pciexp_scan_bus.
2. Sets device MPS to its max cap in pciexp_tune_dev.
3. 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.
4. 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:
Patch Set #15, 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().
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.
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.
Patch Set #15, 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))`.
Patch Set #15, 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.
To view, visit change 77338. To unsubscribe, or for help writing mail filters, visit settings.