[PATCH 0/3] Make pci/endpoint branch build

[PATCH 0/3] Make pci/endpoint branch build

[Niklas Cassel]

The pci/next branch has currently not merged the latest pci/endpoint
branch.

Perhaps this delay in creating a new pci/next branch is because the
pci/endpoint branch currently fails to build.

Fix the build error on the pci/endpoint branch such that it will be
possible to queue further patches.


Kind regards,
Niklas

Niklas Cassel (3):
  PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()
  PCI: qcom-ep: Make use of dw_pcie_ep_deinit_notify()
  PCI: tegra194: Make use of dw_pcie_ep_deinit_notify()

 drivers/pci/controller/dwc/pcie-designware-ep.c | 13 +++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h    |  5 +++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c       |  2 +-
 drivers/pci/controller/dwc/pcie-tegra194.c      |  2 +-
 4 files changed, 20 insertions(+), 2 deletions(-)

-- 
2.45.1

[PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Niklas Cassel]

Add a DWC specific wrapper function (dw_pcie_ep_deinit_notify()) around
pci_epc_deinit_notify(), similar to how we have a wrapper function
(dw_pcie_ep_init_notify()) around pci_epc_init_notify().

This will allow the DWC glue drivers to use the same API layer for init
and deinit notification.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 13 +++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h    |  5 +++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 2063cf2049e5..3c9079651dff 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -39,6 +39,19 @@ void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
 }
 EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify);
 
+/**
+ * dw_pcie_ep_deinit_notify - Notify EPF drivers about EPC deinitialization
+ *			      complete
+ * @ep: DWC EP device
+ */
+void dw_pcie_ep_deinit_notify(struct dw_pcie_ep *ep)
+{
+	struct pci_epc *epc = ep->epc;
+
+	pci_epc_deinit_notify(epc);
+}
+EXPORT_SYMBOL_GPL(dw_pcie_ep_deinit_notify);
+
 /**
  * dw_pcie_ep_get_func_from_ep - Get the struct dw_pcie_ep_func corresponding to
  *				 the endpoint function
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index f8e5431a207b..dc63f764b8ba 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -672,6 +672,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep);
 int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep);
 void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep);
 void dw_pcie_ep_deinit(struct dw_pcie_ep *ep);
+void dw_pcie_ep_deinit_notify(struct dw_pcie_ep *ep);
 void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep);
 int dw_pcie_ep_raise_intx_irq(struct dw_pcie_ep *ep, u8 func_no);
 int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
@@ -706,6 +707,10 @@ static inline void dw_pcie_ep_deinit(struct dw_pcie_ep *ep)
 {
 }
 
+static inline void dw_pcie_ep_deinit_notify(struct dw_pcie_ep *ep)
+{
+}
+
 static inline void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep)
 {
 }
-- 
2.45.1

Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Bjorn Helgaas]

On Tue, May 28, 2024 at 03:00:37PM +0200, Niklas Cassel wrote:
> Add a DWC specific wrapper function (dw_pcie_ep_deinit_notify()) around
> pci_epc_deinit_notify(), similar to how we have a wrapper function
> (dw_pcie_ep_init_notify()) around pci_epc_init_notify().
> 
> This will allow the DWC glue drivers to use the same API layer for init
> and deinit notification.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 13 +++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h    |  5 +++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 2063cf2049e5..3c9079651dff 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -39,6 +39,19 @@ void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify);
>  
> +/**
> + * dw_pcie_ep_deinit_notify - Notify EPF drivers about EPC deinitialization
> + *			      complete
> + * @ep: DWC EP device
> + */
> +void dw_pcie_ep_deinit_notify(struct dw_pcie_ep *ep)
> +{
> +	struct pci_epc *epc = ep->epc;
> +
> +	pci_epc_deinit_notify(epc);
> +}
> +EXPORT_SYMBOL_GPL(dw_pcie_ep_deinit_notify);

