LinuxLists.cc - [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

2023-12-19 04:49:37

Subject: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

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 <[email protected]>
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 <[email protected]>
---

Notes:
@Niklas:
I have not test your case. I should be equal to previous's fix in
mail list.

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

2023-12-19 04:50:39

by Frank Li

[permalink] [raw]

Subject: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

2023-12-19 10:07:28

by Niklas Cassel

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

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 <[email protected]>
> 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 <[email protected]>
> ---
>
> 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:

Reviewed-by: Niklas Cassel <[email protected]>

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

2023-12-19 14:20:43

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

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 <[email protected]>
> > 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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
>
> >
> > 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
> >

2023-12-19 14:49:25

by Niklas Cassel

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> 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 <[email protected]>
> > > 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 <[email protected]>
> > > ---
> > >
> > > 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.

A s8 can hold a max value of 127.
CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.

Since the DWC code can be synthesized with 256 iATUs,
your code will not work on systems with 128 or more iATUs.

If we continue to use a u8, and offset the saved value by one,
we will at least be able to support 255-1 == 254 iATUs.

Kind regards,
Niklas

2024-01-09 13:52:40

by Niklas Cassel

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

Hello Frank,

On Tue, Dec 19, 2023 at 03:38:33PM +0100, Niklas Cassel wrote:
> On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > 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 <[email protected]>
> > > > 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 <[email protected]>
> > > > ---
> > > >
> > > > 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.
>
> A s8 can hold a max value of 127.
> CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
>
> Since the DWC code can be synthesized with 256 iATUs,
> your code will not work on systems with 128 or more iATUs.
>
> If we continue to use a u8, and offset the saved value by one,
> we will at least be able to support 255-1 == 254 iATUs.

Do you plan to send out a v2?

Kind regards,
Niklas

2024-01-09 15:04:44

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Tue, Jan 09, 2024 at 01:52:15PM +0000, Niklas Cassel wrote:
> Hello Frank,
>
> On Tue, Dec 19, 2023 at 03:38:33PM +0100, Niklas Cassel wrote:
> > On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > > 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 <[email protected]>
> > > > > 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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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.
> >
> > A s8 can hold a max value of 127.
> > CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
> >
> > Since the DWC code can be synthesized with 256 iATUs,
> > your code will not work on systems with 128 or more iATUs.
> >
> > If we continue to use a u8, and offset the saved value by one,
> > we will at least be able to support 255-1 == 254 iATUs.
>
> Do you plan to send out a v2?

It is easy to change to u16. But I hope Manivannan Sadhasivam have time to
review it.

Frank

>
>
> Kind regards,
> Niklas

2024-01-24 17:11:36

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Tue, Jan 09, 2024 at 01:52:15PM +0000, Niklas Cassel wrote:
> Hello Frank,
>
> On Tue, Dec 19, 2023 at 03:38:33PM +0100, Niklas Cassel wrote:
> > On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > > 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 <[email protected]>
> > > > > 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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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.
> >
> > A s8 can hold a max value of 127.
> > CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
> >
> > Since the DWC code can be synthesized with 256 iATUs,
> > your code will not work on systems with 128 or more iATUs.
> >
> > If we continue to use a u8, and offset the saved value by one,
> > we will at least be able to support 255-1 == 254 iATUs.
>
> Do you plan to send out a v2?

@mani:
Do you have any comments about this fixes except u8's problem?

Frank

>
>
> Kind regards,
> Niklas

2024-01-30 06:29:57

by Manivannan Sadhasivam

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Wed, Jan 24, 2024 at 12:10:01PM -0500, Frank Li wrote:
> On Tue, Jan 09, 2024 at 01:52:15PM +0000, Niklas Cassel wrote:
> > Hello Frank,
> >
> > On Tue, Dec 19, 2023 at 03:38:33PM +0100, Niklas Cassel wrote:
> > > On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > > > 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 <[email protected]>
> > > > > > 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 <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > 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.
> > >
> > > A s8 can hold a max value of 127.
> > > CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
> > >
> > > Since the DWC code can be synthesized with 256 iATUs,
> > > your code will not work on systems with 128 or more iATUs.
> > >
> > > If we continue to use a u8, and offset the saved value by one,
> > > we will at least be able to support 255-1 == 254 iATUs.
> >
> > Do you plan to send out a v2?
>
> @mani:
> Do you have any comments about this fixes except u8's problem?

IMO, the core issue lies in the EPF driver. It calls set_bar() during init, but
it is not clearing the BARs with clear_bar() during LINK_DOWN.

If the BARs were cleared properly, then we would not see this issue.

I'm planning to do a cleanup of the behavior of EPF with core_init_notifier and
it should get addressed there.

- Mani

--
மணிவண்ணன் சதாசிவம்

2024-02-29 12:03:30

by Niklas Cassel

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Tue, Jan 30, 2024 at 11:59:38AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jan 24, 2024 at 12:10:01PM -0500, Frank Li wrote:
> > On Tue, Jan 09, 2024 at 01:52:15PM +0000, Niklas Cassel wrote:
> > > Hello Frank,
> > >
> > > On Tue, Dec 19, 2023 at 03:38:33PM +0100, Niklas Cassel wrote:
> > > > On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > > > > 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 <[email protected]>
> > > > > > > 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 <[email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > > 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.
> > > >
> > > > A s8 can hold a max value of 127.
> > > > CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
> > > >
> > > > Since the DWC code can be synthesized with 256 iATUs,
> > > > your code will not work on systems with 128 or more iATUs.
> > > >
> > > > If we continue to use a u8, and offset the saved value by one,
> > > > we will at least be able to support 255-1 == 254 iATUs.
> > >
> > > Do you plan to send out a v2?
> >
> > @mani:
> > Do you have any comments about this fixes except u8's problem?
>
> IMO, the core issue lies in the EPF driver. It calls set_bar() during init, but
> it is not clearing the BARs with clear_bar() during LINK_DOWN.
>
> If the BARs were cleared properly, then we would not see this issue.
>
> I'm planning to do a cleanup of the behavior of EPF with core_init_notifier and
> it should get addressed there.

Hello Mani, Frank,

Please reconsider respinning this.

I think that the invalid usage of find_first_zero_bit() is very bad,
because other people might copy the find_first_zero_bit() usage here.

find_first_zero_bit() can (and will) return 0, so this check is just bad:

if (!ep->bar_to_atu[bar])
free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);

..

ep->bar_to_atu[bar] = free_win;
set_bit(free_win, ep->ib_window_map);

No matter which code Mani adds that cleans up the BARs, will not change that
the logic is this function is just broken.

Kind regards,
Niklas

2024-02-29 20:14:06

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Thu, Feb 29, 2024 at 01:03:14PM +0100, Niklas Cassel wrote:
> On Tue, Jan 30, 2024 at 11:59:38AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jan 24, 2024 at 12:10:01PM -0500, Frank Li wrote:
> > > On Tue, Jan 09, 2024 at 01:52:15PM +0000, Niklas Cassel wrote:
> > > > Hello Frank,
> > > >
> > > > On Tue, Dec 19, 2023 at 03:38:33PM +0100, Niklas Cassel wrote:
> > > > > On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > > > > > 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 <[email protected]>
> > > > > > > > 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 <[email protected]>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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.
> > > > >
> > > > > A s8 can hold a max value of 127.
> > > > > CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
> > > > >
> > > > > Since the DWC code can be synthesized with 256 iATUs,
> > > > > your code will not work on systems with 128 or more iATUs.
> > > > >
> > > > > If we continue to use a u8, and offset the saved value by one,
> > > > > we will at least be able to support 255-1 == 254 iATUs.
> > > >
> > > > Do you plan to send out a v2?
> > >
> > > @mani:
> > > Do you have any comments about this fixes except u8's problem?
> >
> > IMO, the core issue lies in the EPF driver. It calls set_bar() during init, but
> > it is not clearing the BARs with clear_bar() during LINK_DOWN.
> >
> > If the BARs were cleared properly, then we would not see this issue.
> >
> > I'm planning to do a cleanup of the behavior of EPF with core_init_notifier and
> > it should get addressed there.
>
> Hello Mani, Frank,
>
> Please reconsider respinning this.
>
> I think that the invalid usage of find_first_zero_bit() is very bad,
> because other people might copy the find_first_zero_bit() usage here.
>
>
> find_first_zero_bit() can (and will) return 0, so this check is just bad:
>
> if (!ep->bar_to_atu[bar])
> free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
>
> ...
>
> ep->bar_to_atu[bar] = free_win;
> set_bit(free_win, ep->ib_window_map);
>
>
>
> No matter which code Mani adds that cleans up the BARs, will not change that
> the logic is this function is just broken.

Hello Mani:
if you agree this fix, I can respin this patch by fix s8 problem.

Frank

>
>
> Kind regards,
> Niklas

2024-03-04 08:54:31

by Manivannan Sadhasivam

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Tue, Dec 19, 2023 at 02:38:34PM +0000, Niklas Cassel wrote:
> On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > 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 <[email protected]>
> > > > 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 <[email protected]>
> > > > ---
> > > >
> > > > 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.
>
> A s8 can hold a max value of 127.
> CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
>
> Since the DWC code can be synthesized with 256 iATUs,
> your code will not work on systems with 128 or more iATUs.
>
> If we continue to use a u8, and offset the saved value by one,
> we will at least be able to support 255-1 == 254 iATUs.
>

Agree. I cannot suggest a better alternative. So let's go with this. But please
add a comment before bar_to_atu assignment to make it clear. Like,

/*
* Always increment free_win before assignment, since value 0 is used to
* identify unallocated mapping.
*/
ep->bar_to_atu[bar] = free_win + 1;

- Mani

>
> Kind regards,
> Niklas

--
மணிவண்ணன் சதாசிவம்

2024-03-04 17:24:52

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Mon, Mar 04, 2024 at 02:18:41PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Dec 19, 2023 at 02:38:34PM +0000, Niklas Cassel wrote:
> > On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > > 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 <[email protected]>
> > > > > 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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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.
> >
> > A s8 can hold a max value of 127.
> > CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
> >
> > Since the DWC code can be synthesized with 256 iATUs,
> > your code will not work on systems with 128 or more iATUs.
> >
> > If we continue to use a u8, and offset the saved value by one,
> > we will at least be able to support 255-1 == 254 iATUs.
> >
>
> Agree. I cannot suggest a better alternative. So let's go with this. But please
> add a comment before bar_to_atu assignment to make it clear. Like,
>
> /*
> * Always increment free_win before assignment, since value 0 is used to
> * identify unallocated mapping.
> */
> ep->bar_to_atu[bar] = free_win + 1;

This patch already change to use "-1" as free. Only issue for this patch is
that use 's16' instead of 's8' becasue max ATU number is 255.

Frank

>
> - Mani
>
> >
> > Kind regards,
> > Niklas
>
> --
> மணிவண்ணன் சதாசிவம்

2024-03-04 18:10:35

by Manivannan Sadhasivam

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Mon, Mar 04, 2024 at 11:47:36AM -0500, Frank Li wrote:
> On Mon, Mar 04, 2024 at 02:18:41PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Dec 19, 2023 at 02:38:34PM +0000, Niklas Cassel wrote:
> > > On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > > > 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 <[email protected]>
> > > > > > 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 <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > 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.
> > >
> > > A s8 can hold a max value of 127.
> > > CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
> > >
> > > Since the DWC code can be synthesized with 256 iATUs,
> > > your code will not work on systems with 128 or more iATUs.
> > >
> > > If we continue to use a u8, and offset the saved value by one,
> > > we will at least be able to support 255-1 == 254 iATUs.
> > >
> >
> > Agree. I cannot suggest a better alternative. So let's go with this. But please
> > add a comment before bar_to_atu assignment to make it clear. Like,
> >
> > /*
> > * Always increment free_win before assignment, since value 0 is used to
> > * identify unallocated mapping.
> > */
> > ep->bar_to_atu[bar] = free_win + 1;
>
> This patch already change to use "-1" as free. Only issue for this patch is
> that use 's16' instead of 's8' becasue max ATU number is 255.
>

Niklas's initial suggestion of keeping u8 for the array and 0 as the unallocated
placeholder sounds good to me. Please use that instead.

Even though iATU window index starts from 0, the comment I suggested will
clarify what this bar_to_atu[] does.

- Mani

--
மணிவண்ணன் சதாசிவம்

2024-03-04 19:15:29

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Mon, Mar 04, 2024 at 11:40:05PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 04, 2024 at 11:47:36AM -0500, Frank Li wrote:
> > On Mon, Mar 04, 2024 at 02:18:41PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Dec 19, 2023 at 02:38:34PM +0000, Niklas Cassel wrote:
> > > > On Tue, Dec 19, 2023 at 09:20:21AM -0500, Frank Li wrote:
> > > > > 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 <[email protected]>
> > > > > > > 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 <[email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > > 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.
> > > >
> > > > A s8 can hold a max value of 127.
> > > > CX_ATU_NUM_OUTBOUND_REGIONS seems to be 0-255.
> > > >
> > > > Since the DWC code can be synthesized with 256 iATUs,
> > > > your code will not work on systems with 128 or more iATUs.
> > > >
> > > > If we continue to use a u8, and offset the saved value by one,
> > > > we will at least be able to support 255-1 == 254 iATUs.
> > > >
> > >
> > > Agree. I cannot suggest a better alternative. So let's go with this. But please
> > > add a comment before bar_to_atu assignment to make it clear. Like,
> > >
> > > /*
> > > * Always increment free_win before assignment, since value 0 is used to
> > > * identify unallocated mapping.
> > > */
> > > ep->bar_to_atu[bar] = free_win + 1;
> >
> > This patch already change to use "-1" as free. Only issue for this patch is
> > that use 's16' instead of 's8' becasue max ATU number is 255.
> >
>
> Niklas's initial suggestion of keeping u8 for the array and 0 as the unallocated
> placeholder sounds good to me. Please use that instead.
>

It is impossible to keep u8, because 255 + 1 will 0 for u8. Previously
Niklas's initial suggestion have not consider this condition. If u8 have to
change to u16 or s16 anyways, I prefer use -1 as free.

Frank

> Even though iATU window index starts from 0, the comment I suggested will
> clarify what this bar_to_atu[] does.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

2024-03-04 21:06:53

by Niklas Cassel

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Mon, Mar 04, 2024 at 02:13:27PM -0500, Frank Li wrote:
> >
> > Niklas's initial suggestion of keeping u8 for the array and 0 as the unallocated
> > placeholder sounds good to me. Please use that instead.
> >
>
> It is impossible to keep u8, because 255 + 1 will 0 for u8. Previously
> Niklas's initial suggestion have not consider this condition. If u8 have to
> change to u16 or s16 anyways, I prefer use -1 as free.

Well, to be fair, my suggestion was:
"If we continue to use a u8, and offset the saved value by one,
we will at least be able to support 255-1 == 254 iATUs."

But we have this define:
drivers/pci/controller/dwc/pcie-designware.h:#define MAX_IATU_IN 256
(Even if it isn't used anywhere.)

But as ridiculous as it may seem to support that many inbound ranges,
that is the max number of windows supported by the hardware, so why
not just let the driver support the max supported by the hardware?

We are talking about:
struct dw_pcie_ep {
...
u8 bar_to_atu[PCI_STD_NUM_BARS];
...
}

where PCI_STD_NUM_BARS == 6.

And where struct dw_pcie_ep is kzalloced for what I assume is all drivers.

So I'm actually for your idea of changing it to u16, or even unsigned int.

If the code is simplified if we use a u16 or unsigned int (because we don't
need any weird if (val < MAX_IATU_IN - 1) check), then I'm all for it.

What I personally don't like with your patch in $subject,
was that you changed both dw_pcie_ep_clear_bar() to set the "clear value"
to -1, but you also need a
memset(ep->bar_to_atu, -1, sizeof(ep->bar_to_atu)); in dw_pcie_ep_init().

I much prefer to have 0 as the default/unused value, because it will
automatically get set when you do kzalloc().
Seeing a memset -1 just looks a bit hackish to be, but I realize that
it is personal preference.

If it is super important to save 8 bytes from the heap, then I would
even prefer changing the MAX_IATU_IN 256 to something smaller, like
127 or whatever, just as long as we don't need that extra memset -1 :)

Kind regards,
Niklas

2024-03-04 21:17:48

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

On Mon, Mar 04, 2024 at 09:04:30PM +0000, Niklas Cassel wrote:
> On Mon, Mar 04, 2024 at 02:13:27PM -0500, Frank Li wrote:
> > >
> > > Niklas's initial suggestion of keeping u8 for the array and 0 as the unallocated
> > > placeholder sounds good to me. Please use that instead.
> > >
> >
> > It is impossible to keep u8, because 255 + 1 will 0 for u8. Previously
> > Niklas's initial suggestion have not consider this condition. If u8 have to
> > change to u16 or s16 anyways, I prefer use -1 as free.
>
> Well, to be fair, my suggestion was:
> "If we continue to use a u8, and offset the saved value by one,
> we will at least be able to support 255-1 == 254 iATUs."
>
> But we have this define:
> drivers/pci/controller/dwc/pcie-designware.h:#define MAX_IATU_IN 256
> (Even if it isn't used anywhere.)
>
> But as ridiculous as it may seem to support that many inbound ranges,
> that is the max number of windows supported by the hardware, so why
> not just let the driver support the max supported by the hardware?
>
>
> We are talking about:
> struct dw_pcie_ep {
> ...
> u8 bar_to_atu[PCI_STD_NUM_BARS];
> ...
> }
>
> where PCI_STD_NUM_BARS == 6.
>
> And where struct dw_pcie_ep is kzalloced for what I assume is all drivers.
>
> So I'm actually for your idea of changing it to u16, or even unsigned int.
>
> If the code is simplified if we use a u16 or unsigned int (because we don't
> need any weird if (val < MAX_IATU_IN - 1) check), then I'm all for it.
>
>
> What I personally don't like with your patch in $subject,
> was that you changed both dw_pcie_ep_clear_bar() to set the "clear value"
> to -1, but you also need a
> memset(ep->bar_to_atu, -1, sizeof(ep->bar_to_atu)); in dw_pcie_ep_init().
>
>
> I much prefer to have 0 as the default/unused value, because it will
> automatically get set when you do kzalloc().
> Seeing a memset -1 just looks a bit hackish to be, but I realize that
> it is personal preference.
>
>
> If it is super important to save 8 bytes from the heap, then I would
> even prefer changing the MAX_IATU_IN 256 to something smaller, like
> 127 or whatever, just as long as we don't need that extra memset -1 :)

Okay, Let me work on '0' version.

Frank

>
>
> Kind regards,
> Niklas