| Message ID | 20240412160841.925927-1-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [v4,1/1] PCI: dwc: Fix index 0 incorrectly being interpreted as a free ATU slot | expand |
On Fri, Apr 12, 2024 at 12:08:41PM -0400, Frank Li wrote: > When PERST# assert and deassert happens on the PERST# supported platforms, > the both iATU0 and iATU6 will map inbound window to BAR0. DMA will access > to the area that was previously allocated (iATU0) for BAR0, instead of the > new area (iATU6) for BAR0. > > Right now, we dodge the bullet because both iATU0 and iATU6 should > currently translate inbound accesses to BAR0 to the same allocated memory > area. However, having two separate inbound mappings for the same BAR is a > disaster waiting to happen. > > 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 <Niklas.Cassel@wdc.com> > Closes: https://lore.kernel.org/linux-pci/ZXsRp+Lzg3x%2Fnhk3@x1-carbon/ > Tested-by: Niklas Cassel <niklas.cassel@wdc.com> > Fixes: 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address") > Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- Anyone take care this patch? Krzysztof Wilczyński, Lorenzo? Frank > > Notes: > Change from v3 to v4 > - commit message add "Right now, we dodge..." > - Add Reviewed-by: Manivannan Sadhasivam > > Change from v2 to v3 > - Add impact in commit message > - Add mani's detail description > - Fix Closes link > > 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 >
Hello, > When PERST# assert and deassert happens on the PERST# supported platforms, > the both iATU0 and iATU6 will map inbound window to BAR0. DMA will access > to the area that was previously allocated (iATU0) for BAR0, instead of the > new area (iATU6) for BAR0. > > Right now, we dodge the bullet because both iATU0 and iATU6 should > currently translate inbound accesses to BAR0 to the same allocated memory > area. However, having two separate inbound mappings for the same BAR is a > disaster waiting to happen. > > 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." Applied to controller/dwc, thank you! [1/1] PCI: dwc: Fix index 0 incorrectly being interpreted as a free ATU slot https://git.kernel.org/pci/pci/c/cd3c2f0fff46 Krzysztof
On Sat, May 18, 2024 at 02:06:50AM +0900, Krzysztof Wilczyński wrote: > Hello, > > > When PERST# assert and deassert happens on the PERST# supported platforms, > > the both iATU0 and iATU6 will map inbound window to BAR0. DMA will access > > to the area that was previously allocated (iATU0) for BAR0, instead of the > > new area (iATU6) for BAR0. > > > > Right now, we dodge the bullet because both iATU0 and iATU6 should > > currently translate inbound accesses to BAR0 to the same allocated memory > > area. However, having two separate inbound mappings for the same BAR is a > > disaster waiting to happen. > > > > 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." > > Applied to controller/dwc, thank you! > > [1/1] PCI: dwc: Fix index 0 incorrectly being interpreted as a free ATU slot > https://git.kernel.org/pci/pci/c/cd3c2f0fff46 > > Krzysztof Hello PCI maintainers, There was a message sent out that this patch was applied, yet the patch does not appear to be part of the pull request that was sent out yesterday: https://lore.kernel.org/linux-pci/20240520222943.GA7973@bhelgaas/T/#u In fact, there seems to be many PCI patches that have been reviewed and ready to be included (some of them for months) that is not part of the pull request. Looking at pci/next, these patches do not appear there either, so I assume that these patches will also not be included in a follow-up pull request. Some of these patches are actual fixes, like the patch in $subject, and do not appear to depend on any other patches, so what is the reason for not including them in the PCI pull request? Kind regards, Niklas
On Tue, May 21, 2024 at 12:16:55PM +0200, Niklas Cassel wrote: > On Sat, May 18, 2024 at 02:06:50AM +0900, Krzysztof Wilczyński wrote: > > Hello, > > > > > When PERST# assert and deassert happens on the PERST# supported platforms, > > > the both iATU0 and iATU6 will map inbound window to BAR0. DMA will access > > > to the area that was previously allocated (iATU0) for BAR0, instead of the > > > new area (iATU6) for BAR0. > > > > > > Right now, we dodge the bullet because both iATU0 and iATU6 should > > > currently translate inbound accesses to BAR0 to the same allocated memory > > > area. However, having two separate inbound mappings for the same BAR is a > > > disaster waiting to happen. > > > > > > 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." > > > > Applied to controller/dwc, thank you! > > > > [1/1] PCI: dwc: Fix index 0 incorrectly being interpreted as a free ATU slot > > https://git.kernel.org/pci/pci/c/cd3c2f0fff46 > > > > Krzysztof > > Hello PCI maintainers, > > There was a message sent out that this patch was applied, yet the patch does > not appear to be part of the pull request that was sent out yesterday: > https://lore.kernel.org/linux-pci/20240520222943.GA7973@bhelgaas/T/#u > > In fact, there seems to be many PCI patches that have been reviewed and ready > to be included (some of them for months) that is not part of the pull request. > > Looking at pci/next, these patches do not appear there either, so I assume > that these patches will also not be included in a follow-up pull request. > > Some of these patches are actual fixes, like the patch in $subject, and do not > appear to depend on any other patches, so what is the reason for not including > them in the PCI pull request? The problem was that we didn't get these applied soon enough for them to get any time in linux-next before the merge window opened. I don't like to add non-trivial things during the merge window, so I deferred most of these. I plan to get them in linux-next as soon as v6.10-rc1 is tagged. If we can make a case for post-merge window fixes, e.g., to fix a regression in the pull request or other serious issue, that's always a possibility. Bjorn
On 2024/05/21 16:14, Bjorn Helgaas wrote: > On Tue, May 21, 2024 at 12:16:55PM +0200, Niklas Cassel wrote: >> On Sat, May 18, 2024 at 02:06:50AM +0900, Krzysztof Wilczyński wrote: >>> Hello, >>> >>>> When PERST# assert and deassert happens on the PERST# supported platforms, >>>> the both iATU0 and iATU6 will map inbound window to BAR0. DMA will access >>>> to the area that was previously allocated (iATU0) for BAR0, instead of the >>>> new area (iATU6) for BAR0. >>>> >>>> Right now, we dodge the bullet because both iATU0 and iATU6 should >>>> currently translate inbound accesses to BAR0 to the same allocated memory >>>> area. However, having two separate inbound mappings for the same BAR is a >>>> disaster waiting to happen. >>>> >>>> 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." >>> >>> Applied to controller/dwc, thank you! >>> >>> [1/1] PCI: dwc: Fix index 0 incorrectly being interpreted as a free ATU slot >>> https://git.kernel.org/pci/pci/c/cd3c2f0fff46 >>> >>> Krzysztof >> >> Hello PCI maintainers, >> >> There was a message sent out that this patch was applied, yet the patch does >> not appear to be part of the pull request that was sent out yesterday: >> https://lore.kernel.org/linux-pci/20240520222943.GA7973@bhelgaas/T/#u >> >> In fact, there seems to be many PCI patches that have been reviewed and ready >> to be included (some of them for months) that is not part of the pull request. >> >> Looking at pci/next, these patches do not appear there either, so I assume >> that these patches will also not be included in a follow-up pull request. >> >> Some of these patches are actual fixes, like the patch in $subject, and do not >> appear to depend on any other patches, so what is the reason for not including >> them in the PCI pull request? > > The problem was that we didn't get these applied soon enough for them > to get any time in linux-next before the merge window opened. I don't > like to add non-trivial things during the merge window, so I deferred > most of these. I plan to get them in linux-next as soon as v6.10-rc1 > is tagged. We understand that, and we agree with this. However, the point was more about WHY these patches were not applied earlier as many of them were fully reviewed for several weeks. We have more patches to send out that depend on all these deferred patches, and this deferring is delaying us as well. It would be great if going forward, a more timely processing & applying of reviewed patches happened. Thank you. > > If we can make a case for post-merge window fixes, e.g., to fix a > regression in the pull request or other serious issue, that's always a > possibility. > > Bjorn
Hello, [...] > It would be great if going forward, a more timely processing & applying > of reviewed patches happened. Thank you. Agreed. I will see about improving this going forward. Sorry for troubles! Krzysztof
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);