What is the value of this wrapper?  

I see that dw_pcie_ep_deinit_notify() would be parallel to
dw_pcie_ep_init_notify() and dw_pcie_ep_linkup(), but none of these
has any DWC-specific content other than the fact that
pcie-designware.h provides stubs for the non-CONFIG_PCIE_DW_EP case.

What if we added stubs to pci-epc.h pci_epc_init_notify(),
pci_epc_deinit_notify(), pci_epc_linkup(), and pci_epc_linkdown() for
the non-CONFIG_PCI_ENDPOINT case instead?  Then we might be able to
drop all these DWC-specific wrappers.

>  /**
>   * dw_pcie_ep_get_func_from_ep - Get the struct dw_pcie_ep_func corresponding to
>   *				 the endpoint function
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index f8e5431a207b..dc63f764b8ba 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -672,6 +672,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep);
>  int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep);
>  void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep);
>  void dw_pcie_ep_deinit(struct dw_pcie_ep *ep);
> +void dw_pcie_ep_deinit_notify(struct dw_pcie_ep *ep);
>  void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep);
>  int dw_pcie_ep_raise_intx_irq(struct dw_pcie_ep *ep, u8 func_no);
>  int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
> @@ -706,6 +707,10 @@ static inline void dw_pcie_ep_deinit(struct dw_pcie_ep *ep)
>  {
>  }
>  
> +static inline void dw_pcie_ep_deinit_notify(struct dw_pcie_ep *ep)
> +{
> +}
> +
>  static inline void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep)
>  {
>  }
> -- 
> 2.45.1
> 

Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Niklas Cassel]

On Tue, May 28, 2024 at 10:55:34AM -0500, Bjorn Helgaas wrote:
> On Tue, May 28, 2024 at 03:00:37PM +0200, Niklas Cassel wrote:
> > Add a DWC specific wrapper function (dw_pcie_ep_deinit_notify()) around
> > pci_epc_deinit_notify(), similar to how we have a wrapper function
> > (dw_pcie_ep_init_notify()) around pci_epc_init_notify().
> > 
> > This will allow the DWC glue drivers to use the same API layer for init
> > and deinit notification.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 13 +++++++++++++
> >  drivers/pci/controller/dwc/pcie-designware.h    |  5 +++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 2063cf2049e5..3c9079651dff 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -39,6 +39,19 @@ void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify);
> >  
> > +/**
> > + * dw_pcie_ep_deinit_notify - Notify EPF drivers about EPC deinitialization
> > + *			      complete
> > + * @ep: DWC EP device
> > + */
> > +void dw_pcie_ep_deinit_notify(struct dw_pcie_ep *ep)
> > +{
> > +	struct pci_epc *epc = ep->epc;
> > +
> > +	pci_epc_deinit_notify(epc);
> > +}
> > +EXPORT_SYMBOL_GPL(dw_pcie_ep_deinit_notify);
> 
> What is the value of this wrapper?  
> 
> I see that dw_pcie_ep_deinit_notify() would be parallel to
> dw_pcie_ep_init_notify() and dw_pcie_ep_linkup(), but none of these
> has any DWC-specific content other than the fact that
> pcie-designware.h provides stubs for the non-CONFIG_PCIE_DW_EP case.

Exactly what you are saying, consistency with the existing design.

To me, it seems a bit weird to use:
dw_pcie_ep_init_notify() to notify init completion, and then to use
pci_epc_deinit_notify() to notify deinit completion.

deinit notify callback should basically undo what the init notify callback
did, so it would make sense that the naming of the API calls are similar.


> 
> What if we added stubs to pci-epc.h pci_epc_init_notify(),
> pci_epc_deinit_notify(), pci_epc_linkup(), and pci_epc_linkdown() for
> the non-CONFIG_PCI_ENDPOINT case instead?  Then we might be able to
> drop all these DWC-specific wrappers.

