lkml.org 
[lkml]   [2022]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RESEND v4 14/15] PCI: dwc: Check iATU in/outbound ranges setup methods status
    On Fri, Jun 24, 2022 at 05:39:46PM +0300, Serge Semin wrote:
    > Let's make the DWC PCIe RC/EP safer and more verbose for the invalid or
    > failed inbound and outbound iATU windows setups. Needless to say that
    > silently ignoring iATU regions setup errors may cause unpredictable
    > errors. For instance if for some reason a cfg or IO window fails to be
    > activated, then any CFG/IO requested won't reach target PCIe devices and
    > the corresponding accessors will return platform-specific random values.
    >
    > First of all we need to convert dw_pcie_ep_outbound_atu() method to check
    > whether the specified outbound iATU range is successfully setup. That
    > method is called by the pci_epc_ops.map_addr callback. Thus we'll make the
    > EP-specific CPU->PCIe memory mappings saver.
    >
    > Secondly since the iATU outbound range programming method now returns the
    > operation status, it will be handy to take that status into account in the
    > pci_ops.{map_bus,read,write} methods. Thus any failed mapping will be
    > immediately noticeable by the PCIe CFG operations requesters.
    >
    > Finally we need to convert the dw_pcie_setup_rc() method to returning the
    > operation status, since the iATU outbound ranges setup procedure may now
    > fail. It will be especially handy in case if the DW PCIe RC DT-node has
    > invalid/unsupported (dma-)ranges property. Note since the suggested
    > modification causes having too wide code indentation, it is reasonable
    > from maintainability and readability points of view to move the outbound
    > ranges setup procedure in the separate function.
    >
    > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

    Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

    Thanks,
    Mani

    > Reviewed-by: Rob Herring <robh@kernel.org>
    > ---
    > .../pci/controller/dwc/pcie-designware-ep.c | 9 +-
    > .../pci/controller/dwc/pcie-designware-host.c | 153 ++++++++++++------
    > drivers/pci/controller/dwc/pcie-designware.h | 5 +-
    > drivers/pci/controller/dwc/pcie-intel-gw.c | 6 +-
    > 4 files changed, 114 insertions(+), 59 deletions(-)
    >
    > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
    > index 2e91222f7c98..627c4b69878c 100644
    > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
    > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
    > @@ -184,8 +184,9 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
    > phys_addr_t phys_addr,
    > u64 pci_addr, size_t size)
    > {
    > - u32 free_win;
    > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
    > + u32 free_win;
    > + int ret;
    >
    > free_win = find_first_zero_bit(ep->ob_window_map, pci->num_ob_windows);
    > if (free_win >= pci->num_ob_windows) {
    > @@ -193,8 +194,10 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, u8 func_no,
    > return -EINVAL;
    > }
    >
    > - dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
    > - phys_addr, pci_addr, size);
    > + ret = dw_pcie_prog_ep_outbound_atu(pci, func_no, free_win, PCIE_ATU_TYPE_MEM,
    > + phys_addr, pci_addr, size);
    > + if (ret)
    > + return ret;
    >
    > set_bit(free_win, ep->ob_window_map);
    > ep->outbound_addr[free_win] = phys_addr;
    > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
    > index e0a2819608c6..6993ce9e856d 100644
    > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
    > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
    > @@ -412,7 +412,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
    >
    > dw_pcie_iatu_detect(pci);
    >
    > - dw_pcie_setup_rc(pp);
    > + ret = dw_pcie_setup_rc(pp);
    > + if (ret)
    > + goto err_free_msi;
    >
    > if (!dw_pcie_link_up(pci)) {
    > ret = dw_pcie_start_link(pci);
    > @@ -466,10 +468,10 @@ EXPORT_SYMBOL_GPL(dw_pcie_host_deinit);
    > static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
    > unsigned int devfn, int where)
    > {
    > - int type;
    > - u32 busdev;
    > struct dw_pcie_rp *pp = bus->sysdata;
    > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
    > + int type, ret;
    > + u32 busdev;
    >
    > /*
    > * Checking whether the link is up here is a last line of defense
    > @@ -490,8 +492,10 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
    > else
    > type = PCIE_ATU_TYPE_CFG1;
    >
    > -
    > - dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev, pp->cfg0_size);
    > + ret = dw_pcie_prog_outbound_atu(pci, 0, type, pp->cfg0_base, busdev,
    > + pp->cfg0_size);
    > + if (ret)
    > + return NULL;
    >
    > return pp->va_cfg0_base + where;
    > }
    > @@ -499,33 +503,45 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
    > static int dw_pcie_rd_other_conf(struct pci_bus *bus, unsigned int devfn,
    > int where, int size, u32 *val)
    > {
    > - int ret;
    > struct dw_pcie_rp *pp = bus->sysdata;
    > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
    > + int ret;
    >
    > ret = pci_generic_config_read(bus, devfn, where, size, val);
    > + if (ret != PCIBIOS_SUCCESSFUL)
    > + return ret;
    >
    > - if (!ret && pp->cfg0_io_shared)
    > - dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO, pp->io_base,
    > - pp->io_bus_addr, pp->io_size);
    > + if (pp->cfg0_io_shared) {
    > + ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
    > + pp->io_base, pp->io_bus_addr,
    > + pp->io_size);
    > + if (ret)
    > + return PCIBIOS_SET_FAILED;
    > + }
    >
    > - return ret;
    > + return PCIBIOS_SUCCESSFUL;
    > }
    >
    > static int dw_pcie_wr_other_conf(struct pci_bus *bus, unsigned int devfn,
    > int where, int size, u32 val)
    > {
    > - int ret;
    > struct dw_pcie_rp *pp = bus->sysdata;
    > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
    > + int ret;
    >
    > ret = pci_generic_config_write(bus, devfn, where, size, val);
    > + if (ret != PCIBIOS_SUCCESSFUL)
    > + return ret;
    >
    > - if (!ret && pp->cfg0_io_shared)
    > - dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO, pp->io_base,
    > - pp->io_bus_addr, pp->io_size);
    > + if (pp->cfg0_io_shared) {
    > + ret = dw_pcie_prog_outbound_atu(pci, 0, PCIE_ATU_TYPE_IO,
    > + pp->io_base, pp->io_bus_addr,
    > + pp->io_size);
    > + if (ret)
    > + return PCIBIOS_SET_FAILED;
    > + }
    >
    > - return ret;
    > + return PCIBIOS_SUCCESSFUL;
    > }
    >
    > static struct pci_ops dw_child_pcie_ops = {
    > @@ -552,10 +568,72 @@ static struct pci_ops dw_pcie_ops = {
    > .write = pci_generic_config_write,
    > };
    >
    > -void dw_pcie_setup_rc(struct dw_pcie_rp *pp)
    > +static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
    > {
    > - u32 val, ctrl, num_ctrls;
    > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
    > + struct resource_entry *entry;
    > + int i, ret;
    > +
    > + /* Note the very first outbound ATU is used for CFG IOs */
    > + if (!pci->num_ob_windows) {
    > + dev_err(pci->dev, "No outbound iATU found\n");
    > + return -EINVAL;
    > + }
    > +
    > + /*
    > + * Ensure all outbound windows are disabled before proceeding with
    > + * the MEM/IO ranges setups.
    > + */
    > + for (i = 0; i < pci->num_ob_windows; i++)
    > + dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_OB, i);
    > +
    > + i = 0;
    > + resource_list_for_each_entry(entry, &pp->bridge->windows) {
    > + if (resource_type(entry->res) != IORESOURCE_MEM)
    > + continue;
    > +
    > + if (pci->num_ob_windows <= ++i)
    > + break;
    > +
    > + ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_MEM,
    > + entry->res->start,
    > + entry->res->start - entry->offset,
    > + resource_size(entry->res));
    > + if (ret) {
    > + dev_err(pci->dev, "Failed to set MEM range %pr\n",
    > + entry->res);
    > + return ret;
    > + }
    > + }
    > +
    > + if (pp->io_size) {
    > + if (pci->num_ob_windows > ++i) {
    > + ret = dw_pcie_prog_outbound_atu(pci, i, PCIE_ATU_TYPE_IO,
    > + pp->io_base,
    > + pp->io_bus_addr,
    > + pp->io_size);
    > + if (ret) {
    > + dev_err(pci->dev, "Failed to set IO range %pr\n",
    > + entry->res);
    > + return ret;
    > + }
    > + } else {
    > + pp->cfg0_io_shared = true;
    > + }
    > + }
    > +
    > + if (pci->num_ob_windows <= i)
    > + dev_warn(pci->dev, "Resources exceed number of ATU entries (%d)\n",
    > + pci->num_ob_windows);
    > +
    > + return 0;
    > +}
    > +
    > +int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
    > +{
    > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
    > + u32 val, ctrl, num_ctrls;
    > + int ret;
    >
    > /*
    > * Enable DBI read-only registers for writing/updating configuration.
    > @@ -610,42 +688,9 @@ void dw_pcie_setup_rc(struct dw_pcie_rp *pp)
    > * ATU, so we should not program the ATU here.
    > */
    > if (pp->bridge->child_ops == &dw_child_pcie_ops) {
    > - int i, atu_idx = 0;
    > - struct resource_entry *entry;
    > -
    > - /*
    > - * Ensure all outbound windows are disabled so there are
    > - * multiple matches
    > - */
    > - for (i = 0; i < pci->num_ob_windows; i++)
    > - dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_OB, i);
    > -
    > - /* Get last memory resource entry */
    > - resource_list_for_each_entry(entry, &pp->bridge->windows) {
    > - if (resource_type(entry->res) != IORESOURCE_MEM)
    > - continue;
    > -
    > - if (pci->num_ob_windows <= ++atu_idx)
    > - break;
    > -
    > - dw_pcie_prog_outbound_atu(pci, atu_idx,
    > - PCIE_ATU_TYPE_MEM, entry->res->start,
    > - entry->res->start - entry->offset,
    > - resource_size(entry->res));
    > - }
    > -
    > - if (pp->io_size) {
    > - if (pci->num_ob_windows > ++atu_idx)
    > - dw_pcie_prog_outbound_atu(pci, atu_idx,
    > - PCIE_ATU_TYPE_IO, pp->io_base,
    > - pp->io_bus_addr, pp->io_size);
    > - else
    > - pp->cfg0_io_shared = true;
    > - }
    > -
    > - if (pci->num_ob_windows <= atu_idx)
    > - dev_warn(dev, "Resources exceed number of ATU entries (%d)\n",
    > - pci->num_ob_windows);
    > + ret = dw_pcie_iatu_setup(pp);
    > + if (ret)
    > + return ret;
    > }
    >
    > dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0);
    > @@ -658,5 +703,7 @@ void dw_pcie_setup_rc(struct dw_pcie_rp *pp)
    > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
    >
    > dw_pcie_dbi_ro_wr_dis(pci);
    > +
    > + return 0;
    > }
    > EXPORT_SYMBOL_GPL(dw_pcie_setup_rc);
    > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
    > index 60f1ddc54933..c3e73ed9aff5 100644
    > --- a/drivers/pci/controller/dwc/pcie-designware.h
    > +++ b/drivers/pci/controller/dwc/pcie-designware.h
    > @@ -387,7 +387,7 @@ static inline void dw_pcie_stop_link(struct dw_pcie *pci)
    >
    > #ifdef CONFIG_PCIE_DW_HOST
    > irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp);
    > -void dw_pcie_setup_rc(struct dw_pcie_rp *pp);
    > +int dw_pcie_setup_rc(struct dw_pcie_rp *pp);
    > int dw_pcie_host_init(struct dw_pcie_rp *pp);
    > void dw_pcie_host_deinit(struct dw_pcie_rp *pp);
    > int dw_pcie_allocate_domains(struct dw_pcie_rp *pp);
    > @@ -399,8 +399,9 @@ static inline irqreturn_t dw_handle_msi_irq(struct dw_pcie_rp *pp)
    > return IRQ_NONE;
    > }
    >
    > -static inline void dw_pcie_setup_rc(struct dw_pcie_rp *pp)
    > +static inline int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
    > {
    > + return 0;
    > }
    >
    > static inline int dw_pcie_host_init(struct dw_pcie_rp *pp)
    > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c
    > index a44f685ec94d..c3481200e86a 100644
    > --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
    > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
    > @@ -302,7 +302,11 @@ static int intel_pcie_host_setup(struct intel_pcie *pcie)
    > intel_pcie_ltssm_disable(pcie);
    > intel_pcie_link_setup(pcie);
    > intel_pcie_init_n_fts(pci);
    > - dw_pcie_setup_rc(&pci->pp);
    > +
    > + ret = dw_pcie_setup_rc(&pci->pp);
    > + if (ret)
    > + goto app_init_err;
    > +
    > dw_pcie_upconfig_setup(pci);
    >
    > intel_pcie_device_rst_deassert(pcie);
    > --
    > 2.35.1
    >

    --
    மணிவண்ணன் சதாசிவம்

    \
     
     \ /
      Last update: 2022-08-01 15:55    [W:2.308 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site