On Mon, Mar 04, 2024 at 05:46:16PM -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 free_win + 1. Initialize bar_to_atu as 0 to indicate
> it have not allocate atu to the bar.
Lorenzo, Krzysztof, Manivannan: any thoughts on this? I don't want to
ask Linus to pull it during the merge window since this hasn't even
been in -next yet. But we could consider it as a fix for v6.9 if it's
urgent.
IMO the commit log does need to say something about what the actual
user-visible problem is. I can't tell what breakage this fixes.
> Reported-by: Niklas Cassel <Niklas.Cassel@xxxxxxx>
> Closes: 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")
> Reviewed-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> ---
>
> Notes:
> Change from v1 to v2
> - update subject
> - use free_win + 1 solution
> - still leave MAX_IATU_IN as 256. I am not sure if there are platfrom have
> 256 ATU. Suppose it only use max 6 in current EP framework.
> - @Niklas, can you help test it. My platform become unstable today.
>
> drivers/pci/controller/dwc/pcie-designware-ep.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 5befed2dc02b7..ba932bafdb230 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -139,7 +139,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");
> @@ -153,7 +153,11 @@ 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;
> + /*
> + * Always increment free_win before assignment, since value 0 is used to identify
> + * unallocated mapping.
> + */
> + ep->bar_to_atu[bar] = free_win + 1;
> set_bit(free_win, ep->ib_window_map);
>
> return 0;
> @@ -190,7 +194,10 @@ 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;
> +
> + if (!ep->bar_to_atu[bar])
> + return;
>
> __dw_pcie_ep_reset_bar(pci, func_no, bar, epf_bar->flags);
>
> --
> 2.34.1
>
