by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [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 <[email protected]>
> > > > > Cc: Om Prakash Singh <[email protected]>
> > > > > Cc: Vidya Sagar <[email protected]>
> > > > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > > > ---
> > > > > .../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/[email protected]/)
> > > >
> > > > 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/[email protected]/#24633629

Thanks! (Or the canonical permanent URL:
https://lore.kernel.org/r/[email protected])

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

2022-07-26 12:57:47

by Manivannan Sadhasivam

[permalink] [raw]

Subject: Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

On Thu, Jul 21, 2022 at 10:30:55AM -0500, Bjorn Helgaas wrote:
> 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 <[email protected]>
> > > > > > Cc: Om Prakash Singh <[email protected]>
> > > > > > Cc: Vidya Sagar <[email protected]>
> > > > > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > > > > ---
> > > > > > .../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/[email protected]/)
> > > > >
> > > > > 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/[email protected]/#24633629
>
> Thanks! (Or the canonical permanent URL:
> https://lore.kernel.org/r/[email protected])
>
> > > 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.

"core_init_notifer" flag was added to let the EPF driver know that the
endpoint controller has completed its initialization. This is needed for
platforms that require refclk from the host to be active before accessing
any controller specific registers. Tegra194 and Qcom are one of those kinds.

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

DBI access requires refclk on the above mentioned platforms, so yes
there is a strong connection.

This patch (also mine) moves away all of the DBI access to the
init_complete() notifier callback. Because, without this patch we need
to bring-up the host first and then power-on the endpoint. Otherwise,
if the endpoint is powered-on first, it will crash as there would no
refclk from host.

> Maybe Vidya's patch needs some enhancement to cover other paths where
> DBI may be accessed before we're ready?
>

FWIW, Vidya's patch looks good to me. I pointed out my patch just
because it covers the theoretical issue mentioned by Kishon.

Thanks,
Mani

> Bjorn