On Wed, May 25, 2011 at 2:34 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> 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.
Does this sound better?
/*
* 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. Ensure that the max payload size of all children bridges and
* devices are the same as the parent bus.
*/
>> 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.
Very true. Will be corrected in the next version.
>
>> +{
>> + 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".
Agreed. Fixed.
>
>> + 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