The PCI endpoint subsystem currently does not provide any stubs at all,
so that would be a bigger change compared to this small patch.
(And considering that the pci/endpoint branch does not build, I opted
for the smaller patch.)

Your suggestion would of course work as well, but if we go that route,
then we should probably add stubs for all functions in both
include/linux/pci-epc.h and include/linux/pci-epf.h.
As long as the DWC glue drivers use the same "API layer" for init and
deinit notification, I'm happy :)


Kind regards,
Niklas

Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Bjorn Helgaas]

On Tue, May 28, 2024 at 09:17:40PM +0200, Niklas Cassel wrote:
> On Tue, May 28, 2024 at 10:55:34AM -0500, Bjorn Helgaas wrote:
> > On Tue, May 28, 2024 at 03:00:37PM +0200, Niklas Cassel wrote:
> > > Add a DWC specific wrapper function (dw_pcie_ep_deinit_notify()) around
> > > pci_epc_deinit_notify(), similar to how we have a wrapper function
> > > (dw_pcie_ep_init_notify()) around pci_epc_init_notify().
> > > 
> > > This will allow the DWC glue drivers to use the same API layer for init
> > > and deinit notification.
> > > 
> > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware-ep.c | 13 +++++++++++++
> > >  drivers/pci/controller/dwc/pcie-designware.h    |  5 +++++
> > >  2 files changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index 2063cf2049e5..3c9079651dff 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -39,6 +39,19 @@ void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify);
> > >  
> > > +/**
> > > + * dw_pcie_ep_deinit_notify - Notify EPF drivers about EPC deinitialization
> > > + *			      complete
> > > + * @ep: DWC EP device
> > > + */
> > > +void dw_pcie_ep_deinit_notify(struct dw_pcie_ep *ep)
> > > +{
> > > +	struct pci_epc *epc = ep->epc;
> > > +
> > > +	pci_epc_deinit_notify(epc);
> > > +}
> > > +EXPORT_SYMBOL_GPL(dw_pcie_ep_deinit_notify);
> > 
> > What is the value of this wrapper?  
> > 
> > I see that dw_pcie_ep_deinit_notify() would be parallel to
> > dw_pcie_ep_init_notify() and dw_pcie_ep_linkup(), but none of these
> > has any DWC-specific content other than the fact that
> > pcie-designware.h provides stubs for the non-CONFIG_PCIE_DW_EP case.
> 
> Exactly what you are saying, consistency with the existing design.
> 
> To me, it seems a bit weird to use:
> dw_pcie_ep_init_notify() to notify init completion, and then to use
> pci_epc_deinit_notify() to notify deinit completion.
> 
> deinit notify callback should basically undo what the init notify callback
> did, so it would make sense that the naming of the API calls are similar.
> 
> > What if we added stubs to pci-epc.h pci_epc_init_notify(),
> > pci_epc_deinit_notify(), pci_epc_linkup(), and pci_epc_linkdown() for
> > the non-CONFIG_PCI_ENDPOINT case instead?  Then we might be able to
> > drop all these DWC-specific wrappers.
> 
> The PCI endpoint subsystem currently does not provide any stubs at all,
> so that would be a bigger change compared to this small patch.
> (And considering that the pci/endpoint branch does not build, I opted
> for the smaller patch.)

> Your suggestion would of course work as well, but if we go that route,
> then we should probably add stubs for all functions in both
> include/linux/pci-epc.h and include/linux/pci-epf.h.
> As long as the DWC glue drivers use the same "API layer" for init and
> deinit notification, I'm happy :)

The cadence, rcar, and rockchip drivers use pci_epc_init_notify() with
no wrapper.

A bunch of DWC-based drivers (artpec6, dra7xx, imx6, keembay, ks, ls,
qcom, rcar_gen4, etc) use the dw_pcie_ep_init_notify() wrapper.

ls and qcom even use *both*: pci_epc_linkdown() but
dw_pcie_ep_linkup().

