lkml.org 
[lkml]   [2022]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used
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@socionext.com>
> > > > > Cc: Om Prakash Singh <omp@nvidia.com>
> > > > > Cc: Vidya Sagar <vidyas@nvidia.com>
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > ---
> > > > > .../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@nvidia.com/)
> > > >
> > > > 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@socionext.com/#24633629

Thanks! (Or the canonical permanent URL:
https://lore.kernel.org/r/576457dd-3e66-a3b9-f51c-ea94bc267fdb@ti.com)

> > 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

\
 
 \ /
  Last update: 2022-07-21 17:32    [W:0.121 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site