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.
Reported-by: Niklas Cassel <[email protected]>
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 <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
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
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.
>
> Reported-by: Niklas Cassel <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Frank Li <[email protected]>
Tested-by: Niklas Cassel <[email protected]>
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.
>
> Reported-by: Niklas Cassel <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Frank Li <[email protected]>
> ---
Any chance of this fix being picked up?
Kind regards,
Niklas
On Wed, Mar 13, 2024 at 12:09:04PM +0100, Niklas Cassel wrote:
> 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.
> >
> > Reported-by: Niklas Cassel <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: Frank Li <[email protected]>
> > ---
>
> Any chance of this fix being picked up?
Gentle ping.
Kind regards,
Niklas
On Wed, Mar 20, 2024 at 09:53:14AM +0100, Niklas Cassel wrote:
> On Wed, Mar 13, 2024 at 12:09:04PM +0100, Niklas Cassel wrote:
> > 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.
> > >
> > > Reported-by: Niklas Cassel <[email protected]>
> > > 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 <[email protected]>
> > > Signed-off-by: Frank Li <[email protected]>
> > > ---
> >
> > Any chance of this fix being picked up?
>
> Gentle ping.
Now it is v6.9 merge windows. You'd better ping two weeks after linus
create v6.9-rc1 tag.
Frank
>
>
> Kind regards,
> Niklas
On Wed, Mar 20, 2024 at 10:29:42AM -0400, Frank Li wrote:
> On Wed, Mar 20, 2024 at 09:53:14AM +0100, Niklas Cassel wrote:
> > On Wed, Mar 13, 2024 at 12:09:04PM +0100, Niklas Cassel wrote:
> > > 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.
> > > >
> > > > Reported-by: Niklas Cassel <[email protected]>
> > > > 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 <[email protected]>
> > > > Signed-off-by: Frank Li <[email protected]>
> > > > ---
> > >
> > > Any chance of this fix being picked up?
> >
> > Gentle ping.
>
> Now it is v6.9 merge windows. You'd better ping two weeks after linus
> create v6.9-rc1 tag.
I don't follow this logic.
Two weeks after v6.9-rc1 is v6.9-rc3.
Why wait that long to merge a fix?
The PCI pull request for v6.9-rc1 has already been merged.
Merging new features (to a submaintainer tree) might temporarily be put
on hold in relation to the merge window, but for fixes, it is quite
possible to both queue things (in a submaintainer tree), and to send
accumulated fixes to Linus during the merge window.
I did it myself just a few days ago, and Linus pulled it already:
https://lore.kernel.org/linux-ide/171087658094.21820.15365015832308818327.pr-tracker-bot@kernel.org/T/#t
Kind regards,
Niklas
On Thu, Mar 21, 2024 at 09:16:56AM +0100, Niklas Cassel wrote:
> On Wed, Mar 20, 2024 at 10:29:42AM -0400, Frank Li wrote:
> > On Wed, Mar 20, 2024 at 09:53:14AM +0100, Niklas Cassel wrote:
> > > On Wed, Mar 13, 2024 at 12:09:04PM +0100, Niklas Cassel wrote:
> > > > 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.
> > > > >
> > > > > Reported-by: Niklas Cassel <[email protected]>
> > > > > 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 <[email protected]>
> > > > > Signed-off-by: Frank Li <[email protected]>
> > > > > ---
> > > >
> > > > Any chance of this fix being picked up?
> > >
> > > Gentle ping.
> >
> > Now it is v6.9 merge windows. You'd better ping two weeks after linus
> > create v6.9-rc1 tag.
>
> I don't follow this logic.
> Two weeks after v6.9-rc1 is v6.9-rc3.
> Why wait that long to merge a fix?
>
> The PCI pull request for v6.9-rc1 has already been merged.
>
> Merging new features (to a submaintainer tree) might temporarily be put
> on hold in relation to the merge window, but for fixes, it is quite
> possible to both queue things (in a submaintainer tree), and to send
> accumulated fixes to Linus during the merge window.
>
> I did it myself just a few days ago, and Linus pulled it already:
> https://lore.kernel.org/linux-ide/171087658094.21820.15365015832308818327.pr-tracker-bot@kernel.org/T/#t
I am not maintainer of PCI. It was just my observation for pci subsystem.
Frank
>
>
> Kind regards,
> Niklas
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Frank Li <[email protected]>
> ---
>
> 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
>
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.
>
I'd rewrite the commit message as below:
"The mapping between PCI BAR and iATU inbound window are maintained in the
dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
existing mapping in the array and if it is not found (i.e., value in the array
indexed by the BAR is found to be 0), then it will allocate a new map value
using find_first_zero_bit().
The issue here is, the existing logic failed to consider the fact that the map
value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
'0' as the map value for BAR0 (note that it returns the first zero bit
position).
Due to this, when PERST# assert + deassert happens on the PERST# supported
platforms, the inbound window allocation restarts from BAR0 and the existing
logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
fact that it considers '0' as an invalid map value.
So fix this issue by always incrementing the map value before assigning to
bar_to_atu[] array and then decrementing it while fetching. This will make sure
that the map value '0' always represents the invalid mapping."
> Reported-by: Niklas Cassel <[email protected]>
> Closes: https://lore.kernel.org/linux-pci/ZXt2A+Fusfz3luQV@x1-carbon/T/#u
This is not the correct link that reported the issue. This one is:
https://lore.kernel.org/linux-pci/ZXsRp+Lzg3x%2Fnhk3@x1-carbon/
> Fixes: 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
> Reviewed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Frank Li <[email protected]>
With above mentioned changes,
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> ---
>
> 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
>
--
மணிவண்ணன் சதாசிவம்
On Thu, Mar 21, 2024 at 10:43:45PM +0530, Manivannan Sadhasivam wrote:
> 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.
>
> I'd rewrite the commit message as below:
>
> "The mapping between PCI BAR and iATU inbound window are maintained in the
> dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
> BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
> existing mapping in the array and if it is not found (i.e., value in the array
> indexed by the BAR is found to be 0), then it will allocate a new map value
> using find_first_zero_bit().
>
> The issue here is, the existing logic failed to consider the fact that the map
> value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
> '0' as the map value for BAR0 (note that it returns the first zero bit
> position).
>
> Due to this, when PERST# assert + deassert happens on the PERST# supported
> platforms, the inbound window allocation restarts from BAR0 and the existing
> logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
> fact that it considers '0' as an invalid map value.
>
> So fix this issue by always incrementing the map value before assigning to
> bar_to_atu[] array and then decrementing it while fetching. This will make sure
> that the map value '0' always represents the invalid mapping."
This translates C code to English in great detail, but still doesn't
tell me what's broken from a user's point of view, how urgent the fix
is, or how it should be handled.
DMA doesn't work because ATU setup is wrong? Driver MMIO access to
the device doesn't work? OS crashes? How? Incorrectly routed access
causes UR response? Happens on every boot? Only after a reboot or
controller reset? What platforms are affected? "PERST# supported
platforms" is not actionable without a lot of research or pre-existing
knowledge. Should this be backported to -stable?
Bjorn
On Thu, Mar 21, 2024 at 01:07:32PM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 21, 2024 at 10:43:45PM +0530, Manivannan Sadhasivam wrote:
> > 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.
> >
> > I'd rewrite the commit message as below:
> >
> > "The mapping between PCI BAR and iATU inbound window are maintained in the
> > dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
> > BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
> > existing mapping in the array and if it is not found (i.e., value in the array
> > indexed by the BAR is found to be 0), then it will allocate a new map value
> > using find_first_zero_bit().
> >
> > The issue here is, the existing logic failed to consider the fact that the map
> > value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
> > '0' as the map value for BAR0 (note that it returns the first zero bit
> > position).
> >
> > Due to this, when PERST# assert + deassert happens on the PERST# supported
> > platforms, the inbound window allocation restarts from BAR0 and the existing
> > logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
> > fact that it considers '0' as an invalid map value.
> >
> > So fix this issue by always incrementing the map value before assigning to
> > bar_to_atu[] array and then decrementing it while fetching. This will make sure
> > that the map value '0' always represents the invalid mapping."
>
> This translates C code to English in great detail, but still doesn't
> tell me what's broken from a user's point of view, how urgent the fix
> is, or how it should be handled.
>
> DMA doesn't work because ATU setup is wrong? Driver MMIO access to
> the device doesn't work? OS crashes? How? Incorrectly routed access
> causes UR response? Happens on every boot? Only after a reboot or
> controller reset? What platforms are affected? "PERST# supported
> platforms" is not actionable without a lot of research or pre-existing
> knowledge. Should this be backported to -stable?
>
Severity is less for the bug fixed by this patch. We have 8 inbound iATU windows
on almost all of the platforms and after PERST# assert + deassert, BAR0 uses map
'6' instead of '0'.
This has no user visibility since the mapping will go fine and we have only 6
BARs. So I'd not mark this as as critical fix that needs special attention.
Frank: Please ammend the commit message with the bug impact.
- Mani
--
மணிவண்ணன் சதாசிவம்
On Fri, Mar 22, 2024 at 10:56:23AM +0530, Manivannan Sadhasivam wrote:
> On Thu, Mar 21, 2024 at 01:07:32PM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 21, 2024 at 10:43:45PM +0530, Manivannan Sadhasivam wrote:
> > > 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.
> > >
> > > I'd rewrite the commit message as below:
> > >
> > > "The mapping between PCI BAR and iATU inbound window are maintained in the
> > > dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
> > > BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
> > > existing mapping in the array and if it is not found (i.e., value in the array
> > > indexed by the BAR is found to be 0), then it will allocate a new map value
> > > using find_first_zero_bit().
> > >
> > > The issue here is, the existing logic failed to consider the fact that the map
> > > value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
> > > '0' as the map value for BAR0 (note that it returns the first zero bit
> > > position).
> > >
> > > Due to this, when PERST# assert + deassert happens on the PERST# supported
> > > platforms, the inbound window allocation restarts from BAR0 and the existing
> > > logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
> > > fact that it considers '0' as an invalid map value.
> > >
> > > So fix this issue by always incrementing the map value before assigning to
> > > bar_to_atu[] array and then decrementing it while fetching. This will make sure
> > > that the map value '0' always represents the invalid mapping."
> >
> > This translates C code to English in great detail, but still doesn't
> > tell me what's broken from a user's point of view, how urgent the fix
> > is, or how it should be handled.
> >
> > DMA doesn't work because ATU setup is wrong? Driver MMIO access to
> > the device doesn't work? OS crashes? How? Incorrectly routed access
> > causes UR response? Happens on every boot? Only after a reboot or
> > controller reset? What platforms are affected? "PERST# supported
> > platforms" is not actionable without a lot of research or pre-existing
> > knowledge. Should this be backported to -stable?
> >
>
> Severity is less for the bug fixed by this patch. We have 8 inbound iATU windows
> on almost all of the platforms and after PERST# assert + deassert, BAR0 uses map
> '6' instead of '0'.
>
> This has no user visibility since the mapping will go fine and we have only 6
> BARs. So I'd not mark this as as critical fix that needs special attention.
So we will have 6 mappings configured, but only 5 BARs, so two mappings for
BAR0. The iATU looks at them in order, so index 0 will override index 6.
We are lucky that the endpoint subsystem does not clean up allocations properly
right now (you have an outstanding series which fixes this).
If the endpoint subsystem did clean up resources properly, we would DMA to the
area that was previously allocated for BAR0, instead of the new area for BAR0.
Perhaps just include this fix in your series that actually cleans up the BARs?
Kind regards,
Niklas
On Fri, Mar 22, 2024 at 07:19:01AM +0100, Niklas Cassel wrote:
> On Fri, Mar 22, 2024 at 10:56:23AM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Mar 21, 2024 at 01:07:32PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Mar 21, 2024 at 10:43:45PM +0530, Manivannan Sadhasivam wrote:
> > > > 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.
> > > >
> > > > I'd rewrite the commit message as below:
> > > >
> > > > "The mapping between PCI BAR and iATU inbound window are maintained in the
> > > > dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
> > > > BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
> > > > existing mapping in the array and if it is not found (i.e., value in the array
> > > > indexed by the BAR is found to be 0), then it will allocate a new map value
> > > > using find_first_zero_bit().
> > > >
> > > > The issue here is, the existing logic failed to consider the fact that the map
> > > > value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
> > > > '0' as the map value for BAR0 (note that it returns the first zero bit
> > > > position).
> > > >
> > > > Due to this, when PERST# assert + deassert happens on the PERST# supported
> > > > platforms, the inbound window allocation restarts from BAR0 and the existing
> > > > logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
> > > > fact that it considers '0' as an invalid map value.
> > > >
> > > > So fix this issue by always incrementing the map value before assigning to
> > > > bar_to_atu[] array and then decrementing it while fetching. This will make sure
> > > > that the map value '0' always represents the invalid mapping."
> > >
> > > This translates C code to English in great detail, but still doesn't
> > > tell me what's broken from a user's point of view, how urgent the fix
> > > is, or how it should be handled.
> > >
> > > DMA doesn't work because ATU setup is wrong? Driver MMIO access to
> > > the device doesn't work? OS crashes? How? Incorrectly routed access
> > > causes UR response? Happens on every boot? Only after a reboot or
> > > controller reset? What platforms are affected? "PERST# supported
> > > platforms" is not actionable without a lot of research or pre-existing
> > > knowledge. Should this be backported to -stable?
> >
> > Severity is less for the bug fixed by this patch. We have 8 inbound iATU windows
> > on almost all of the platforms and after PERST# assert + deassert, BAR0 uses map
> > '6' instead of '0'.
> >
> > This has no user visibility since the mapping will go fine and we have only 6
> > BARs. So I'd not mark this as as critical fix that needs special attention.
>
> So we will have 6 mappings configured, but only 5 BARs, so two mappings for
> BAR0. The iATU looks at them in order, so index 0 will override index 6.
Sounds like we dodge the bullet as long as the mappings for BAR 0 are
identical, which doesn't feel like much comfort.
> We are lucky that the endpoint subsystem does not clean up allocations properly
> right now (you have an outstanding series which fixes this).
>
> If the endpoint subsystem did clean up resources properly, we would DMA to the
> area that was previously allocated for BAR0, instead of the new area for BAR0.
This is the right level of abstraction for the commit log -- sounds
like there's some reset scenario where the pre-reset iATU windows are
not cleared out and we reallocate iATU windows, and we end up using
one of the stale windows instead of the new one, which could lead to
DMA to the wrong area. That incorrect DMA sounds like data corruption
in the right circumstances.
Of course it can *also* include some detail about the mechanism of why
that stale entry still exists and when it can be used.
Bjorn
On Fri, Mar 22, 2024 at 07:19:01AM +0100, Niklas Cassel wrote:
> On Fri, Mar 22, 2024 at 10:56:23AM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Mar 21, 2024 at 01:07:32PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Mar 21, 2024 at 10:43:45PM +0530, Manivannan Sadhasivam wrote:
> > > > 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.
> > > >
> > > > I'd rewrite the commit message as below:
> > > >
> > > > "The mapping between PCI BAR and iATU inbound window are maintained in the
> > > > dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
> > > > BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
> > > > existing mapping in the array and if it is not found (i.e., value in the array
> > > > indexed by the BAR is found to be 0), then it will allocate a new map value
> > > > using find_first_zero_bit().
> > > >
> > > > The issue here is, the existing logic failed to consider the fact that the map
> > > > value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
> > > > '0' as the map value for BAR0 (note that it returns the first zero bit
> > > > position).
> > > >
> > > > Due to this, when PERST# assert + deassert happens on the PERST# supported
> > > > platforms, the inbound window allocation restarts from BAR0 and the existing
> > > > logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
> > > > fact that it considers '0' as an invalid map value.
> > > >
> > > > So fix this issue by always incrementing the map value before assigning to
> > > > bar_to_atu[] array and then decrementing it while fetching. This will make sure
> > > > that the map value '0' always represents the invalid mapping."
> > >
> > > This translates C code to English in great detail, but still doesn't
> > > tell me what's broken from a user's point of view, how urgent the fix
> > > is, or how it should be handled.
> > >
> > > DMA doesn't work because ATU setup is wrong? Driver MMIO access to
> > > the device doesn't work? OS crashes? How? Incorrectly routed access
> > > causes UR response? Happens on every boot? Only after a reboot or
> > > controller reset? What platforms are affected? "PERST# supported
> > > platforms" is not actionable without a lot of research or pre-existing
> > > knowledge. Should this be backported to -stable?
> > >
> >
> > Severity is less for the bug fixed by this patch. We have 8 inbound iATU windows
> > on almost all of the platforms and after PERST# assert + deassert, BAR0 uses map
> > '6' instead of '0'.
> >
> > This has no user visibility since the mapping will go fine and we have only 6
> > BARs. So I'd not mark this as as critical fix that needs special attention.
>
> So we will have 6 mappings configured, but only 5 BARs, so two mappings for
> BAR0. The iATU looks at them in order, so index 0 will override index 6.
>
> We are lucky that the endpoint subsystem does not clean up allocations properly
> right now (you have an outstanding series which fixes this).
>
> If the endpoint subsystem did clean up resources properly, we would DMA to the
> area that was previously allocated for BAR0, instead of the new area for BAR0.
>
How would DMA happen to the previously allocated area? When the BARs are cleared
properly and then allocated again, BAR0 will get the map of 0 again and then the
iATU will map window 0 with BAR0. Only if we don't clear the BARs properly (as
like now), then it will result in BAR0 having 2 identical mappings and even with
that there won't be any issue since both map 0 and 6 are valid.
Am I missing anything?
> Perhaps just include this fix in your series that actually cleans up the BARs?
>
Yeah, makes sense. Once we agree on a finalized commit message in this thread,
I'll include this patch in my series.
- Mani
--
மணிவண்ணன் சதாசிவம்
On Tue, Apr 02, 2024 at 10:42:28AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Mar 22, 2024 at 07:19:01AM +0100, Niklas Cassel wrote:
> > On Fri, Mar 22, 2024 at 10:56:23AM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Mar 21, 2024 at 01:07:32PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Mar 21, 2024 at 10:43:45PM +0530, Manivannan Sadhasivam wrote:
> > > > > 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.
> > > > >
> > > > > I'd rewrite the commit message as below:
> > > > >
> > > > > "The mapping between PCI BAR and iATU inbound window are maintained in the
> > > > > dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
> > > > > BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
> > > > > existing mapping in the array and if it is not found (i.e., value in the array
> > > > > indexed by the BAR is found to be 0), then it will allocate a new map value
> > > > > using find_first_zero_bit().
> > > > >
> > > > > The issue here is, the existing logic failed to consider the fact that the map
> > > > > value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
> > > > > '0' as the map value for BAR0 (note that it returns the first zero bit
> > > > > position).
> > > > >
> > > > > Due to this, when PERST# assert + deassert happens on the PERST# supported
> > > > > platforms, the inbound window allocation restarts from BAR0 and the existing
> > > > > logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
> > > > > fact that it considers '0' as an invalid map value.
> > > > >
> > > > > So fix this issue by always incrementing the map value before assigning to
> > > > > bar_to_atu[] array and then decrementing it while fetching. This will make sure
> > > > > that the map value '0' always represents the invalid mapping."
> > > >
> > > > This translates C code to English in great detail, but still doesn't
> > > > tell me what's broken from a user's point of view, how urgent the fix
> > > > is, or how it should be handled.
> > > >
> > > > DMA doesn't work because ATU setup is wrong? Driver MMIO access to
> > > > the device doesn't work? OS crashes? How? Incorrectly routed access
> > > > causes UR response? Happens on every boot? Only after a reboot or
> > > > controller reset? What platforms are affected? "PERST# supported
> > > > platforms" is not actionable without a lot of research or pre-existing
> > > > knowledge. Should this be backported to -stable?
> > > >
> > >
> > > Severity is less for the bug fixed by this patch. We have 8 inbound iATU windows
> > > on almost all of the platforms and after PERST# assert + deassert, BAR0 uses map
> > > '6' instead of '0'.
> > >
> > > This has no user visibility since the mapping will go fine and we have only 6
> > > BARs. So I'd not mark this as as critical fix that needs special attention.
> >
> > So we will have 6 mappings configured, but only 5 BARs, so two mappings for
> > BAR0. The iATU looks at them in order, so index 0 will override index 6.
> >
> > We are lucky that the endpoint subsystem does not clean up allocations properly
> > right now (you have an outstanding series which fixes this).
> >
> > If the endpoint subsystem did clean up resources properly, we would DMA to the
> > area that was previously allocated for BAR0, instead of the new area for BAR0.
> >
>
> How would DMA happen to the previously allocated area? When the BARs are cleared
> properly and then allocated again, BAR0 will get the map of 0 again and then the
> iATU will map window 0 with BAR0. Only if we don't clear the BARs properly (as
> like now), then it will result in BAR0 having 2 identical mappings and even with
> that there won't be any issue since both map 0 and 6 are valid.
>
> Am I missing anything?
Like Bjorn summarize it:
"We dodge the bullet as long as the mappings for BAR 0 are identical,
which doesn't feel like much comfort."
Yes, right now we don't have a cleanup of either the backing memory for
the BAR, or the iATUs mapping the PCI address to backing memory.
(We allocate the backing memory for the BARs in .bind(), and free it in
unbind().)
So the superfluous iATU6 mapping will be the same as the iATU0 mapping.
After your series, we will still allocate and free the backing memory
in .bind()/.unbind(), but we will set/clear the iATU mapping in the
init()/.deinit() EPF callbacks.
>
> > Perhaps just include this fix in your series that actually cleans up the BARs?
> >
>
> Yeah, makes sense. Once we agree on a finalized commit message in this thread,
> I'll include this patch in my series.
I think that we have spent too much time on this patch already.
My suggestion is that you simply apply it to pci/endpoint branch directly and
fixup the commit message (like Bjorn usually does with [bhelgaas: commit log])
after Frank's Sign-off.
Kind regards,
Niklas
On Tue, Apr 02, 2024 at 11:23:49AM +0200, Niklas Cassel wrote:
> On Tue, Apr 02, 2024 at 10:42:28AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Mar 22, 2024 at 07:19:01AM +0100, Niklas Cassel wrote:
> > > On Fri, Mar 22, 2024 at 10:56:23AM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Mar 21, 2024 at 01:07:32PM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Mar 21, 2024 at 10:43:45PM +0530, Manivannan Sadhasivam wrote:
> > > > > > 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.
> > > > > >
> > > > > > I'd rewrite the commit message as below:
> > > > > >
> > > > > > "The mapping between PCI BAR and iATU inbound window are maintained in the
> > > > > > dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
> > > > > > BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
> > > > > > existing mapping in the array and if it is not found (i.e., value in the array
> > > > > > indexed by the BAR is found to be 0), then it will allocate a new map value
> > > > > > using find_first_zero_bit().
> > > > > >
> > > > > > The issue here is, the existing logic failed to consider the fact that the map
> > > > > > value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
> > > > > > '0' as the map value for BAR0 (note that it returns the first zero bit
> > > > > > position).
> > > > > >
> > > > > > Due to this, when PERST# assert + deassert happens on the PERST# supported
> > > > > > platforms, the inbound window allocation restarts from BAR0 and the existing
> > > > > > logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
> > > > > > fact that it considers '0' as an invalid map value.
> > > > > >
> > > > > > So fix this issue by always incrementing the map value before assigning to
> > > > > > bar_to_atu[] array and then decrementing it while fetching. This will make sure
> > > > > > that the map value '0' always represents the invalid mapping."
> > > > >
> > > > > This translates C code to English in great detail, but still doesn't
> > > > > tell me what's broken from a user's point of view, how urgent the fix
> > > > > is, or how it should be handled.
> > > > >
> > > > > DMA doesn't work because ATU setup is wrong? Driver MMIO access to
> > > > > the device doesn't work? OS crashes? How? Incorrectly routed access
> > > > > causes UR response? Happens on every boot? Only after a reboot or
> > > > > controller reset? What platforms are affected? "PERST# supported
> > > > > platforms" is not actionable without a lot of research or pre-existing
> > > > > knowledge. Should this be backported to -stable?
> > > > >
> > > >
> > > > Severity is less for the bug fixed by this patch. We have 8 inbound iATU windows
> > > > on almost all of the platforms and after PERST# assert + deassert, BAR0 uses map
> > > > '6' instead of '0'.
> > > >
> > > > This has no user visibility since the mapping will go fine and we have only 6
> > > > BARs. So I'd not mark this as as critical fix that needs special attention.
> > >
> > > So we will have 6 mappings configured, but only 5 BARs, so two mappings for
> > > BAR0. The iATU looks at them in order, so index 0 will override index 6.
> > >
> > > We are lucky that the endpoint subsystem does not clean up allocations properly
> > > right now (you have an outstanding series which fixes this).
> > >
> > > If the endpoint subsystem did clean up resources properly, we would DMA to the
> > > area that was previously allocated for BAR0, instead of the new area for BAR0.
> > >
> >
> > How would DMA happen to the previously allocated area? When the BARs are cleared
> > properly and then allocated again, BAR0 will get the map of 0 again and then the
> > iATU will map window 0 with BAR0. Only if we don't clear the BARs properly (as
> > like now), then it will result in BAR0 having 2 identical mappings and even with
> > that there won't be any issue since both map 0 and 6 are valid.
> >
> > Am I missing anything?
>
> Like Bjorn summarize it:
> "We dodge the bullet as long as the mappings for BAR 0 are identical,
> which doesn't feel like much comfort."
>
> Yes, right now we don't have a cleanup of either the backing memory for
> the BAR, or the iATUs mapping the PCI address to backing memory.
> (We allocate the backing memory for the BARs in .bind(), and free it in
> unbind().)
>
> So the superfluous iATU6 mapping will be the same as the iATU0 mapping.
>
> After your series, we will still allocate and free the backing memory
> in .bind()/.unbind(), but we will set/clear the iATU mapping in the
> .init()/.deinit() EPF callbacks.
>
>
> >
> > > Perhaps just include this fix in your series that actually cleans up the BARs?
> > >
> >
> > Yeah, makes sense. Once we agree on a finalized commit message in this thread,
> > I'll include this patch in my series.
>
> I think that we have spent too much time on this patch already.
>
> My suggestion is that you simply apply it to pci/endpoint branch directly and
> fixup the commit message (like Bjorn usually does with [bhelgaas: commit log])
> after Frank's Sign-off.
I combined mani and your suggested message to
https://lore.kernel.org/imx/[email protected]/
I am okay to fine tune it by yourself.
Frank
>
>
> Kind regards,
> Niklas