Personally I would drop the dw_*() wrappers.  It's a bigger patch but
not any more complicated, and the result is consistency across both
DWC and the non-DWC drivers.

I don't know if we need to add stubs for *all* the functions.  I'd
probably defer that until we trip over them.

Bjorn

Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Niklas Cassel]

On Tue, May 28, 2024 at 02:55:39PM -0500, Bjorn Helgaas wrote:
> On Tue, May 28, 2024 at 09:17:40PM +0200, Niklas Cassel wrote:
> > 
> > > What if we added stubs to pci-epc.h pci_epc_init_notify(),
> > > pci_epc_deinit_notify(), pci_epc_linkup(), and pci_epc_linkdown() for
> > > the non-CONFIG_PCI_ENDPOINT case instead?  Then we might be able to
> > > drop all these DWC-specific wrappers.
> > 
> > The PCI endpoint subsystem currently does not provide any stubs at all,
> > so that would be a bigger change compared to this small patch.
> > (And considering that the pci/endpoint branch does not build, I opted
> > for the smaller patch.)
> 
> > Your suggestion would of course work as well, but if we go that route,
> > then we should probably add stubs for all functions in both
> > include/linux/pci-epc.h and include/linux/pci-epf.h.
> > As long as the DWC glue drivers use the same "API layer" for init and
> > deinit notification, I'm happy :)
> 
> The cadence, rcar, and rockchip drivers use pci_epc_init_notify() with
> no wrapper.
> 
> A bunch of DWC-based drivers (artpec6, dra7xx, imx6, keembay, ks, ls,
> qcom, rcar_gen4, etc) use the dw_pcie_ep_init_notify() wrapper.
> 
> ls and qcom even use *both*: pci_epc_linkdown() but
> dw_pcie_ep_linkup().
> 
> Personally I would drop the dw_*() wrappers.  It's a bigger patch but
> not any more complicated, and the result is consistency across both
> DWC and the non-DWC drivers.
> 
> I don't know if we need to add stubs for *all* the functions.  I'd
> probably defer that until we trip over them.

Hello Mani,

considering that:

1) Bjorn dropped the commit:
"PCI: endpoint: Introduce 'epc_deinit' event and notify the EPF drivers"
which means that you will need resubmit your patch.

2) Any changes I would do would conflict with your patch.
(It probably makes most sense put your patch as the final patch in a series.)

3) You are the PCI endpoint maintainer, so you are most suited to decide
which functions to stub (if any).

4) Your patch only affects tegra and qcom, and I don't have the hardware
for either, so I wouldn't be able to test.

Thus, I do not intend to respin this series.
I hope that is okay with you.


Kind regards,
Niklas

Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Manivannan Sadhasivam]

Hi Niklas,

