diff mbox series

[v3,1/1] PCI: dwc: Fix index 0 incorrectly being interpreted as a free ATU slot

Message ID 20240326193540.3610570-1-Frank.Li@nxp.com (mailing list archive)
State Superseded
Headers show
Series [v3,1/1] PCI: dwc: Fix index 0 incorrectly being interpreted as a free ATU slot | expand

Commit Message

Frank Li March 26, 2024, 7:35 p.m. UTC 
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.

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>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---

Notes:
    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(-)

Comments

Niklas Cassel April 8, 2024, 10:23 a.m. UTC | #1 
On Tue, Mar 26, 2024 at 03:35:40PM -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.

Nit: If we want additional clarity, we could also add:
""
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.
""

If the maintainers feel like this additional information is important, I think
it could be added while applying. (But I also think that the existing commit
message is detailed enough to be applied as is.)


> 
> 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>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 
> Notes:
>     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
>
Manivannan Sadhasivam April 10, 2024, 6:03 p.m. UTC | #2 
On Mon, Apr 08, 2024 at 12:23:20PM +0200, Niklas Cassel wrote:
> On Tue, Mar 26, 2024 at 03:35:40PM -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.
> 
> Nit: If we want additional clarity, we could also add:
> ""
> 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.
> ""

Since Bjorn asked for the above info, it should get added.

With that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> 
> If the maintainers feel like this additional information is important, I think
> it could be added while applying. (But I also think that the existing commit
> message is detailed enough to be applied as is.)
> 
> 
> > 
> > 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>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > 
> > Notes:
> >     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
> > 
>
Frank Li April 10, 2024, 6:16 p.m. UTC | #3 
On Wed, Apr 10, 2024 at 11:33:41PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Apr 08, 2024 at 12:23:20PM +0200, Niklas Cassel wrote:
> > On Tue, Mar 26, 2024 at 03:35:40PM -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.
> > 
> > Nit: If we want additional clarity, we could also add:
> > ""
> > 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.
> > ""
> 
> Since Bjorn asked for the above info, it should get added.
> 
> With that,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Mani:

Do you need me rework patch? Or you can handle it by yourself when apply?

Frank

> 
> - Mani
> 
> > 
> > If the maintainers feel like this additional information is important, I think
> > it could be added while applying. (But I also think that the existing commit
> > message is detailed enough to be applied as is.)
> > 
> > 
> > > 
> > > 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>
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > 
> > > Notes:
> > >     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
> > > 
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
Niklas Cassel April 12, 2024, 1:37 p.m. UTC | #4 
On Wed, Apr 10, 2024 at 02:16:52PM -0400, Frank Li wrote:
> On Wed, Apr 10, 2024 at 11:33:41PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Apr 08, 2024 at 12:23:20PM +0200, Niklas Cassel wrote:
> > > On Tue, Mar 26, 2024 at 03:35:40PM -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.
> > > 
> > > Nit: If we want additional clarity, we could also add:
> > > ""
> > > 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.
> > > ""
> > 
> > Since Bjorn asked for the above info, it should get added.
> > 
> > With that,
> > 
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Mani:
> 
> Do you need me rework patch? Or you can handle it by yourself when apply?

It appears that Krzysztof is the one picking up patches for:
drivers/pci/controller/dwc/

Since he has the bottommost Signed-off-by on patches in:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=controller%2Fdwc

So I don't think that Mani can amend it, since he is most likely not the
one picking up this patch.

The fastest way is probably just to send out a v4.
(But I feel you... this patch has already taken too long :P)


Kind regards,
Niklas
diff mbox series

Patch

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);