On Sat, Dec 16, 2023 at 10:11:19AM +0000, Niklas Cassel wrote:
> On Thu, Dec 14, 2023 at 04:28:44PM -0500, Frank Li wrote:
> >
>
> (snip)
>
> > Does below change fix your problem?
>
> It is basically the same fix as I sent out earlier in this thread,
> but yes, it does solve 1 out of 2 problems introduced by the patch
> in $subject, so:
>
> Tested-by: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index f6207989fc6ad..2868d44649ef7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -177,7 +177,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > if (!ep->bar_to_atu[bar])
> > free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
> > else
> > - free_win = ep->bar_to_atu[bar];
> > + free_win = ep->bar_to_atu[bar] - 1;
> >
> > if (free_win >= pci->num_ib_windows) {
> > dev_err(pci->dev, "No free inbound window\n");
> > @@ -191,7 +191,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > return ret;
> > }
> >
> > - ep->bar_to_atu[bar] = free_win;
> > + ep->bar_to_atu[bar] = free_win + 1;
> > set_bit(free_win, ep->ib_window_map);
> >
> > return 0;
> > @@ -228,7 +228,7 @@ static void dw_pcie_ep_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > struct dw_pcie_ep *ep = epc_get_drvdata(epc);
> > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > enum pci_barno bar = epf_bar->barno;
> > - u32 atu_index = ep->bar_to_atu[bar];
> > + u32 atu_index = ep->bar_to_atu[bar] - 1;
>
> You probably want to add a:
>
> if (!ep->bar_to_atu[bar])
> return;
>
> here, so that dw_pcie_ep_clear_bar() will never try to use -1 as index.
> (E.g. if clear_bar() is called twice on the same BAR.)
>
> >
> > __dw_pcie_ep_reset_bar(pci, func_no, bar, epf_bar->flags);
> >
>
> (snip)
>
> > > > from dw_pcie_ep_set_bar(), also needs to be dropped, so that the iATU
> > > > settings will be re-written for platforms with core_init_notifiers.
> > > >
> > > > Right now, for a platform with a core_init_notifier, if you run
> > > > pci_endpoint_test + reboot the RC (so that PERST is asserted + deasserted),
> > > > and then run pci_endpoint_test again, then I'm quite sure that
> > > > pci_endpoint_test will not pass the second time (because iATU settings
> > > > were not rewritten after reset).
> > > >
> > > > It would be nice if Mani or Vidya could confirm this.
>
> So problem 2 out of 2 introduced by the patch in $subject is that
> DWC drivers with a .core_init_notifier, will perform a reset_control_assert()
> to reset the core (which will reset both sticky and non-sticky registers),
> which means that the early return in dw_pcie_ep_set_bar():
> https://github.com/torvalds/linux/blob/v6.7-rc5/drivers/pci/controller/dwc/pcie-designware-ep.c#L268-L269
>
> while returning after the iATU settings have been written,
> it will return before:
>
> dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1));
> dw_pcie_writel_dbi(pci, reg, flags);
>
> Which means that, for drivers with a .core_init_notifier, BARx_REG and
> BARx_MASK registers will not be written. This means that they will have
> reset values for these registers, which means that e.g. the BAR_SIZE
> (which is defined by BARx_MASK) might be incorrect for these platforms
> because this function returns early.
>
> I will not send a fix for this problem, I will leave that to you, or Mani,
> or Vidya, and hope that people are happy that I simply reported this issue.
>
The fix for this issue shouldn't be in the DWC driver but rather in the EPF
drivers. Because, EPF drivers are the ones calling pci_epc_set_bar() during
bind(). So they have to properly cleanup the resources once the perst assertion
happens. This issue also applies to other resources such as DMA channels.
The problem here is, there is a disconnect between DWC (EPC) and EPF drivers.
When the perst assertion happens, the event is not passed to EPF drivers
resulting in the EPF drivers having no knowledge that they need to give up the
resources.
We need to fix this in a sane way and I'm looking into it.
But I really appreciate your finding here and in other thread where we discussed
similar issue.
- Mani
>
> Here is my suggested solution in case anyone wants to take a stab at it:
>
> > > > I guess one option would be modify dw_pcie_ep_init_notify() to call
> > > > dw_pcie_ep_clear_bar() on all non-NULL BARs stored in ep->epf_bar[],
> > > > before calling pci_epc_init_notify(). That way the second .core_init
> > > > (pci_epf_test_core_init()) call will use write the settings, because
> > > > ep->epf_bar[] will be empty, so the "write the settings only the first
> > > > time" approach will then also work for core_init_notifier platforms.
>
>
> Kind regards,
> Niklas
--
மணிவண்ணன் சதாசிவம்