On Wed, May 29, 2024 at 09:35:44AM +0200, Niklas Cassel wrote:
> On Tue, May 28, 2024 at 02:55:39PM -0500, Bjorn Helgaas wrote:
> > On Tue, May 28, 2024 at 09:17:40PM +0200, Niklas Cassel wrote:
> > > 
> > > > What if we added stubs to pci-epc.h pci_epc_init_notify(),
> > > > pci_epc_deinit_notify(), pci_epc_linkup(), and pci_epc_linkdown() for
> > > > the non-CONFIG_PCI_ENDPOINT case instead?  Then we might be able to
> > > > drop all these DWC-specific wrappers.
> > > 
> > > The PCI endpoint subsystem currently does not provide any stubs at all,
> > > so that would be a bigger change compared to this small patch.
> > > (And considering that the pci/endpoint branch does not build, I opted
> > > for the smaller patch.)
> > 
> > > Your suggestion would of course work as well, but if we go that route,
> > > then we should probably add stubs for all functions in both
> > > include/linux/pci-epc.h and include/linux/pci-epf.h.
> > > As long as the DWC glue drivers use the same "API layer" for init and
> > > deinit notification, I'm happy :)
> > 
> > The cadence, rcar, and rockchip drivers use pci_epc_init_notify() with
> > no wrapper.
> > 
> > A bunch of DWC-based drivers (artpec6, dra7xx, imx6, keembay, ks, ls,
> > qcom, rcar_gen4, etc) use the dw_pcie_ep_init_notify() wrapper.
> > 
> > ls and qcom even use *both*: pci_epc_linkdown() but
> > dw_pcie_ep_linkup().
> > 
> > Personally I would drop the dw_*() wrappers.  It's a bigger patch but
> > not any more complicated, and the result is consistency across both
> > DWC and the non-DWC drivers.
> > 
> > I don't know if we need to add stubs for *all* the functions.  I'd
> > probably defer that until we trip over them.
> 
> Hello Mani,
> 
> considering that:
> 
> 1) Bjorn dropped the commit:
> "PCI: endpoint: Introduce 'epc_deinit' event and notify the EPF drivers"
> which means that you will need resubmit your patch.
> 
> 2) Any changes I would do would conflict with your patch.
> (It probably makes most sense put your patch as the final patch in a series.)
> 
> 3) You are the PCI endpoint maintainer, so you are most suited to decide
> which functions to stub (if any).
> 
> 4) Your patch only affects tegra and qcom, and I don't have the hardware
> for either, so I wouldn't be able to test.
> 
> Thus, I do not intend to respin this series.
> I hope that is okay with you.
> 

That's fine. Thanks a lot for stepping in to fix the build issue. I was on
vacation, so couldn't act on your query/series promptly.

Let us conclude the fix here itself as we have more than 1 threads going on.
I did consider adding the stubs to pci-epc.h, but only the deinit API requires
that. So I thought it will look odd to add stub for only one function, that too
for one of the two variants (init/deinit).

So I went ahead with the ugly (yes) conditional for the deinit_notify API.

Ideally, I would've expected both dwc and EP subsystem to provide stubs for the
APIs used by the common driver (host and EP). But since the controller drivers
were using the conditional check to differentiate between host and EP mode,
compilers were smart enough to spot the dead functions and removes them. So
there were no reports so far.

But in this case, the pci_epc_deinit_notify() is called in a separate helper and
hence the issue.

So to conclude, I think it is best if we can add stub just for
pci_epc_deinit_notify() in pci-epc.h and get rid of the dummy
dw_pcie_ep_init_notify() wrapper to make the init/deinit API usage consistent.

Also I do not want to remove the wrapper for dw_pcie_ep_linkup() since its
conterpart dw_pcie_ep_linkdown() is required.

Let me know what you think! I will submit a new series with the left over
patches.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Niklas Cassel]

On Wed, May 29, 2024 at 07:46:14PM +0530, Manivannan Sadhasivam wrote:
> 
> That's fine. Thanks a lot for stepping in to fix the build issue. I was on
> vacation, so couldn't act on your query/series promptly.

Welcome back ;)


> 
> Let us conclude the fix here itself as we have more than 1 threads going on.
> I did consider adding the stubs to pci-epc.h, but only the deinit API requires
> that. So I thought it will look odd to add stub for only one function, that too
> for one of the two variants (init/deinit).
> 
> So I went ahead with the ugly (yes) conditional for the deinit_notify API.
> 
> Ideally, I would've expected both dwc and EP subsystem to provide stubs for the
> APIs used by the common driver (host and EP). But since the controller drivers
> were using the conditional check to differentiate between host and EP mode,
> compilers were smart enough to spot the dead functions and removes them. So
> there were no reports so far.
> 
> But in this case, the pci_epc_deinit_notify() is called in a separate helper and
> hence the issue.
> 
> So to conclude, I think it is best if we can add stub just for
> pci_epc_deinit_notify() in pci-epc.h and get rid of the dummy
> dw_pcie_ep_init_notify() wrapper to make the init/deinit API usage consistent.
> 
> Also I do not want to remove the wrapper for dw_pcie_ep_linkup() since its
> conterpart dw_pcie_ep_linkdown() is required.

