On Tue, Dec 19, 2023 at 10:07:14AM +0000, Niklas Cassel wrote:
> On Mon, Dec 18, 2023 at 11:48:43PM -0500, Frank Li wrote:
> > dw_pcie_ep_inbound_atu()
> > {
> > ...
> > 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];
> > ...
> > }
> >
> > The atu index 0 is valid case for atu number. The find_first_zero_bit()
> > will return 6 when second time call into this function if atu is 0. Suppose
> > it should use branch 'free_win = ep->bar_to_atu[bar]'.
> >
> > Change 'bar_to_atu' to s8. Initialize bar_to_atu as -1 to indicate it have
> > not allocate atu to the bar.
> >
> > Reported-by: Niklas Cassel <Niklas.Cassel@xxxxxxx>
> > Close: https://lore.kernel.org/linux-pci/ZXt2A+Fusfz3luQV@x1-carbon/T/#u
> > Fixes: 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> >
> > Notes:
> > @Niklas:
> > I have not test your case. I should be equal to previous's fix in
> > mail list.
>
> Hello Frank,
>
> Thank you for sending a proper fix for this!
>
> Personally, I slightly prefer your fix that saves the iatu index + 1, and
> keeps 0 to mean unused. That way, you don't need the memset, and you don't
> need to change the type to signed, but either way is fine by me, so:
index + 1 don't match hardware iATU index. It will be confused because
other parts is 0 based.
So I choose "-1" as free iATU.
Frank
>
> Reviewed-by: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> >
> > drivers/pci/controller/dwc/pcie-designware-ep.c | 11 ++++++++---
> > drivers/pci/controller/dwc/pcie-designware.h | 2 +-
> > 2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index f6207989fc6ad..0ff5cd64f49b0 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -174,7 +174,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > u32 free_win;
> > struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >
> > - if (!ep->bar_to_atu[bar])
> > + if (ep->bar_to_atu[bar] < 0)
> > free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
> > else
> > free_win = ep->bar_to_atu[bar];
> > @@ -228,14 +228,17 @@ 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];
> > + s8 atu_index = ep->bar_to_atu[bar];
> > +
> > + if (atu_index < 0)
> > + return;
> >
> > __dw_pcie_ep_reset_bar(pci, func_no, bar, epf_bar->flags);
> >
> > dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, atu_index);
> > clear_bit(atu_index, ep->ib_window_map);
> > ep->epf_bar[bar] = NULL;
> > - ep->bar_to_atu[bar] = 0;
> > + ep->bar_to_atu[bar] = -1;
> > }
> >
> > static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > @@ -767,6 +770,8 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > return -ENOMEM;
> > ep->outbound_addr = addr;
> >
> > + memset(ep->bar_to_atu, -1, sizeof(ep->bar_to_atu));
> > +
> > epc = devm_pci_epc_create(dev, &epc_ops);
> > if (IS_ERR(epc)) {
> > dev_err(dev, "Failed to create epc device\n");
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 55ff76e3d3846..5879907c5cf25 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -362,7 +362,7 @@ struct dw_pcie_ep {
> > phys_addr_t phys_base;
> > size_t addr_size;
> > size_t page_size;
> > - u8 bar_to_atu[PCI_STD_NUM_BARS];
> > + s8 bar_to_atu[PCI_STD_NUM_BARS];
> > phys_addr_t *outbound_addr;
> > unsigned long *ib_window_map;
> > unsigned long *ob_window_map;
> > --
> > 2.34.1
> >
