>> +static int pcie_bus_update_set(struct pci_dev *dev, void *data)
>> +{
>> + int mps, p_mps;
>> +
>> + if (!pci_is_pcie(dev) || !dev->bus->self)
>> + return 0;
>> +
>> + mps = pcie_get_mps(dev);
>> + p_mps = pcie_get_mps(dev->bus->self);
>> +
>> + if (mps < p_mps)
>> + goto update;
Hi Jon,
Thanks for your review and comments!
>
> It would be more clear to have it return 0 if mps >= p_mps, and not
> have the goto statement.
OK, will update.
>
>> +
>> + return 0;
>> +
>> +update:
>> + /* If current mpss is lager than upstream, use upstream mps to update
>> + * current mps, otherwise print warning info.
>> + */
>> + if ((128 << dev->pcie_mpss) >= p_mps)
>> + pcie_write_mps(dev, p_mps);
>> + else
>> + dev_warn(&dev->dev, "MPS %d MPSS %d both smaller than upstream MPS %d\n"
>> + "If necessary, use \"pci=pcie_bus_peer2peer\" boot parameter to avoid this problem\n",
>> + mps, 128 << dev->pcie_mpss, p_mps);
>> + return 0;
>> +}
>> +
>> +static void pcie_bus_update_setting(struct pci_bus *bus)
>> +{
>> + if (bus->self->is_hotplug_bridge)
>> + pci_walk_bus(bus, pcie_bus_update_set, NULL);
>
> Why not just call pcie_bus_configure_set(bus->self, &smpss); with smpss of 0?
Because for safety, I won't touch the running pcie device(RP, UP/DP,EP) mps.
Consider the following case:
Root Port------>Upstream Port------>Downstream Port 1 ---->Endpoint device A (newly inserted device)
mps=256 256 | 256 128
|Downstream Port 2 ---->Endpoint device B
256 256
This patch try to update device A's mps equal to its parent DP 1 mps (256).
Because EP A device is not running, newly inserted, not configure yet. So configure its mps
to 256B is safe.
But if we try to configure DP 1 mps to 128B, it's not safe. its parent Upstream port still is 256.
>
>> +}
>> +
>> /* pcie_bus_configure_settings requires that pci_walk_bus work in a top-down,
>> * parents then children fashion. If this changes, then this code will not
>> * work as designed.
>> @@ -1614,6 +1648,15 @@ void pcie_bus_configure_settings(struct pci_bus *bus, u8 mpss)
>> if (!pci_is_pcie(bus->self))
>> return;
>>
>> + /* Sometimes we should update device mps here,
>> + * eg. after hot add, device mps value will be
>> + * set to default(128B), but the upstream port
>> + * mps value may be larger than 128B, if we do
>> + * not update the device mps, it maybe can not
>> + * work normally.
>
> This is slightly confusing to me. It would be more clear to say:
> There are situations (i.e., hot add) where the upstream port might
> have a larger MPS than the device. In these situations, the port MPS
> needs to be reconfigured to the lower value or the device will not
> operate properly.
Sorry for my poor English, I mean the device is the newly hot added device, not
the port device. So we will only reconfigure the newly hot added device mps.
>
>> + */
>> + pcie_bus_update_setting(bus);
>
> This only seems to be necessary in the "TUNE_OFF" case. It would be
> best to move it under that, just 2 lines down.
Good idea, will update, thanks!
Thanks!
Yijing.
>
>> +
>> if (pcie_bus_config == PCIE_BUS_TUNE_OFF)
>
> Perhaps it is time to make "SAFE" the default option, per the
> discussion at last years PCI mini-summit.
> Bjorn, thoughts?
>
>
> Thanks,
> Jon
>
>> return;
>>
>> --
>> 1.7.1
>>
>>
>
> .
>
--
Thanks!
Yijing
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html