I see, sounds good.

However, if we add a stub for pci_epc_deinit_notify(), it makes sense to also
add a stub for pci_epc_init_notify(). (I'm quite sure tegra will fail to link
if you change it from dw_pcie_ep_init_notify() to pci_epc_init_notify()
otherwise.)

We should probably also address Bjorn comment:
"ls and qcom even use *both*: pci_epc_linkdown() but dw_pcie_ep_linkup()."

As far as I can tell, it is only ls (not sure why Bjorn also mentioned qcom):
drivers/pci/controller/dwc/pci-layerscape-ep.c:         pci_epc_linkdown(pci->ep.epc);
But this should probably also be fixed to use dw_pcie_ep_linkdown().


Kind regards,
Niklas

Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 04:54:57PM +0200, Niklas Cassel wrote:
> On Wed, May 29, 2024 at 07:46:14PM +0530, Manivannan Sadhasivam wrote:
> > 
> > That's fine. Thanks a lot for stepping in to fix the build issue. I was on
> > vacation, so couldn't act on your query/series promptly.
> 
> Welcome back ;)
> 
> 
> > 
> > Let us conclude the fix here itself as we have more than 1 threads going on.
> > I did consider adding the stubs to pci-epc.h, but only the deinit API requires
> > that. So I thought it will look odd to add stub for only one function, that too
> > for one of the two variants (init/deinit).
> > 
> > So I went ahead with the ugly (yes) conditional for the deinit_notify API.
> > 
> > Ideally, I would've expected both dwc and EP subsystem to provide stubs for the
> > APIs used by the common driver (host and EP). But since the controller drivers
> > were using the conditional check to differentiate between host and EP mode,
> > compilers were smart enough to spot the dead functions and removes them. So
> > there were no reports so far.
> > 
> > But in this case, the pci_epc_deinit_notify() is called in a separate helper and
> > hence the issue.
> > 
> > So to conclude, I think it is best if we can add stub just for
> > pci_epc_deinit_notify() in pci-epc.h and get rid of the dummy
> > dw_pcie_ep_init_notify() wrapper to make the init/deinit API usage consistent.
> > 
> > Also I do not want to remove the wrapper for dw_pcie_ep_linkup() since its
> > conterpart dw_pcie_ep_linkdown() is required.
> 
> I see, sounds good.
> 
> However, if we add a stub for pci_epc_deinit_notify(), it makes sense to also
> add a stub for pci_epc_init_notify(). (I'm quite sure tegra will fail to link
> if you change it from dw_pcie_ep_init_notify() to pci_epc_init_notify()
> otherwise.)
> 

No it doesn't. Reason is, the EP IRQ handler itself gets optimized out due to
the CONFIG_PCIE_TEGRA194_EP check in tegra_pcie_dw_probe().

> We should probably also address Bjorn comment:
> "ls and qcom even use *both*: pci_epc_linkdown() but dw_pcie_ep_linkup()."
> 
> As far as I can tell, it is only ls (not sure why Bjorn also mentioned qcom):
> drivers/pci/controller/dwc/pci-layerscape-ep.c:         pci_epc_linkdown(pci->ep.epc);
> But this should probably also be fixed to use dw_pcie_ep_linkdown().
> 

Agree. I will fix that also.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Bjorn Helgaas]

On Wed, May 29, 2024 at 04:54:57PM +0200, Niklas Cassel wrote:
> ...

> We should probably also address Bjorn comment:
> "ls and qcom even use *both*: pci_epc_linkdown() but dw_pcie_ep_linkup()."
> 
> As far as I can tell, it is only ls (not sure why Bjorn also mentioned qcom):
> drivers/pci/controller/dwc/pci-layerscape-ep.c:         pci_epc_linkdown(pci->ep.epc);
> But this should probably also be fixed to use dw_pcie_ep_linkdown().

