On Wed, May 25, 2011 at 1:01 PM, Jon Mason <jon.mason@xxxxxxxx> wrote:
> Ensure that the max payload size on the root port is the same as the max
> payload size on all intermediate bridges and devices. This is required
> to work around buggy BIOS revisions found on various whitebox
> motherboards which do not configure mps beyond one level below the root
> port.
>
> Signed-off-by: Jon Mason <mason@xxxxxxxx>
> Signed-off-by: Andrew Gallatin <gallatin@xxxxxxxx>
> ---
> drivers/pci/bus.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 44 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 69546e9..fc16b67 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -175,6 +175,49 @@ int pci_bus_add_child(struct pci_bus *bus)
> return retval;
> }
>
> +/*
> + * Ensure that the max payload size on the root port is the same as the max
> + * payload size on all intermediate bridges.
This description doesn't seem to quite match what the function does.
It looks like the function sets a single PCIe device's MPS to that of
the upstream bridge.
> This is required to work around
> + * buggy BIOS revisions found on various whitebox motherboards which do not
> + * configure mps beyond one level below the root port.
> + */
> +static void pci_mps_workaround(struct pci_dev *child, struct pci_dev *parent)
Why bother passing in the parent, when we could as easily look up
dev->bus->self inside this function? Passing in the parent opens the
possibility of errors in the caller that do non-sensical things, e.g.,
passing in a child and parent that are unrelated to each other.
> +{
> + u16 val, pmps, cmps;
> + int cap, rc;
> +
> + if (!parent)
> + return;
> +
> + cap = pci_find_capability(parent, PCI_CAP_ID_EXP);
> + if (!cap)
> + return;
> +
> + rc = pci_read_config_word(parent, cap + PCI_EXP_DEVCTL, &val);
> + if (rc)
> + return;
> +
> + pmps = val & PCI_EXP_DEVCTL_PAYLOAD;
> +
> + cap = pci_find_capability(child, PCI_CAP_ID_EXP);
> + if (!cap)
> + return;
> +
> + rc = pci_read_config_word(child, cap + PCI_EXP_DEVCTL, &val);
> + if (rc)
> + return;
> +
> + cmps = val & PCI_EXP_DEVCTL_PAYLOAD;
> +
> + if (pmps != cmps) {
> + printk(KERN_WARNING "Child MPS of %d != Parent MPS of %d! "
> + "Most likely caused by bad BIOS. Working around...\n",
> + 128 << (cmps >> 5), 128 << (pmps >> 5));
Please use dev_warn() here so we know which device is involved. Also,
please make the message explicit about how we're working around it,
e.g., "setting child MPS to %d".
> + val = (val & ~ PCI_EXP_DEVCTL_PAYLOAD) | pmps;
> + pci_write_config_word(child, cap + PCI_EXP_DEVCTL, val);
> + }
> +}
> +
> /**
> * pci_bus_add_devices - insert newly discovered PCI devices
> * @bus: bus to check for new devices
> @@ -194,6 +237,7 @@ void pci_bus_add_devices(const struct pci_bus *bus)
> int retval;
>
> list_for_each_entry(dev, &bus->devices, bus_list) {
> + pci_mps_workaround(dev, dev->bus->self);
> /* Skip already-added devices */
> if (dev->is_added)
> continue;
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
