Re: [PATCH -v3] PCI: update device mps when doing pci hotplug

[Jon Mason <jdmason@kudzu.us>]

On Tue, Aug 6, 2013 at 6:52 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>> +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.

I understand your logic now and agree.

> But if we try to configure DP 1 mps to 128B, it's not safe. its parent Upstream port still is 256.

This is exactly the case that the "PERFORMANCE" option is trying to
allow.  If the MRRS is set to the MPS of the device, it should work.
If not, then we should rip out all of the "PERFORMANCE" code.  Is this
something you can verify?

Thanks,
Jon

>>
>>> +}
>>> +
>>>  /* 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 linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html