qcom_pcie_ep_global_irq_thread() calls both pci_epc_linkdown() and
dw_pcie_ep_linkup():

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-qcom-ep.c?id=v6.10-rc1#n628

Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Niklas Cassel]

On Wed, May 29, 2024 at 12:25:15PM -0500, Bjorn Helgaas wrote:
> On Wed, May 29, 2024 at 04:54:57PM +0200, Niklas Cassel wrote:
> > ...
> 
> > We should probably also address Bjorn comment:
> > "ls and qcom even use *both*: pci_epc_linkdown() but dw_pcie_ep_linkup()."
> > 
> > As far as I can tell, it is only ls (not sure why Bjorn also mentioned qcom):
> > drivers/pci/controller/dwc/pci-layerscape-ep.c:         pci_epc_linkdown(pci->ep.epc);
> > But this should probably also be fixed to use dw_pcie_ep_linkdown().
> 
> qcom_pcie_ep_global_irq_thread() calls both pci_epc_linkdown() and
> dw_pcie_ep_linkup():
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-qcom-ep.c?id=v6.10-rc1#n628

I see, you were looking at old code (Linus' tree) ;)
In pci/next there is just ls :)

qcom-ep was fixed in:
813c83de4ac0 ("PCI: qcom-ep: Use the generic dw_pcie_ep_linkdown() API to handle Link Down event")

Anyway, since Mani wanted to keep the wrapper, both qcom and ls should use
the wrapper after his series.


Kind regards,
Niklas

[PATCH 2/3] PCI: qcom-ep: Make use of dw_pcie_ep_deinit_notify()

[Niklas Cassel]

By using dw_pcie_ep_deinit_notify(), the init and deinit notification is
performed using the same API layer. This makes the driver more consistent.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index b6a5010cc1a7..25bf790e9725 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -507,7 +507,7 @@ static void qcom_pcie_perst_assert(struct dw_pcie *pci)
 		return;
 	}
 
-	pci_epc_deinit_notify(pci->ep.epc);
+	dw_pcie_ep_deinit_notify(&pci->ep);
 	dw_pcie_ep_cleanup(&pci->ep);
 	qcom_pcie_disable_resources(pcie_ep);
 	pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
-- 
2.45.1

[PATCH 3/3] PCI: tegra194: Make use of dw_pcie_ep_deinit_notify()

[Niklas Cassel]

By using dw_pcie_ep_deinit_notify(), the init and deinit notification is
performed using the same API layer. This makes the driver more consistent.

It also fixes a linker error where pci_epc_deinit_notify() would fail to
link in certain Kconfig combinations, because the PCI endpoint API layer
does not provide any dummy implementations.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202405130815.BwBrIepL-lkp@intel.com
Fixes: f94f2844f28c ("PCI: endpoint: Introduce 'epc_deinit' event and notify the EPF drivers")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 4b28f8beedfe..be1f10cadf1d 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1715,7 +1715,7 @@ static void pex_ep_event_pex_rst_assert(struct tegra_pcie_dw *pcie)
 	if (ret)
 		dev_err(pcie->dev, "Failed to go Detect state: %d\n", ret);
 
-	pci_epc_deinit_notify(pcie->pci.ep.epc);
+	dw_pcie_ep_deinit_notify(&pcie->pci.ep);
 	dw_pcie_ep_cleanup(&pcie->pci.ep);
 
 	reset_control_assert(pcie->core_rst);
-- 
2.45.1

Re: [PATCH 0/3] Make pci/endpoint branch build

[Bjorn Helgaas]

On Tue, May 28, 2024 at 03:00:36PM +0200, Niklas Cassel wrote:
> The pci/next branch has currently not merged the latest pci/endpoint
> branch.
> 
> Perhaps this delay in creating a new pci/next branch is because the
> pci/endpoint branch currently fails to build.

