Re: [PATCH v12 8/8] PCI: endpoint: Remove "core_init_notifier" flag
On Fri, Apr 12, 2024 at 03:22:16PM -0500, Bjorn Helgaas wrote: > On Wed, Mar 27, 2024 at 02:43:37PM +0530, Manivannan Sadhasivam wrote: > > "core_init_notifier" flag is set by the glue drivers requiring refclk from > > the host to complete the DWC core initialization. Also, those drivers will > > send a notification to the EPF drivers once the initialization is fully > > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > > will start functioning. > > > > For the rest of the drivers generating refclk locally, EPF drivers will > > start functioning post binding with them. EPF drivers rely on the > > 'core_init_notifier' flag to differentiate between the drivers. > > Unfortunately, this creates two different flows for the EPF drivers. > > > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > > a single initialization flow for the EPF drivers. This is done by calling > > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > > send the notification to the EPF drivers once the initialization is fully > > completed. > > Thanks for doing this! I think this is a significantly nicer > solution than core_init_notifier was. > > One question: both qcom and tegra194 call dw_pcie_ep_init_registers() > from an interrupt handler, but they register that handler in a > different order with respect to dw_pcie_ep_init(). > > I don't know what actually starts the process that leads to the > interrupt, but if it's dw_pcie_ep_init(), then one of these (qcom, I > think) must be racy: > Your analysis is correct. But there is no race observed as of now since the IRQ will only be enabled by configuring the endpoint using configfs interface and right now I use an init script to do that. By that time, the driver would've already probed completely. But there is a slight chance that if the driver gets loaded as a module and the userspace script starts configuring the endpoint interface using inotify watch or something similar, then race could occur since the IRQ handler may not be registered at that point. > qcom_pcie_ep_probe > dw_pcie_ep_init <- A > qcom_pcie_ep_enable_irq_resources > devm_request_threaded_irq(qcom_pcie_ep_perst_irq_thread) <- B > > qcom_pcie_ep_perst_irq_thread > qcom_pcie_perst_deassert > dw_pcie_ep_init_registers > > tegra_pcie_dw_probe > tegra_pcie_config_ep > devm_request_threaded_irq(tegra_pcie_ep_pex_rst_irq) <- B > dw_pcie_ep_init <- A > > tegra_pcie_ep_pex_rst_irq > pex_ep_event_pex_rst_deassert > dw_pcie_ep_init_registers > > Whatever the right answer is, I think qcom and tegra194 should both > order dw_pcie_ep_init() and the devm_request_threaded_irq() the same > way. > Agree. The right way is to register the IRQ handler first and then do dw_pcie_ep_init(). I will fix it in the qcom driver. Thanks for spotting! - Mani -- மணிவண்ணன் சதாசிவம்
Re: [PATCH v12 8/8] PCI: endpoint: Remove "core_init_notifier" flag
On Wed, Mar 27, 2024 at 02:43:37PM +0530, Manivannan Sadhasivam wrote: > "core_init_notifier" flag is set by the glue drivers requiring refclk from > the host to complete the DWC core initialization. Also, those drivers will > send a notification to the EPF drivers once the initialization is fully > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > will start functioning. > > For the rest of the drivers generating refclk locally, EPF drivers will > start functioning post binding with them. EPF drivers rely on the > 'core_init_notifier' flag to differentiate between the drivers. > Unfortunately, this creates two different flows for the EPF drivers. > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > a single initialization flow for the EPF drivers. This is done by calling > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > send the notification to the EPF drivers once the initialization is fully > completed. Thanks for doing this! I think this is a significantly nicer solution than core_init_notifier was. One question: both qcom and tegra194 call dw_pcie_ep_init_registers() from an interrupt handler, but they register that handler in a different order with respect to dw_pcie_ep_init(). I don't know what actually starts the process that leads to the interrupt, but if it's dw_pcie_ep_init(), then one of these (qcom, I think) must be racy: qcom_pcie_ep_probe dw_pcie_ep_init <- A qcom_pcie_ep_enable_irq_resources devm_request_threaded_irq(qcom_pcie_ep_perst_irq_thread) <- B qcom_pcie_ep_perst_irq_thread qcom_pcie_perst_deassert dw_pcie_ep_init_registers tegra_pcie_dw_probe tegra_pcie_config_ep devm_request_threaded_irq(tegra_pcie_ep_pex_rst_irq) <- B dw_pcie_ep_init <- A tegra_pcie_ep_pex_rst_irq pex_ep_event_pex_rst_deassert dw_pcie_ep_init_registers Whatever the right answer is, I think qcom and tegra194 should both order dw_pcie_ep_init() and the devm_request_threaded_irq() the same way. Bjorn
Re: [PATCH v12 8/8] PCI: endpoint: Remove "core_init_notifier" flag
On Wed, Mar 27, 2024 at 02:43:37PM +0530, Manivannan Sadhasivam wrote: > "core_init_notifier" flag is set by the glue drivers requiring refclk from > the host to complete the DWC core initialization. Also, those drivers will > send a notification to the EPF drivers once the initialization is fully > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > will start functioning. > > For the rest of the drivers generating refclk locally, EPF drivers will > start functioning post binding with them. EPF drivers rely on the > 'core_init_notifier' flag to differentiate between the drivers. > Unfortunately, this creates two different flows for the EPF drivers. > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > a single initialization flow for the EPF drivers. This is done by calling > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > send the notification to the EPF drivers once the initialization is fully > completed. > > Only difference here is that, the drivers requiring refclk from host will > send the notification once refclk is received, while others will send it > during probe time itself. > > But this also requires the EPC core driver to deliver the notification > after EPF driver bind. Because, the glue driver can send the notification > before the EPF drivers bind() and in those cases the EPF drivers will miss > the event. To accommodate this, EPC core is now caching the state of the > EPC initialization in 'init_complete' flag and pci-ep-cfs driver sends the > notification to EPF drivers based on that after each EPF driver bind. > > Tested-by: Niklas Cassel Reviewed-by: Frank Li > Signed-off-by: Manivannan Sadhasivam > --- > drivers/pci/controller/cadence/pcie-cadence-ep.c | 2 ++ > drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ > drivers/pci/controller/dwc/pci-imx6.c | 2 ++ > drivers/pci/controller/dwc/pci-keystone.c | 2 ++ > drivers/pci/controller/dwc/pci-layerscape-ep.c| 2 ++ > drivers/pci/controller/dwc/pcie-artpec6.c | 2 ++ > drivers/pci/controller/dwc/pcie-designware-ep.c | 1 + > drivers/pci/controller/dwc/pcie-designware-plat.c | 2 ++ > drivers/pci/controller/dwc/pcie-keembay.c | 2 ++ > drivers/pci/controller/dwc/pcie-qcom-ep.c | 1 - > drivers/pci/controller/dwc/pcie-rcar-gen4.c | 2 ++ > drivers/pci/controller/dwc/pcie-tegra194.c| 1 - > drivers/pci/controller/dwc/pcie-uniphier-ep.c | 2 ++ > drivers/pci/controller/pcie-rcar-ep.c | 2 ++ > drivers/pci/controller/pcie-rockchip-ep.c | 2 ++ > drivers/pci/endpoint/functions/pci-epf-test.c | 18 +- > drivers/pci/endpoint/pci-ep-cfs.c | 9 + > drivers/pci/endpoint/pci-epc-core.c | 22 ++ > include/linux/pci-epc.h | 7 --- > 19 files changed, 65 insertions(+), 18 deletions(-) > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c > b/drivers/pci/controller/cadence/pcie-cadence-ep.c > index 81c50dc64da9..55c42ca2b777 100644 > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c > @@ -746,6 +746,8 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep) > > spin_lock_init(>lock); > > + pci_epc_init_notify(epc); > + > return 0; > > free_epc_mem: > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c > b/drivers/pci/controller/dwc/pci-dra7xx.c > index 395042b29ffc..d2d17d37d3e0 100644 > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > @@ -474,6 +474,8 @@ static int dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx, > return ret; > } > > + dw_pcie_ep_init_notify(ep); > + > return 0; > } > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > b/drivers/pci/controller/dwc/pci-imx6.c > index 8d28ecc381bc..917c69edee1d 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -1131,6 +1131,8 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie, > return ret; > } > > + dw_pcie_ep_init_notify(ep); > + > /* Start LTSSM. */ > imx6_pcie_ltssm_enable(dev); > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c > b/drivers/pci/controller/dwc/pci-keystone.c > index 81ebac520650..d3a7d14ee685 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -1293,6 +1293,8 @@ static int ks_pcie_probe(struct platform_device *pdev) > goto err_ep_init; > } > > + dw_pcie_ep_init_notify(>ep); > + > break; > default: > dev_err(dev, "INVALID device type %d\n", mode); > diff --git
Re: [PATCH v12 8/8] PCI: endpoint: Remove "core_init_notifier" flag
On Wed, Mar 27, 2024 at 02:43:37PM +0530, Manivannan Sadhasivam wrote: > "core_init_notifier" flag is set by the glue drivers requiring refclk from > the host to complete the DWC core initialization. Also, those drivers will > send a notification to the EPF drivers once the initialization is fully > completed using the pci_epc_init_notify() API. Only then, the EPF drivers > will start functioning. > > For the rest of the drivers generating refclk locally, EPF drivers will > start functioning post binding with them. EPF drivers rely on the > 'core_init_notifier' flag to differentiate between the drivers. > Unfortunately, this creates two different flows for the EPF drivers. > > So to avoid that, let's get rid of the "core_init_notifier" flag and follow > a single initialization flow for the EPF drivers. This is done by calling > the dw_pcie_ep_init_notify() from all glue drivers after the completion of > dw_pcie_ep_init_registers() API. This will allow all the glue drivers to > send the notification to the EPF drivers once the initialization is fully > completed. > > Only difference here is that, the drivers requiring refclk from host will > send the notification once refclk is received, while others will send it > during probe time itself. > > But this also requires the EPC core driver to deliver the notification > after EPF driver bind. Because, the glue driver can send the notification > before the EPF drivers bind() and in those cases the EPF drivers will miss > the event. To accommodate this, EPC core is now caching the state of the > EPC initialization in 'init_complete' flag and pci-ep-cfs driver sends the > notification to EPF drivers based on that after each EPF driver bind. > > Tested-by: Niklas Cassel > Signed-off-by: Manivannan Sadhasivam > --- Reviewed-by: Niklas Cassel
[PATCH v12 8/8] PCI: endpoint: Remove "core_init_notifier" flag
"core_init_notifier" flag is set by the glue drivers requiring refclk from the host to complete the DWC core initialization. Also, those drivers will send a notification to the EPF drivers once the initialization is fully completed using the pci_epc_init_notify() API. Only then, the EPF drivers will start functioning. For the rest of the drivers generating refclk locally, EPF drivers will start functioning post binding with them. EPF drivers rely on the 'core_init_notifier' flag to differentiate between the drivers. Unfortunately, this creates two different flows for the EPF drivers. So to avoid that, let's get rid of the "core_init_notifier" flag and follow a single initialization flow for the EPF drivers. This is done by calling the dw_pcie_ep_init_notify() from all glue drivers after the completion of dw_pcie_ep_init_registers() API. This will allow all the glue drivers to send the notification to the EPF drivers once the initialization is fully completed. Only difference here is that, the drivers requiring refclk from host will send the notification once refclk is received, while others will send it during probe time itself. But this also requires the EPC core driver to deliver the notification after EPF driver bind. Because, the glue driver can send the notification before the EPF drivers bind() and in those cases the EPF drivers will miss the event. To accommodate this, EPC core is now caching the state of the EPC initialization in 'init_complete' flag and pci-ep-cfs driver sends the notification to EPF drivers based on that after each EPF driver bind. Tested-by: Niklas Cassel Signed-off-by: Manivannan Sadhasivam --- drivers/pci/controller/cadence/pcie-cadence-ep.c | 2 ++ drivers/pci/controller/dwc/pci-dra7xx.c | 2 ++ drivers/pci/controller/dwc/pci-imx6.c | 2 ++ drivers/pci/controller/dwc/pci-keystone.c | 2 ++ drivers/pci/controller/dwc/pci-layerscape-ep.c| 2 ++ drivers/pci/controller/dwc/pcie-artpec6.c | 2 ++ drivers/pci/controller/dwc/pcie-designware-ep.c | 1 + drivers/pci/controller/dwc/pcie-designware-plat.c | 2 ++ drivers/pci/controller/dwc/pcie-keembay.c | 2 ++ drivers/pci/controller/dwc/pcie-qcom-ep.c | 1 - drivers/pci/controller/dwc/pcie-rcar-gen4.c | 2 ++ drivers/pci/controller/dwc/pcie-tegra194.c| 1 - drivers/pci/controller/dwc/pcie-uniphier-ep.c | 2 ++ drivers/pci/controller/pcie-rcar-ep.c | 2 ++ drivers/pci/controller/pcie-rockchip-ep.c | 2 ++ drivers/pci/endpoint/functions/pci-epf-test.c | 18 +- drivers/pci/endpoint/pci-ep-cfs.c | 9 + drivers/pci/endpoint/pci-epc-core.c | 22 ++ include/linux/pci-epc.h | 7 --- 19 files changed, 65 insertions(+), 18 deletions(-) diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c index 81c50dc64da9..55c42ca2b777 100644 --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c @@ -746,6 +746,8 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep) spin_lock_init(>lock); + pci_epc_init_notify(epc); + return 0; free_epc_mem: diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c index 395042b29ffc..d2d17d37d3e0 100644 --- a/drivers/pci/controller/dwc/pci-dra7xx.c +++ b/drivers/pci/controller/dwc/pci-dra7xx.c @@ -474,6 +474,8 @@ static int dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx, return ret; } + dw_pcie_ep_init_notify(ep); + return 0; } diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 8d28ecc381bc..917c69edee1d 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -1131,6 +1131,8 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie, return ret; } + dw_pcie_ep_init_notify(ep); + /* Start LTSSM. */ imx6_pcie_ltssm_enable(dev); diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index 81ebac520650..d3a7d14ee685 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -1293,6 +1293,8 @@ static int ks_pcie_probe(struct platform_device *pdev) goto err_ep_init; } + dw_pcie_ep_init_notify(>ep); + break; default: dev_err(dev, "INVALID device type %d\n", mode); diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c index 9eb2233e3d7f..7dde6d5fa4d8 100644 --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c @@ -286,6 +286,8 @@ static int __init
