Re: [PATCH V1] PCI: designware-ep: Fix DBI access before core init

Vidya Sagar <vidyas@xxxxxxxxxx> · Thu, 7 Jul 2022 22:01:30 +0530

On 7/7/2022 9:30 PM, Bjorn Helgaas wrote:
External email: Use caution opening links or attachments


[+cc Serge]

On Thu, Jul 07, 2022 at 02:39:08PM +0530, Vidya Sagar wrote:
Hi,
Anyone has review comments for this change?
Without this change, Tegra194's endpoint mode is effectively broken.

Did Tegra194 endpoint mode ever work, or has this always been broken.
Wondering if there's a Fixes: or stable tag we should add.  Also cc'd
Serge since he's been doing similar work on dwc.

Yes. The endpoint mode of Tegra194 did work particularly after the 
commit ac37dde72177 ("PCI: dwc: Add API to notify core initialization 
completion")
There are many changes that got merged after this and hence couldn't 
really point to one change to quote in Fixes: tag.

Thanks & Regards,
Vidya Sagar


On 6/22/2022 9:31 AM, Vidya Sagar wrote:
Platforms that cannot support their core initialization without the
reference clock from the host, implement the feature 'core_init_notifier'
to indicate the DesignWare sub-system about when their core is getting
initialized. Any accesses to the core (Ex:- DBI) would result in system
hang in such systems (Ex:- tegra194). This patch moves any access to the
core to dw_pcie_ep_init_complete() API which is effectively called only
after the core initialization.

Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
---
   .../pci/controller/dwc/pcie-designware-ep.c   | 88 +++++++++++--------
   1 file changed, 49 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 0eda8236c125..9feec720175f 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -639,9 +639,14 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
   int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
   {
     struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+   struct dw_pcie_ep_func *ep_func;
+   struct device *dev = pci->dev;
+   struct pci_epc *epc = ep->epc;
     unsigned int offset;
     unsigned int nbars;
     u8 hdr_type;
+   u8 func_no;
+   void *addr;
     u32 reg;
     int i;
@@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
             return -EIO;
     }
+   dw_pcie_iatu_detect(pci);
+
+   ep->ib_window_map = devm_kcalloc(dev,
+                                    BITS_TO_LONGS(pci->num_ib_windows),
+                                    sizeof(long),
+                                    GFP_KERNEL);
+   if (!ep->ib_window_map)
+           return -ENOMEM;
+
+   ep->ob_window_map = devm_kcalloc(dev,
+                                    BITS_TO_LONGS(pci->num_ob_windows),
+                                    sizeof(long),
+                                    GFP_KERNEL);
+   if (!ep->ob_window_map)
+           return -ENOMEM;
+
+   addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t),
+                       GFP_KERNEL);
+   if (!addr)
+           return -ENOMEM;
+   ep->outbound_addr = addr;
+
+   for (func_no = 0; func_no < epc->max_functions; func_no++) {
+           ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL);
+           if (!ep_func)
+                   return -ENOMEM;
+
+           ep_func->func_no = func_no;
+           ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no,
+                                                         PCI_CAP_ID_MSI);
+           ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no,
+                                                          PCI_CAP_ID_MSIX);
+
+           list_add_tail(&ep_func->list, &ep->func_list);
+   }
+
     offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
     dw_pcie_dbi_ro_wr_en(pci);
@@ -677,8 +718,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
   int dw_pcie_ep_init(struct dw_pcie_ep *ep)
   {
     int ret;
-   void *addr;
-   u8 func_no;
     struct resource *res;
     struct pci_epc *epc;
     struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
@@ -686,7 +725,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
     struct platform_device *pdev = to_platform_device(dev);
     struct device_node *np = dev->of_node;
     const struct pci_epc_features *epc_features;
-   struct dw_pcie_ep_func *ep_func;
     INIT_LIST_HEAD(&ep->func_list);
@@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
             }
     }
-   dw_pcie_iatu_detect(pci);
-
     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
     if (!res)
             return -EINVAL;
@@ -717,26 +753,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
     ep->phys_base = res->start;
     ep->addr_size = resource_size(res);
-   ep->ib_window_map = devm_kcalloc(dev,
-                                    BITS_TO_LONGS(pci->num_ib_windows),
-                                    sizeof(long),
-                                    GFP_KERNEL);
-   if (!ep->ib_window_map)
-           return -ENOMEM;
-
-   ep->ob_window_map = devm_kcalloc(dev,
-                                    BITS_TO_LONGS(pci->num_ob_windows),
-                                    sizeof(long),
-                                    GFP_KERNEL);
-   if (!ep->ob_window_map)
-           return -ENOMEM;
-
-   addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t),
-                       GFP_KERNEL);
-   if (!addr)
-           return -ENOMEM;
-   ep->outbound_addr = addr;
-
     if (pci->link_gen < 1)
             pci->link_gen = of_pci_get_max_link_speed(np);
@@ -753,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
     if (ret < 0)
             epc->max_functions = 1;
-   for (func_no = 0; func_no < epc->max_functions; func_no++) {
-           ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL);
-           if (!ep_func)
-                   return -ENOMEM;
-
-           ep_func->func_no = func_no;
-           ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no,
-                                                         PCI_CAP_ID_MSI);
-           ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no,
-                                                          PCI_CAP_ID_MSIX);
-
-           list_add_tail(&ep_func->list, &ep->func_list);
-   }
-
     if (ep->ops->ep_init)
             ep->ops->ep_init(ep);
@@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
                     return 0;
     }
+   /*
+    * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this
+    * step as platforms that implement 'core_init_notifier' feature may
+    * not have the hardware ready (i.e. core initialized) for access
+    * (Ex: tegra194). Any hardware access on such platforms result
+    * in system hard hang.
+    */
+
     return dw_pcie_ep_init_complete(ep);
   }
   EXPORT_SYMBOL_GPL(dw_pcie_ep_init);