The topic branches have things queued on them for build testing, but
they haven't been merged to pci/next yet.  It's standard practice that
we don't add v6.11 content to -next until v6.10-rc1 is tagged:
https://lore.kernel.org/linux-next/20240523111023.7ade2aa4@canb.auug.org.au/

v6.10-rc1 was tagged 5/26, and 5/27 was a US holiday.  The topic
branches will be merged to pci/next today.

Bjorn

Re: [PATCH 0/3] Make pci/endpoint branch build

[Niklas Cassel]

On Tue, May 28, 2024 at 09:44:52AM -0500, Bjorn Helgaas wrote:
> On Tue, May 28, 2024 at 03:00:36PM +0200, Niklas Cassel wrote:
> > The pci/next branch has currently not merged the latest pci/endpoint
> > branch.
> > 
> > Perhaps this delay in creating a new pci/next branch is because the
> > pci/endpoint branch currently fails to build.
> 
> The topic branches have things queued on them for build testing, but
> they haven't been merged to pci/next yet.  It's standard practice that
> we don't add v6.11 content to -next until v6.10-rc1 is tagged:
> https://lore.kernel.org/linux-next/20240523111023.7ade2aa4@canb.auug.org.au/
> 
> v6.10-rc1 was tagged 5/26, and 5/27 was a US holiday.  The topic
> branches will be merged to pci/next today.

Ok, great!

Although, perhaps you would want to defer merging pci/endpoint to pci/next
until the linker error has been fixed.

We have already gotten a bunch of reports of this on the linux-pci list:
https://lore.kernel.org/linux-pci/202405270544.yKgcokbA-lkp@intel.com/T/#u
https://lore.kernel.org/linux-pci/202405130815.BwBrIepL-lkp@intel.com/T/#u
https://lore.kernel.org/linux-pci/202405140344.glj9WfzM-lkp@intel.com/T/#u

Merging the branch to pci/next is guaranteed to have even move people hit
and complain about this error.


Kind regards,
Niklas

Re: [PATCH 0/3] Make pci/endpoint branch buildgg

[Bjorn Helgaas]

On Tue, May 28, 2024 at 08:57:04PM +0200, Niklas Cassel wrote:
> On Tue, May 28, 2024 at 09:44:52AM -0500, Bjorn Helgaas wrote:
> > On Tue, May 28, 2024 at 03:00:36PM +0200, Niklas Cassel wrote:
> > > The pci/next branch has currently not merged the latest pci/endpoint
> > > branch.
> > > 
> > > Perhaps this delay in creating a new pci/next branch is because the
> > > pci/endpoint branch currently fails to build.
> > 
> > The topic branches have things queued on them for build testing, but
> > they haven't been merged to pci/next yet.  It's standard practice that
> > we don't add v6.11 content to -next until v6.10-rc1 is tagged:
> > https://lore.kernel.org/linux-next/20240523111023.7ade2aa4@canb.auug.org.au/
> > 
> > v6.10-rc1 was tagged 5/26, and 5/27 was a US holiday.  The topic
> > branches will be merged to pci/next today.
> 
> Ok, great!
> 
> Although, perhaps you would want to defer merging pci/endpoint to pci/next
> until the linker error has been fixed.
> 
> We have already gotten a bunch of reports of this on the linux-pci list:
> https://lore.kernel.org/linux-pci/202405270544.yKgcokbA-lkp@intel.com/T/#u
> https://lore.kernel.org/linux-pci/202405130815.BwBrIepL-lkp@intel.com/T/#u
> https://lore.kernel.org/linux-pci/202405140344.glj9WfzM-lkp@intel.com/T/#u
> 
> Merging the branch to pci/next is guaranteed to have even move people hit
> and complain about this error.

I included pci/endpoint but dropped the "PCI: endpoint: Introduce
'epc_deinit' event and notify the EPF drivers" patch until we fix that
issue.

Bjorn