Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Kyösti Mälkki, Michał Żygowski, Nico Huber.
Krystian Hebel 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 20:
(6 comments)
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/73575b57_1a45e8ce : PS18, Line 596: unsigned int
Though, in general, limiting the type also constraints
the compiler in the choices of optimizations it can make.
Yes, but those optimizations are often the cause of "compiler does what I've told it to do instead of what I wanted" type of errors, especially if there were arithmetic operations on the right side of the assignment. That said, this isn't the case here, and as all other calls are using `unsigned int`, keeping this consistent with existing code seems more important, so I'm marking this and following as resolved.
https://review.coreboot.org/c/coreboot/+/77338/comment/1a128972_d5fa628a : PS18, Line 610: unsigned int
All `pci_find_capability(dev, PCI_CAP_ID_PCIE);` calls were assigning a value to unsigned int (with […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/77338/comment/b2b64d1c_44a5d75d : PS18, Line 623: unsigned int
All `pci_find_capability(dev, PCI_CAP_ID_PCIE);` calls were assigning a value to unsigned int (with […]
Acknowledged
https://review.coreboot.org/c/coreboot/+/77338/comment/a9babf67_72424b94 : PS18, Line 653: pciexp_dev_set_max_payload_size(parent, max_payload);
Done
Done
https://review.coreboot.org/c/coreboot/+/77338/comment/1704640d_11bc7f4a : PS18, Line 721: parent's
From the other comment: […]
I see, I missed that `pci_scan_bus()` in line 758 (so before the loop) calls `pciexp_scan_bus()` for lower levels _before_ it is propagated up by `pciexp_tune_dev()`. Now it makes sense.
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/77338/comment/8400eb05_a602a94d : PS20, Line 603: devctl |= max_payload << 5; Please either mask it with `PCI_EXP_DEVCTL_PAYLOAD` or add a comment that this will never overflow to higher bits of `PCI_EXP_DEVCTL` register due to how callers of this function obtain `max_payload`.