On Thu, Jul 21, 2022 at 02:08:45PM +0530, Manivannan Sadhasivam wrote: > On Tue, Jul 19, 2022 at 05:38:14PM -0500, Bjorn Helgaas wrote: > > On Tue, Jul 19, 2022 at 09:13:58AM +0530, Manivannan Sadhasivam wrote: > > > On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote: > > > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > > > > > For controllers supporting the CORE_INIT notifier, the resources are > > > > > supposed to be enabled in the init_complete function. Currently, > > > > > these controllers are enabling the resources during probe time due to > > > > > the DBI access happens in dw_pcie_ep_init(). > > > > > > > > > > This creates the dependency with the host PCIe controller since the > > > > > resource enablement like PHY depends on host PCIe to be up. For the > > > > > standalone endpoint usecase, this would never work. So let's move all DBI > > > > > access to init_complete function if CORE_INIT notifier is used. For the > > > > > controllers those doesn't support this notifier, this change is a NO-OP. > > > > > > > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx> > > > > > Cc: Om Prakash Singh <omp@xxxxxxxxxx> > > > > > Cc: Vidya Sagar <vidyas@xxxxxxxxxx> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> > > > > > --- > > > > > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > > > > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > > > 2 files changed, 94 insertions(+), 45 deletions(-) > > > > > > > > Sorry this got missed. Seems like there are two patches to solve the > > > > same problem: > > > > > > > > 1) This patch, and > > > > 2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@xxxxxxxxxx/) > > > > > > > > I don't know much about dwc or this issue, but if these patches are > > > > functionally equivalent, I think Vidya's is a little more attractive > > > > because: > > > > > > > > - It's smaller (49 insertions(+), 39 deletions(-)). > > > > > > > > - "core_init_notifier" looks like sort of a corner-case feature and > > > > Vidya's patch doesn't depend on it so it seems more maintainable. > > > > > > > > - It's more straightforward to read -- it basically just moves > > > > things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which > > > > is exactly the sort of thing I expect if we're doing something out > > > > of order. > > > > > > I agree that Vidya's patch is simple but as per the feedback from > > > Kishon on my previous patch, I had to add some extra logic to make > > > sure the move of DBI access doesn't affect the non > > > core_init_notifier platforms. > > > > > > So with my patch, the logic added is essentailly a NO-OP on those. > > > > Can you include the lore URL for Kishon's feedback? I can't find it. > > https://patchwork.kernel.org/project/linux-pci/patch/1630473361-27198-3-git-send-email-hayashi.kunihiko@xxxxxxxxxxxxx/#24633629 Thanks! (Or the canonical permanent URL: https://lore.kernel.org/r/576457dd-3e66-a3b9-f51c-ea94bc267fdb@xxxxxx) > > If we think moving the DBI access is safe on non-core_init_notifier > > platforms, I'd like to do it everywhere so they're all the same. I > > don't want different behavior just to avoid the risk of theoretical > > problems that we think should not happen. > > One more issue Kishon pointed out was that, in the patch the > endpoint controller is configured after pci_epc_create(). So he > raised a concern that if ecp_ops is invoked before the controller > gets configured fully, it could result in aborts. > > While the concern may be true for non-core_init_notifier platforms > (I'm not sure though) but I'm certain not for the core_init_notifier > ones as the EFP drivers only access EPC ops after > dw_pcie_ep_init_complete(). I really don't understand what "core_init_notifier" does, but it seems incidental to this issue. Is there really a connection? It sounds like the only reason to check for it is to limit the scope of the change, not because DBI access is inherently related to core_init_notifier. Maybe Vidya's patch needs some enhancement to cover other paths where DBI may be accessed before we're ready? Bjorn
