| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 15 Oct 2022 15:00:11 -0700
From: Han Jingoo <jingoohan1@...il.com>
To: Vidya Sagar <vidyas@...dia.com>
Cc: gustavo.pimentel@...opsys.com, lpieralisi@...nel.org,
robh@...nel.org, kw@...ux.com, bhelgaas@...gle.com,
Sergey.Semin@...kalelectronics.ru, dmitry.baryshkov@...aro.org,
linmq006@...il.com, ffclaire1224@...il.com,
thierry.reding@...il.com, jonathanh@...dia.com, mani@...nel.org,
linux-pci@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, kthota@...dia.com,
mmaddireddy@...dia.com, sagar.tv@...il.com
Subject: Re: [PATCH V5 1/3] PCI: designware-ep: Fix DBI access before core init
On Thu, Oct 13, 2022 Vidya Sagar <vidyas@...dia.com> 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 the core being
> ready 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.
> It also introduces .ep_init_late() ops hook to be used for any post init
> work that platform drivers may have to do.
>
> Signed-off-by: Vidya Sagar <vidyas@...dia.com>
> ---
> V5:
> * Changed dw_pcie_ep_init_complete() to dw_pcie_ep_init_late()
> * Skipped memory allocation if done already. This is to avoid freeing and then
> allocating again during PERST# toggles from the host.
>
> V4:
> * Addressed review comments from Bjorn and Manivannan
> * Moved dw_pcie_ep_init_complete() inside dw_pcie_ep_init_notify()
> * Added .ep_init_late() ops to perform late init tasks
>
> .../pci/controller/dwc/pcie-designware-ep.c | 125 +++++++++++-------
> drivers/pci/controller/dwc/pcie-designware.h | 10 +-
> 2 files changed, 80 insertions(+), 55 deletions(-)
Acked-by: Jingoo Han <jingoohan1@...il.com>
The function name 'dw_pcie_ep_init_late' looks clear to me. Also,
commit messages
and comments are understandable. So, I am ok with the patches, but I also think
that we need to wait for Manivannan's Tested-by on qcom platforms.
Best regards,
Jingoo Han
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 83ddb190292e..f300ea2f7bf7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -23,14 +23,6 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
> }
> EXPORT_SYMBOL_GPL(dw_pcie_ep_linkup);
>
> -void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
> -{
> - struct pci_epc *epc = ep->epc;
> -
> - pci_epc_init_notify(epc);
> -}
> -EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify);
> -
> struct dw_pcie_ep_func *
> dw_pcie_ep_get_func_from_ep(struct dw_pcie_ep *ep, u8 func_no)
> {
> @@ -640,12 +632,17 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
> return 0;
> }
>
> -int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> +static int dw_pcie_ep_init_late(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;
>
> @@ -658,6 +655,51 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> return -EIO;
> }
>
> + dw_pcie_version_detect(pci);
> +
> + dw_pcie_iatu_detect(pci);
> +
> + if (!ep->ib_window_map) {
> + ep->ib_window_map = devm_bitmap_zalloc(dev, pci->num_ib_windows,
> + GFP_KERNEL);
> + if (!ep->ib_window_map)
> + return -ENOMEM;
> + }
> +
> + if (!ep->ob_window_map) {
> + ep->ob_window_map = devm_bitmap_zalloc(dev, pci->num_ob_windows,
> + GFP_KERNEL);
> + if (!ep->ob_window_map)
> + return -ENOMEM;
> + }
> +
> + if (!ep->outbound_addr) {
> + 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 = dw_pcie_ep_get_func_from_ep(ep, func_no);
> + if (ep_func)
> + continue;
> +
> + 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);
> @@ -676,13 +718,28 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>
> return 0;
> }
> -EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
> +
> +int dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
> +{
> + struct pci_epc *epc = ep->epc;
> + int ret;
> +
> + ret = dw_pcie_ep_init_late(ep);
> + if (ret)
> + return ret;
> +
> + if (ep->ops->ep_init_late)
> + ep->ops->ep_init_late(ep);
> +
> + pci_epc_init_notify(epc);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify);
>
> 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);
> @@ -690,7 +747,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);
>
> @@ -719,26 +775,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> ep->phys_base = res->start;
> ep->addr_size = resource_size(res);
>
> - dw_pcie_version_detect(pci);
> -
> - dw_pcie_iatu_detect(pci);
> -
> - ep->ib_window_map = devm_bitmap_zalloc(dev, pci->num_ib_windows,
> - GFP_KERNEL);
> - if (!ep->ib_window_map)
> - return -ENOMEM;
> -
> - ep->ob_window_map = devm_bitmap_zalloc(dev, pci->num_ob_windows,
> - 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);
>
> @@ -755,20 +791,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);
>
> @@ -793,7 +815,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> return 0;
> }
>
> - ret = dw_pcie_ep_init_complete(ep);
> + /*
> + * 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.
> + */
> + ret = dw_pcie_ep_init_late(ep);
> if (ret)
> goto err_free_epc_mem;
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 45fcdfc8c035..7252513956b7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -253,6 +253,7 @@ struct dw_pcie_rp {
>
> struct dw_pcie_ep_ops {
> void (*ep_init)(struct dw_pcie_ep *ep);
> + void (*ep_init_late)(struct dw_pcie_ep *ep);
> int (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
> enum pci_epc_irq_type type, u16 interrupt_num);
> const struct pci_epc_features* (*get_features)(struct dw_pcie_ep *ep);
> @@ -467,8 +468,7 @@ static inline void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus,
> #ifdef CONFIG_PCIE_DW_EP
> void dw_pcie_ep_linkup(struct dw_pcie_ep *ep);
> int dw_pcie_ep_init(struct dw_pcie_ep *ep);
> -int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep);
> -void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep);
> +int dw_pcie_ep_init_notify(struct dw_pcie_ep *ep);
> void dw_pcie_ep_exit(struct dw_pcie_ep *ep);
> int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 func_no);
> int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
> @@ -490,15 +490,11 @@ static inline int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> return 0;
> }
>
> -static inline int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> +static inline int dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
> {
> return 0;
> }
>
> -static inline void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
> -{
> -}
> -
> static inline void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
> {
> }
> --
> 2.17.1
>
Powered by blists - more mailing lists