| Message ID | 1376470912-40264-3-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Aug 14, 2013 at 2:01 AM, Yijing Wang <wangyijing@huawei.com> wrote: > Currently we don't update device's mps vaule when doing Spelling nit, "value" > pci device hot-add. The hot-added device's mps will be set > to default value (128B). But the upstream port device's mps > may be larger than 128B which was set by firmware during > system bootup. In this case the new added device may not > work normally. This patch try to update the hot added device > mps euqal to its parent mps, if device mpss < parent mps, Spelling nit, "equal". > print warning. > > References: https://bugzilla.kernel.org/show_bug.cgi?id=60671 > Reported-by: Yijing Wang <wangyijing@huawei.com> > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > Cc: Jon Mason <jdmason@kudzu.us> > Cc: stable@vger.kernel.org # 3.4+ > --- > drivers/pci/probe.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 49 insertions(+), 1 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 0699ec0..0d47b4a 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1621,6 +1621,45 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data) > return 0; > } > > +static int pcie_bus_update_set(struct pci_dev *dev, void *data) > +{ > + int mps, p_mps, mpss; > + struct pci_dev *parent; > + > + if (!pci_is_pcie(dev) || !dev->bus->self) > + return 0; > + > + parent = dev->bus->self; > + mps = pcie_get_mps(dev); > + p_mps = pcie_get_mps(dev->bus->self); > + > + if (mps >= p_mps) I'm probably overly paranoid, but I worry what would happen in the > case. Should we try and force the device MPS to a sane value or are we confident that this will never happen? > + return 0; > + > + /* we only update the device mps, unless its parent device is root port, > + * and it is the only slot directly connected to root port. > + */ > + if ((128 << dev->pcie_mpss) >= p_mps) { > + pcie_write_mps(dev, p_mps); > + } else if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT && > + pci_only_one_slot(dev->bus)) { > + mpss = 128 << dev->pcie_mpss; OCD nit, you could move the above line up 4 lines and reuse the variable in the first if-statement. > + pcie_write_mps(parent, mpss); > + pcie_write_mps(dev, mpss); > + } 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); > + } Nit, braces are unnecessary in the else. I think scripts/checkpatch.pl would've complained if you ran it on the patch. If you/Bjorn want to disregard my paranoia and nits, it looks fine to me. > + 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); > +} > + > /* 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. > @@ -1632,8 +1671,17 @@ void pcie_bus_configure_settings(struct pci_bus *bus, u8 mpss) > if (!pci_is_pcie(bus->self)) > return; > > - if (pcie_bus_config == PCIE_BUS_TUNE_OFF) > + if (pcie_bus_config == PCIE_BUS_TUNE_OFF) { > + /* 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. > + */ > + pcie_bus_update_setting(bus); > return; > + } > > /* FIXME - Peer to peer DMA is possible, though the endpoint would need > * to be aware to the MPS of the destination. To work around this, > -- > 1.7.1 > > -- 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
Hi Jon, Thanks for your review and comments! On 2013/8/15 0:59, Jon Mason wrote: > On Wed, Aug 14, 2013 at 2:01 AM, Yijing Wang <wangyijing@huawei.com> wrote: >> Currently we don't update device's mps vaule when doing > > Spelling nit, "value" Will update, thanks! > >> pci device hot-add. The hot-added device's mps will be set >> to default value (128B). But the upstream port device's mps >> may be larger than 128B which was set by firmware during >> system bootup. In this case the new added device may not >> work normally. This patch try to update the hot added device >> mps euqal to its parent mps, if device mpss < parent mps, > > Spelling nit, "equal". Will update. > >> print warning. >> >> References: https://bugzilla.kernel.org/show_bug.cgi?id=60671 >> Reported-by: Yijing Wang <wangyijing@huawei.com> >> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >> Cc: Jon Mason <jdmason@kudzu.us> >> Cc: stable@vger.kernel.org # 3.4+ >> --- >> drivers/pci/probe.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 files changed, 49 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 0699ec0..0d47b4a 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1621,6 +1621,45 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data) >> return 0; >> } >> >> +static int pcie_bus_update_set(struct pci_dev *dev, void *data) >> +{ >> + int mps, p_mps, mpss; >> + struct pci_dev *parent; >> + >> + if (!pci_is_pcie(dev) || !dev->bus->self) >> + return 0; >> + >> + parent = dev->bus->self; >> + mps = pcie_get_mps(dev); >> + p_mps = pcie_get_mps(dev->bus->self); >> + >> + if (mps >= p_mps) > > I'm probably overly paranoid, but I worry what would happen in the > > case. Should we try and force the device MPS to a sane value or are > we confident that this will never happen? In the original patch, this patch will update the mps if mps != p_mps. For conservative, this patch only to fix the issue occurs in hot-plug(mps will never larger than p_mps). I don't know whether BIOS will set mps like mps > p_mps for certain purposes.:) > >> + return 0; >> + >> + /* we only update the device mps, unless its parent device is root port, >> + * and it is the only slot directly connected to root port. >> + */ >> + if ((128 << dev->pcie_mpss) >= p_mps) { >> + pcie_write_mps(dev, p_mps); >> + } else if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT && >> + pci_only_one_slot(dev->bus)) { >> + mpss = 128 << dev->pcie_mpss; > > OCD nit, you could move the above line up 4 lines and reuse the > variable in the first if-statement. Good, thanks! > >> + pcie_write_mps(parent, mpss); >> + pcie_write_mps(dev, mpss); >> + } 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); >> + } > > Nit, braces are unnecessary in the else. I think > scripts/checkpatch.pl would've complained if you ran it on the patch. I used scripts/checkpatch.pl to check patch, result is ok. So if I use scripts/checkpatch.pl to check patch without the pair braces, and result is ok, I will remove it. Thanks! > > If you/Bjorn want to disregard my paranoia and nits, it looks fine to me. > >> + 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); >> +} >> + >> /* 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. >> @@ -1632,8 +1671,17 @@ void pcie_bus_configure_settings(struct pci_bus *bus, u8 mpss) >> if (!pci_is_pcie(bus->self)) >> return; >> >> - if (pcie_bus_config == PCIE_BUS_TUNE_OFF) >> + if (pcie_bus_config == PCIE_BUS_TUNE_OFF) { >> + /* 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. >> + */ >> + pcie_bus_update_setting(bus); >> return; >> + } >> >> /* FIXME - Peer to peer DMA is possible, though the endpoint would need >> * to be aware to the MPS of the destination. To work around this, >> -- >> 1.7.1 >> >> > -- > To unsubscribe from this list: send the line "unsubscribe stable" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > . >
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 0699ec0..0d47b4a 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1621,6 +1621,45 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data) return 0; } +static int pcie_bus_update_set(struct pci_dev *dev, void *data) +{ + int mps, p_mps, mpss; + struct pci_dev *parent; + + if (!pci_is_pcie(dev) || !dev->bus->self) + return 0; + + parent = dev->bus->self; + mps = pcie_get_mps(dev); + p_mps = pcie_get_mps(dev->bus->self); + + if (mps >= p_mps) + return 0; + + /* we only update the device mps, unless its parent device is root port, + * and it is the only slot directly connected to root port. + */ + if ((128 << dev->pcie_mpss) >= p_mps) { + pcie_write_mps(dev, p_mps); + } else if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT && + pci_only_one_slot(dev->bus)) { + mpss = 128 << dev->pcie_mpss; + pcie_write_mps(parent, mpss); + pcie_write_mps(dev, mpss); + } 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); +} + /* 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. @@ -1632,8 +1671,17 @@ void pcie_bus_configure_settings(struct pci_bus *bus, u8 mpss) if (!pci_is_pcie(bus->self)) return; - if (pcie_bus_config == PCIE_BUS_TUNE_OFF) + if (pcie_bus_config == PCIE_BUS_TUNE_OFF) { + /* 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. + */ + pcie_bus_update_setting(bus); return; + } /* FIXME - Peer to peer DMA is possible, though the endpoint would need * to be aware to the MPS of the destination. To work around this,
Currently we don't update device's mps vaule when doing pci device hot-add. The hot-added device's mps will be set to default value (128B). But the upstream port device's mps may be larger than 128B which was set by firmware during system bootup. In this case the new added device may not work normally. This patch try to update the hot added device mps euqal to its parent mps, if device mpss < parent mps, print warning. References: https://bugzilla.kernel.org/show_bug.cgi?id=60671 Reported-by: Yijing Wang <wangyijing@huawei.com> Signed-off-by: Yijing Wang <wangyijing@huawei.com> Cc: Jon Mason <jdmason@kudzu.us> Cc: stable@vger.kernel.org # 3.4+ --- drivers/pci/probe.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 49 insertions(+), 1 deletions(-)